`ColorPalette`: refine test query #53704

brookewp · 2023-08-15T22:58:53Z

What?

Updates the query for testing the color display name in ColorPalette's test.

Why?

This test fails due to the upcoming changes to tooltip (#48222). In the new version, tooltip can be found in the dom even while hidden. Therefore, the getByText query is returning multiple elements, including tooltip. If we change the query to screen.getByRole( 'button', { name: /red/i } ), it matches more than one button.

How?

Until the changes are merged from #52255 (@andrewhayward), I could not find a better way to avoid the above issues other than to specify the span selector. This will fail in the aforementioned PR, but that will be a good reminder to update to getByRole( 'option' ) instead 😄 (or we can add it as a to-do item).

Testing Instructions

Verify tests still pass npm run test:unit packages/components/src/color-palette/test/index.tsx

tyxla

Using selector is a good idea IMHO 👍 We don't have many other options, given the markup of the color palette color components.

I've left a few improvement suggestions, let me know what you think.

tyxla · 2023-08-16T12:08:57Z

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

-		expect( screen.getByText( EXAMPLE_COLORS[ 0 ].name ) ).toBeVisible();
-		expect( screen.getByText( EXAMPLE_COLORS[ 0 ].color ) ).toBeVisible();
+		expect(
+			screen.getByText( colorName, { selector: 'span' } )


If you want to get a bit more specific, you can customize the selector by attribute:

Suggested change

screen.getByText( colorName, { selector: 'span' } )

screen.getByText( colorName, { selector: 'span[data-wp-component="Truncate"]' } )

or by classname:

Suggested change

screen.getByText( colorName, { selector: 'span' } )

screen.getByText( colorName, {

selector: '.components-color-palette__custom-color-name',

} )

or you can combine classname with tagname:

Suggested change

screen.getByText( colorName, { selector: 'span' } )

screen.getByText( colorName, {

selector: 'span.components-color-palette__custom-color-name',

} )

If we use your first suggestion, then we could put it into a variable as well:

const truncateComponent = { selector: 'span[data-wp-component="Truncate"]', };

But this made me wonder...

I am not 100% sure if the Truncate component will be kept in the upcoming ColorPalette changes. But if the parent is updated from a button to options, we can use a getByRole query, as I mentioned earlier.

This made me think about tests in general: Is it best to specify the component so it would error if it is changed (so we remember to update to a better query), or is it better to use the class since it's more likely to stay the same and therefore, pass?

I realize this is an implementation detail since it doesn't matter what we use as long as it ensures the behavior works for the user. I was just curious if you had an opinion on this. 🙂

If we use your first suggestion, then we could put it into a variable as well

We could if we need it multiple times, yes 👍

I am not 100% sure if the Truncate component will be kept in the upcoming ColorPalette changes. But if the parent is updated from a button to options, we can use a getByRole query, as I mentioned earlier.

getByRole is always preferred 👍

This made me think about tests in general: Is it best to specify the component so it would error if it is changed (so we remember to update to a better query), or is it better to use the class since it's more likely to stay the same and therefore, pass? I realize this is an implementation detail since it doesn't matter what we use as long as it ensures the behavior works for the user. I was just curious if you had an opinion on this.

It really depends. In this case, we could argue that both the classname that I'm suggesting and the span that you are suggesting are implementation details. I guess in this case it's about what's the ideal implementation detail to include in the test. I believe the classname one is better, because there might be multiple span with the queried text and it would break the test; while it's a smaller chance to have multiple elements with the same text and the same classname.

Anyway, I wanted to offer a few alternatives that seemed a bit better than just specifying a tagname, but feel free to ignore them, especially if we can change the query to be by role.

Your suggestions are definitely better than just specifying a tag name, so thank you. And this answered my question!

I believe the classname one is better, because there might be multiple span with the queried text and it would break the test; while it's a smaller chance to have multiple elements with the same text and the same classname.

tyxla · 2023-08-16T12:09:57Z

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

+		expect(
+			screen.getByText( colorName, { selector: 'span' } )
+		).toBeVisible();
+		expect( screen.getByText( colorCode ) ).toBeVisible();


This could be further clarified, similar to how we did for the color name above. For example, you could do:

Suggested change

expect( screen.getByText( colorCode ) ).toBeVisible();

expect(

screen.getByText( colorCode, {

selector: '.components-color-palette__custom-color-value',

} )

).toBeVisible();

tyxla · 2023-08-16T12:10:59Z

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

 		expect(
 			screen.getByRole( 'button', {
-				name: `Custom color picker. The currently selected color is called "${ EXAMPLE_COLORS[ 0 ].name }" and has a value of "${ EXAMPLE_COLORS[ 0 ].color }".`,
+				name: `Custom color picker. The currently selected color is called "${ colorName }" and has a value of "${ EXAMPLE_COLORS[ 0 ].color }".`,


Maybe we could rename the color value too:

Suggested change

name: `Custom color picker. The currently selected color is called "${ colorName }" and has a value of "${ EXAMPLE_COLORS[ 0 ].color }".`,

name: `Custom color picker. The currently selected color is called "${ colorName }" and has a value of "${ colorCode }".`,

Good catch! Missed that one 😅

tyxla · 2023-08-16T12:12:10Z

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

+		const colorName = EXAMPLE_COLORS[ 0 ].name;
+		const colorCode = EXAMPLE_COLORS[ 0 ].color;


Could be simplified by using destructuring with aliases:

Suggested change

const colorName = EXAMPLE_COLORS[ 0 ].name;

const colorCode = EXAMPLE_COLORS[ 0 ].color;

const { name: colorName, color: colorCode } = EXAMPLE_COLORS[ 0 ];

Great suggestion!

tyxla · 2023-08-16T12:13:24Z

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

-		).not.toBeInTheDocument();
-		expect(
-			screen.queryByText( EXAMPLE_COLORS[ 0 ].color )
+			screen.queryByText( colorName, { selector: 'span' } )


This selector could be made further specific like we did for the color code and color name queries above, but for non-existence queries it should be fine as-is.

I missed this earlier, but I came across it when I updated the selector to class names, so this should be set with the latest commit. 😄

tyxla

LGTM 👍 Thanks!

I have my own opinion about what implementation detail selector to use, but it's not a requirement or anything. As you said, ideally we can use a role query later.

Anyway, feel free to ship as-is! 🚀

tyxla · 2023-08-17T09:36:03Z

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

@@ -222,9 +222,15 @@ describe( 'ColorPalette', () => {

 	it( 'should display the selected color name and value', async () => {
 		const user = userEvent.setup();
+		// selector for Truncate component to make test queries more explicit
+		const truncateComponent = {
+			selector: 'span[data-wp-component="Truncate"]',


From the three options I suggested above, this is the least preferred one. If I had to pick, I'd pick the span.classname or .classname one. But leaving that to your judgment.

I initially thought it would be good if it errored so we'd update the selector if the opportunity arose, but after reading your reply to my question on which implementation detail is best, it makes sense to change it! With both of the updates in the last commit, I think it is also cleaner and easier to read.

Thanks, the changes look awesome 🙌 🚀

tyxla · 2023-08-17T18:00:49Z

The failing unit test isn't related - seems like it's been flaky all day for some other branches too. Retriggered it, hope it becomes green this time.

github-actions · 2023-08-17T21:15:41Z

Flaky tests detected in 092d4cced8394f5f200644d9b323c39ad62c963d.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5894170920
📝 Reported issues:

[Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

…eries

brookewp · 2023-08-18T02:23:02Z

The failing unit test isn't related - seems like it's been flaky all day for some other branches too. Retriggered it, hope it becomes green this time.

Looks like it's still failing 😞

tyxla · 2023-08-18T09:10:38Z

The failing unit test isn't related - seems like it's been flaky all day for some other branches too. Retriggered it, hope it becomes green this time.

Looks like it's still failing 😞

The remaining failing e2e test is consistently failing on trunk as well. Feel free to ship at will!

brookewp · 2023-08-18T19:46:43Z

The remaining failing e2e test is consistently failing on trunk as well. Feel free to ship at will!

I only have the option to enable auto-merge - is that what you mean? Or perhaps it's possible to merge now but I don't have those permissions?

mirka · 2023-08-18T22:52:34Z

The remaining failing e2e test is consistently failing on trunk as well. Feel free to ship at will!

I only have the option to enable auto-merge - is that what you mean? Or perhaps it's possible to merge now but I don't have those permissions?

Correct, only Core team members have rights to bypass the merge protections. I'll go ahead and merge this then 👍

brookewp added the [Package] Components /packages/components label Aug 15, 2023

brookewp requested a review from ajitbohra as a code owner August 15, 2023 22:58

brookewp requested review from mirka, tyxla and flootr August 15, 2023 22:59

brookewp added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Aug 15, 2023

tyxla reviewed Aug 16, 2023

View reviewed changes

brookewp requested a review from tyxla August 17, 2023 04:36

tyxla approved these changes Aug 17, 2023

View reviewed changes

brookewp force-pushed the update/colorpalette-test branch 2 times, most recently from e46f69f to 092d4cc Compare August 17, 2023 17:42

brookewp mentioned this pull request Aug 18, 2023

ToolTip: refactor using ariakit #48440

Merged

brookewp added 3 commits August 17, 2023 18:50

ColorPalette: refine test query

df7886b

Improve variables based on review feedback

e19a9b5

Update selectors to classes and remove selectors from non-existent qu…

2f3b2dd

…eries

brookewp force-pushed the update/colorpalette-test branch from 092d4cc to 2f3b2dd Compare August 18, 2023 01:50

brookewp enabled auto-merge (squash) August 18, 2023 19:45

brookewp disabled auto-merge August 18, 2023 19:45

mirka merged commit 1d9753e into trunk Aug 18, 2023
48 of 49 checks passed

mirka deleted the update/colorpalette-test branch August 18, 2023 22:52

github-actions bot added this to the Gutenberg 16.6 milestone Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`ColorPalette`: refine test query #53704

`ColorPalette`: refine test query #53704

brookewp commented Aug 15, 2023

tyxla left a comment

tyxla Aug 16, 2023

brookewp Aug 16, 2023

tyxla Aug 17, 2023

brookewp Aug 17, 2023

tyxla Aug 16, 2023

tyxla Aug 16, 2023

brookewp Aug 16, 2023

tyxla Aug 16, 2023

brookewp Aug 16, 2023

tyxla Aug 16, 2023

brookewp Aug 17, 2023

tyxla left a comment

tyxla Aug 17, 2023

brookewp Aug 17, 2023

tyxla Aug 17, 2023

tyxla commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

brookewp commented Aug 18, 2023

tyxla commented Aug 18, 2023

brookewp commented Aug 18, 2023

mirka commented Aug 18, 2023

	screen.getByText( colorName, { selector: 'span' } )
	screen.getByText( colorName, { selector: 'span[data-wp-component="Truncate"]' } )

	name: `Custom color picker. The currently selected color is called "${ colorName }" and has a value of "${ EXAMPLE_COLORS[ 0 ].color }".`,
	name: `Custom color picker. The currently selected color is called "${ colorName }" and has a value of "${ colorCode }".`,

		const colorName = EXAMPLE_COLORS[ 0 ].name;
		const colorCode = EXAMPLE_COLORS[ 0 ].color;

	const colorName = EXAMPLE_COLORS[ 0 ].name;
	const colorCode = EXAMPLE_COLORS[ 0 ].color;
	const { name: colorName, color: colorCode } = EXAMPLE_COLORS[ 0 ];

ColorPalette: refine test query #53704

ColorPalette: refine test query #53704

Conversation

brookewp commented Aug 15, 2023

What?

Why?

How?

Testing Instructions

tyxla 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

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

tyxla 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

tyxla commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

brookewp commented Aug 18, 2023

tyxla commented Aug 18, 2023

brookewp commented Aug 18, 2023

mirka commented Aug 18, 2023

`ColorPalette`: refine test query #53704

`ColorPalette`: refine test query #53704