Button: Update test assertion to match test name

[brookewp]

What?

While working on #48222, I encountered a Button test where the assertion didn't match the test's intention based on the test name should populate tooltip with description content for buttons with visible labels (buttons with children)'.

Why?

As mentioned in the test name, the test is meant to check if the tooltip has been populated with the description content, but the assertion checks the button is visible instead. This will pass, even if the tooltip isn't there or it is populated with the description content. Therefore, if we intend to test the tooltip, we should query for that instead.

How?

By changing the role from button to tooltip. There are no actions taken to show the tooltip, so we also need to add that it is hidden and change the assertion to find by text content instead. I believe the description comes from the aria-label rather than aria-describedby, so we can't check for an accessible description.

Testing Instructions

Ensure tests pass: npm run test:unit packages/components/src/button/index.tsx

[tyxla]

I'd prefer to keep the existing assertion for the button, since it tests the existence of the actual button described by the visually hidden description text. Then I'd just add the assertion you created here for the tooltip, since that tests the hidden tooltip part.

In other words, this tests two different parts of what's rendered and to me it makes sense to test them both for a better test coverage.

[brookewp]

Fair point!

[ciampo]

Just flagging that, after #54312 will get merged, the tooltip won't be found in the DOM until visible (ie. triggered with hover/focus)

[tyxla]

Just flagging that, when #54312 merged, the tooltip won't be found in the DOM until visible (ie. triggered with hover/focus)

Good point @ciampo! I guess then we'll need to trigger a hover, and will need to add a waitFor() so the assertion is called again when the tooltip appears. Something like:

Suggested change

	expect(
	screen.getByRole( 'tooltip', {
	hidden: true,
	} )
	).toHaveTextContent( 'Description text' );
	const user = userEvent.setup();
	await user.hover(
	screen.getByRole( 'button', {
	description: 'Description text',
	} )
	);
	await waitFor( () => {
	expect(
	screen.getByRole( 'tooltip', {
	hidden: true,
	} )
	).toHaveTextContent( 'Description text' );
	} );

[brookewp]

The other tooltip tests used tab so I followed that, which lets us get around using waitFor():

gutenberg/packages/components/src/button/test/index.tsx

Lines 258 to 272 in ea90b50

	expect(
	screen.getByRole( 'button', {
	description: 'Description text',
	} )
	).toBeVisible();
	await user.tab();
	expect(
	screen.getByRole( 'tooltip', {
	name: 'Description text',
	} )
	).toBeVisible();
	await cleanupTooltip( user );

But if hover is preferable, we could use findByRole instead:

const button = screen.getByRole( 'button', {
	description: 'Description text',
} );

expect( button ).toBeVisible();

await user.hover( button );

expect(
	await screen.findByRole( 'tooltip', {
		name: 'Description text',
	} )
).toBeVisible();

Not sure if this info is still relevant, but it seems like find* is preferable over waitFor().

[ciampo]

I'm personally ok with both tab and hover, and I also prefer using findBy instead of waitFor.

I guess we can keep the code as-is and wrap up the PR :)

[tyxla]

I just quickly drafted this as an example, obviously, you can use whatever makes sense the most. In any case you can totally go with tab and findBy if you wish, I think this is a minor stylistic choice that wouldn't affect the quality of the test anyway.

[ciampo]

🚀

[tyxla]

Looking good, thanks @brookewp 🚀

[tyxla]

Restarted the performance tests, seems like a flaky run.