Skip to content

Conversation

@chad1008
Copy link
Contributor

@chad1008 chad1008 commented Nov 7, 2023

What?

Fix two flaky unit tests that were first mentioned in #55287 (comment)

Why?

Because no one likes a flaky test.

How?

Two tests had issues, and were solved by ensuring the component had time to fully update before proceeding.

Tabs › Uncontrolled mode › Disabled tab › should disable the tab when disabled is true

While debugging, I noticed that tabindex=-1 on inactive tests wasn't always being applied in time in the testing environment. This allowed the next tab in line to receive focus, triggering a failure. The new assertion waits for this property to be present before proceeding, ensuring an accurate test. I'd prefer we didn't need an assertion for this type of under-the-hood detail, but I'll take it over flakiness.

Tabs › Tab Activation › should not focus the next tab when the Tab key is pressed

This test relies on pressing the [Tab] key, followed by the [ArrowLeft] key, and then checking that the proper tab has focus. The focus check was happening too quickly sometimes, and new focus hadn't been applied yet. The new assertion waits for and checks the focused element after the [Tab] key, ensuring the new focus is applied before [LeftArrow] is pressed.

Between different testing runs I ran the full unit test suite about 100 times without any failures.

Testing Instructions

note: I didn't encounter the flakiness when running the tests individually, though I maybe just didn't try enough. I just ran the full suite instead and recommend testing this PR the same way.

  • After the test finishes, re-run with Enter/Return.
  • Repeat approximately 372 times. Or, you know... just a bunch.
  • Confirm that anti-flaky status has been achieved.

@chad1008 chad1008 self-assigned this Nov 7, 2023
@chad1008 chad1008 added the [Package] Components /packages/components label Nov 7, 2023
@chad1008 chad1008 requested a review from a team November 7, 2023 19:50
@chad1008 chad1008 added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended and removed [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Nov 7, 2023
@chad1008 chad1008 marked this pull request as ready for review November 7, 2023 19:53
@chad1008 chad1008 requested a review from ajitbohra as a code owner November 7, 2023 19:53
@ciampo
Copy link
Contributor

ciampo commented Nov 7, 2023

cc'ing @diegohaz in case he has anything to add —we have already experienced flakiness when testing ariakit components with @testing-library/user-event a few other times, and although I haven't testing this in person, this fix looks like a good approach.

I also wanted to flag that, during my work on DropdownMenu, I ended up using the @ariakit/test to simulate user interactions instead.

@chad1008 chad1008 changed the title fix flaky unit tests Tabs: Fix flaky unit. tests Nov 8, 2023
@chad1008 chad1008 changed the title Tabs: Fix flaky unit. tests Tabs: Fix flaky unit tests Nov 8, 2023
@chad1008
Copy link
Contributor Author

@ciampo did you think we should go ahead and merge this one now, and if @diegohaz has any recommendations later, we can address them in a followup as needed?

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this sounds like a good approach 🚢

(also, interesting to see that the CHANGELOG check is not running?)

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

FWIW, this has been what I've been suggesting as a solution for when we want to ensure the Ariakit store has finished initializing. Thanks for adding the comments to clarify what we're doing.

@chad1008
Copy link
Contributor Author

(also, interesting to see that the CHANGELOG check is not running?)

😆 That made me pause too, but then I remembered that we specifically excluded PRs that only change specific files like tests, since we don't usually add entries for those.

@chad1008 chad1008 merged commit 55285a8 into trunk Nov 17, 2023
@chad1008 chad1008 deleted the fix/tabs-flaky-unit-tests branch November 17, 2023 15:19
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants