Tabs: Fix flaky unit tests

[chad1008]

What?

Fixes flaky tests!

Why?

Because tests, unlike pastry, should not be flaky!

How?

The Ariakit internals go through a few different renders on selection changes. This can cause the test to run an assertion before the component is actually ready, especially in Controlled Mode.

this PR adds waitFor falls to our first assertions after Controlled mode selection changes in the newest unit tests. That ensures the component is done rendering before moving forward.

Testing Instructions

Just wait for that beautiful green checkmark.

[ciampo]

🚀

[ciampo]

Should we also add this same waiting time after the rerender calls?

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: chad1008 <shireling@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[chad1008]

Looking at @ciampo's comment above and testing further i'm still seeing some slight flakiness. Sometimes I have to run the test 200 times to get it to break, but eventually it does.

I remain stumped as to why this test remains flaky. It's specifically the should continue to handle arrow key navigation properly test that runs in the describe.each to check for both selectonOnMove values.

So far I've determined that:

1. while other tests benefit from a waitFor to ensure a specific element or attribute is rendered, that does not help in this case
2. This is the first test where we are asserting that the selected tab gets focus on the initial keypress in controlled mode, which is why this specific flakiness hasn't been an issue before
3. The test fails because focus goes to the first tab in the list (alpha in our tests). In the failure output I can see that alpha has the data-active-item attribute applied. However, if I monitor the renders in my actual browser and log out the first tab available, it never gets that attribute. In controlled mode the component renders knowing what tab to show right away.
4. I didn't see anything else in the test we could realistically apply a waitFor to and by some more time for the problematic assertion.
5. Looking at renders in the browser, the only difference I see in the initial load is that after the first couple of renders, the button gets type="button" added to it. Waiting for this does not solve the flakiness. That would have been a miserable solution, but it would have been something, which is more than I've got at that moment.

[ciampo]

cc @diegohaz in case anything comes to mind!

[diegohaz]

cc @diegohaz in case anything comes to mind!

I'd try to wait for something before press.Tab(). Unfortunately, anything here could potentially involve implementation details. If you can't identify anything that isn't tied to implementation details to wait for, I'd simply use the sleep utility from @ariakit/test:

await sleep();
await press.Tab();

In our internal tests, our render function is also asynchronous, so we don't have to grapple with this.

[chad1008]

Thank you @diegohaz! Catching up to this after being afk for a bit.

I just ran a quick test and adding a sleep() call before the press.Tab() and it got the test passing consistently for me locally (200 runs, no failures). I'm not sure if we have anything else that it makes sense to wait for as that Tab press is the first thing we need the test to do (and waiting for attributes/focus on the expected tab didn't do the trick) so a manual sleep() might be out best bet.

@mirka I see you self-assigned this one, I'm happy to keep working on it, but I haven't pushed any changes because I don't want to duplicate effort. Let me know if/how I can help!

[mirka]

@chad1008 Go ahead, I haven't done anything yet 👍 Let me know and I can take over at any point!

[tyxla]

Would be nice to finish and land this as some continuous flakiness has been reported. We can then closely monitor for further flakiness and react accordingly.

[chad1008]

Just pushed an update to this PR that implements a sleep() before the problematic assertion. I've also removed some of the previously added waitFor calls that didn't appear needed. This version passed my 200-run stress test nicely.

the one waitFor that I left in place is there because there were rare but sporadic "wrap state changes in act() warnings that we've seen before and solved the same way. If any more of those surface we can apply the same solution in those cases!

[github-actions]

Flaky tests detected in 907fa33.
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/7894449815
📝 Reported issues:

• [Flaky Test] list view should correctly reflect navigation items' structure #58980 in /test/e2e/specs/editor/blocks/navigation-list-view.spec.js

[tyxla]

LGTM 👍

Will reuse the same sleep() approach in #58968.

[chad1008]

Thank you @tyxla!