PaletteEdit: improve unit tests #57645

t-hamano · 2024-01-08T13:52:38Z

Preparation for solving #57309

What?

This PR updates the unit tests for the PaletteEdit component to cover as many scenarios as possible.

Why?

To prevent regressions and bugs and to clarify component specifications.

How?

I wrote the tests to cover as many scenarios as possible based on the current specifications.

Testing Instructions

Run npm run test:unit packages/components/src/palette-edit/test/index.tsx

t-hamano · 2024-01-08T13:56:21Z

packages/components/src/palette-edit/test/index.tsx

+			} )
+		);
+
+		await waitFor( () => {


onChange is debounced, so we need to use waitFor. It appears to have been debounced for performance improvements. See #40285.

To better mimic actual user behavior, it might be better to click the "Done" button rather than do a wait for the onChange.

I don't think onChange is called because the "Done" button only clears the internal editing state. Strictly speaking, this button should probably be called "Back" or "Exit Edit Mode"...

Fair point.

t-hamano · 2024-01-08T13:57:39Z

packages/components/src/palette-edit/test/index.tsx

-	it( 'opens color selector for color palettes', () => {
-		render( <PaletteEdit { ...defaultProps } /> );
-		fireEvent.click( screen.getByLabelText( 'Color: Base' ) );
-		expect( screen.getByLabelText( 'Hex color' ) ).toBeInTheDocument();


This test should be covered by the "can update color palette value" test.

t-hamano · 2024-01-08T13:59:59Z

packages/components/src/palette-edit/test/index.tsx

+		await user.selectOptions( typeSelectElement, 'radial-gradient' );
+
+		await waitFor( () => {
+			expect( defaultProps.onChange ).toHaveBeenLastCalledWith( [


Somehow I had to use toHaveBeenLastCalledWith here. Where I used toHaveBeenCalledTimes, received number of calls was 5, so maybe that has something to do with it.

This is because we're reusing the jest.fn() mock that was set on defaultProps.onChange. We would need to do a reset between tests (e.g. jest.resetAllMocks() in a afterEach()) if we wanted to do it that way. Or we usually just use a fresh mock for each test that needs one:

const onChange = jest.fn(); render( <PaletteEdit { ...defaultProps } onChange={ onChange } /> );

packages/components/src/palette-edit/test/index.tsx

mirka

Thank you for adding test coverage!

packages/components/src/palette-edit/test/index.tsx

mirka · 2024-01-10T15:30:25Z

packages/components/src/palette-edit/test/index.tsx

+			} )
+		);
+
+		await waitFor( () => {


To better mimic actual user behavior, it might be better to click the "Done" button rather than do a wait for the onChange.

packages/components/src/palette-edit/test/index.tsx

mirka · 2024-01-10T16:04:17Z

packages/components/src/palette-edit/test/index.tsx

+		} );
+	} );
+
+	it( 'can not add a new palette', () => {


Suggested change

it( 'can not add a new palette', () => {

it( 'can not add new colors when `canOnlyChangeValues` is enabled', () => {

I believe it's important to check both aspects — either in the same test, or in two separate tests. One checks that our public APIs work as expected, the other one tests that the component works as intended for the end user.

We could technically omit the onChange check if the component is tested in controlled mode, because if onChange didn't work as expected, the component wouldn't work for the end user either.

But I still prefer to test onChange (and the number of times it's been called) because in my experience it's also a great proxy for other undetected issues.

Agreed. Not a blocker for this particular PR though.

packages/components/src/palette-edit/test/index.tsx

mirka · 2024-01-10T16:09:08Z

packages/components/src/palette-edit/test/index.tsx

+				name: 'Show details',
+			} )
+		);
+		await user.click( screen.getByText( 'Primary' ) );


This is a great example of our tests doubling as accessibility tests. The fact that there's no labeled button to click means that this part of the UI is not accessible. Let's fix this in a follow-up.

packages/components/src/palette-edit/test/index.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

ciampo

Not much to add on top of Lena's comment, but still wanted to thank you for the great work so far!

mirka

🚀

t-hamano · 2024-01-12T03:17:30Z

@ciampo @mirka Thanks for the review! As I continue to work on #57309, I would like to add unit tests as needed.

brookewp · 2024-01-12T05:15:04Z

Thank for working on this! I just noticed in trunk the PaletteEdit tests are a bit flaky after this merge. It seems to be caused by these two tests:

shows an option to remove all colors and shows a reset option when the canReset

One option could be using waitFor():

diff --git a/packages/components/src/palette-edit/test/index.tsx b/packages/components/src/palette-edit/test/index.tsx
index 31b712225a..9c3cb14a6b 100644
--- a/packages/components/src/palette-edit/test/index.tsx
+++ b/packages/components/src/palette-edit/test/index.tsx
@@ -176,11 +176,13 @@ describe( 'PaletteEdit', () => {
 				name: 'Color options',
 			} )
 		);
-		expect(
-			screen.getByRole( 'button', {
-				name: 'Reset colors',
-			} )
-		).toBeVisible();
+		await waitFor( () => {
+			expect(
+				screen.getByRole( 'button', {
+					name: 'Reset colors',
+				} )
+			).toBeVisible();
+		} );
 	} );
 
 	it( 'does not show a reset colors option when `canReset` is disabled', async () => {
@@ -192,11 +194,13 @@ describe( 'PaletteEdit', () => {
 				name: 'Color options',
 			} )
 		);
-		expect(
-			screen.queryByRole( 'button', {
-				name: 'Reset colors',
-			} )
-		).not.toBeInTheDocument();
+		await waitFor( () => {
+			expect(
+				screen.queryByRole( 'button', {
+					name: 'Reset colors',
+				} )
+			).not.toBeInTheDocument();
+		} );
 	} );
 
 	it( 'calls the `onChange` with the new color appended', async () => {

What do you think? 🙂

t-hamano · 2024-01-12T05:35:01Z

I just noticed in trunk the PaletteEdit tests are a bit flaky after this merge. It seems to be caused by these two tests:

Thank you for noticing 🙇

One option could be using waitFor():

To me, this approach makes sense.

It appears that many of the tests using the toBeVisible assertion use waitFor
Especially for the dropdown, we may need to wait for the animation to finish (See this comment).

t-hamano · 2024-01-12T16:45:38Z

@ciampo In #57809, I discovered that the instability of unit tests including the PaletteEdit component has been resolved. Thank you for your response 🙇

PaletteEdit: improve unit tests

8b6608b

t-hamano added the [Package] Components /packages/components label Jan 8, 2024

t-hamano self-assigned this Jan 8, 2024

Update changelog

3cf2b5b

t-hamano commented Jan 8, 2024

View reviewed changes

t-hamano requested a review from a team January 8, 2024 14:00

t-hamano commented Jan 8, 2024

View reviewed changes

packages/components/src/palette-edit/test/index.tsx Show resolved Hide resolved

t-hamano added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 8, 2024

t-hamano marked this pull request as ready for review January 8, 2024 14:03

t-hamano requested a review from ajitbohra as a code owner January 8, 2024 14:03

mirka reviewed Jan 10, 2024

View reviewed changes

t-hamano and others added 10 commits January 11, 2024 11:03

Use toBeVisible instead of toBeInTheDocument

d3503bf

Co-authored-by: Lena Morita <lena@jaguchi.com>

Update test name

42e87f1

Co-authored-by: Lena Morita <lena@jaguchi.com>

Update test name

6a27ef2

Co-authored-by: Lena Morita <lena@jaguchi.com>

Update test name

e329ba2

Co-authored-by: Lena Morita <lena@jaguchi.com>

Update test name

7860258

Co-authored-by: Lena Morita <lena@jaguchi.com>

Update test name

6c885e0

Co-authored-by: Lena Morita <lena@jaguchi.com>

Remove Unnecessary uerr click event

791b2a3

Co-authored-by: Lena Morita <lena@jaguchi.com>

Use toBeVisible instead of toBeInTheDocument

a554bdc

use a fresh onChange mock for each test

cf13511

Fix conflicts

5a3ca5a

ciampo reviewed Jan 11, 2024

View reviewed changes

mirka approved these changes Jan 11, 2024

View reviewed changes

t-hamano merged commit d0287db into trunk Jan 12, 2024
56 checks passed

t-hamano deleted the quality/palette-edit-add-test branch January 12, 2024 03:19

github-actions bot added this to the Gutenberg 17.6 milestone Jan 12, 2024

mirka mentioned this pull request Jan 24, 2024

PaletteEdit: Fix palette item accessibility #58214

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PaletteEdit: improve unit tests #57645

PaletteEdit: improve unit tests #57645

t-hamano commented Jan 8, 2024

t-hamano Jan 8, 2024

mirka Jan 10, 2024

t-hamano Jan 11, 2024

mirka Jan 11, 2024

t-hamano Jan 8, 2024

t-hamano Jan 8, 2024

mirka Jan 10, 2024

mirka left a comment

mirka Jan 10, 2024

mirka Jan 10, 2024

ciampo Jan 11, 2024

mirka Jan 11, 2024

mirka Jan 10, 2024

ciampo left a comment

mirka left a comment

t-hamano commented Jan 12, 2024

brookewp commented Jan 12, 2024 •

edited

Loading

t-hamano commented Jan 12, 2024

t-hamano commented Jan 12, 2024

	it( 'can not add a new palette', () => {
	it( 'can not add new colors when `canOnlyChangeValues` is enabled', () => {

PaletteEdit: improve unit tests #57645

PaletteEdit: improve unit tests #57645

Conversation

t-hamano commented Jan 8, 2024

What?

Why?

How?

Testing Instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

mirka left a comment

Choose a reason for hiding this comment

t-hamano commented Jan 12, 2024

brookewp commented Jan 12, 2024 • edited Loading

t-hamano commented Jan 12, 2024

t-hamano commented Jan 12, 2024

brookewp commented Jan 12, 2024 •

edited

Loading