Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Fixes flaky app layout tests that depend on timeout #2666

Closed
wants to merge 4 commits into from

Conversation

pan-kot
Copy link
Member

@pan-kot pan-kot commented Sep 5, 2024

Description

Use waitFor instead of delay for global-breadcrumbs.test.tsx tests.

See failing test: https://github.com/cloudscape-design/components/actions/runs/10718898212/job/29722075662

How has this been tested?

  • Confirmed flakiness with multiple test run
  • Verified not a single test in the fail fails after 1500 test runs
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pan-kot pan-kot requested a review from a team as a code owner September 5, 2024 12:04
@pan-kot pan-kot requested review from just-boris and removed request for a team September 5, 2024 12:04
Comment on lines 53 to 54
render(<BreadcrumbGroup items={defaultBreadcrumbs} />);
await waitFor(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This change breaks the intention of the tests. waitFor runs the first check without any delay and when it passes it moves forward.

The point of this test was to assert there is still only one breadcrumb instance in a long term, even after a timeout.

We can talk about individual flaky tests and how to make them more stable, but applying waitFor for every test in this file is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes total sense!

I added a delay before each wait for to that the first check is delayed as it was before the change.

Copy link
Member

Choose a reason for hiding this comment

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

Now the setup is even more far from original.

How about keep everything unchanged, and only add a couple waitFor to potentially flaky tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first one was different. This one is not. I am replacing this:

render()
delay()
assert()

with this:

render()
delay()
assertUntilItWorks()

The latter is equivalent of having a larger delay, but better efficient. I also run it 1500 times.

The change can be applied not to all tests but only to some. However, it looks like they all can fail. I reproduced failures in 2 or 3 different tests in that file.

Copy link
Member

Choose a reason for hiding this comment

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

I still do not understand what you disliked so much about renderAsync, so you decided to remove it.

I think the correct fix would be to improve renderAsync to make it more stable. It will happen here: #2676

I will close this PR as not needed. Thanks for taking your time

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.79%. Comparing base (09aa16f) to head (408ea63).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2666   +/-   ##
=======================================
  Coverage   95.79%   95.79%           
=======================================
  Files         743      744    +1     
  Lines       20528    20563   +35     
  Branches     6992     7008   +16     
=======================================
+ Hits        19664    19699   +35     
  Misses        856      856           
  Partials        8        8           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants