manager icon indicating copy to clipboard operation
manager copied to clipboard

test: Improve unit test stability when running locally

Open jdamore-linode opened this issue 1 year ago โ€ข 6 comments

Description ๐Ÿ“

When running tests locally on develop, I'm getting anywhere between 3-8 failures each run. We additionally had a flaky unit test in CI during our last release which required a few re-runs to unblock the fire chief. This PR aims to address these failures and clean up a few warning messages.

Many of the failures are mitigated by increasing test timeouts -- I investigated the failures and the corresponding components and found that the tests and components were behaving exactly as expected, but when running alongside the rest of the test suite, the tests' performance drops dramatically (e.g. from 1-2s to 7-8s, or more). Going to post some more details as comments to this PR.

Changes ๐Ÿ”„

  • Upgrade Vitest from 1.2.x to 1.3.x.
  • Increase test timeouts for flaky tests in CreateCertificateDrawer.test.tsx, CreateAPITokenDrawer.test.tsx, and ImageAndPassword.test.tsx
  • Remove calls to act() tests to resolve some failures and warnings
    • In most cases, we were using React Testing Library functions that call act() inside of our own act() statements which triggered the issues (userEvent.*, findBy*, etc.)

Target release date ๐Ÿ—“๏ธ

It'd be great to get this in by the next release just to reduce any risk of a unit test failure hindering the release process, but aside from that there's really no urgency here.

How to test ๐Ÿงช

This is tricky because most of these failures 1) only occur when running locally, 2) only occur when running the entire test suite (or some substantial portion of the test suite), and 3) seem to be performance related and therefore may vary from machine-to-machine.

The best way to confirm whether things are stable is to run the test suite locally:

yarn && yarn test

As an Author I have considered ๐Ÿค”

Check all that apply

  • [ ] ๐Ÿ‘€ Doing a self review
  • [ ] โ” Our contribution guidelines
  • [ ] ๐Ÿค Splitting feature into small PRs
  • [ ] โž• Adding a changeset
  • [ ] ๐Ÿงช Providing/Improving test coverage
  • [ ] ๐Ÿ” Removing all sensitive information from the code and PR description
  • [ ] ๐Ÿšฉ Using a feature flag to protect the release
  • [ ] ๐Ÿ‘ฃ Providing comprehensive reproduction steps
  • [ ] ๐Ÿ“‘ Providing or updating our documentation
  • [ ] ๐Ÿ•› Scheduling a pair reviewing session
  • [ ] ๐Ÿ“ฑ Providing mobile support
  • [ ] โ™ฟ Providing accessibility support

jdamore-linode avatar Mar 13 '24 16:03 jdamore-linode

@hana-linode I added you as a reviewer because you're the only developer I'm aware of who has reproduced these failures so it would be really helpful if you could test these changes, but there's absolutely no rush/if you're busy and you don't get a chance that's totally fine too. Thanks!

jdamore-linode avatar Mar 13 '24 16:03 jdamore-linode

Coverage Report: โŒ
Base Coverage: 82.3%
Current Coverage: 82.29%

github-actions[bot] avatar Mar 13 '24 16:03 github-actions[bot]

I think I just got some flake on src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerSummary/EditLoadBalancerConfigurations/EditRoutes/EditRouteDrawer.test.tsx. Maybe it needs some timeout ๐Ÿ˜ž

https://github.com/linode/manager/actions/runs/8282874790/job/22664807267

bnussman-akamai avatar Mar 14 '24 15:03 bnussman-akamai

I think I just got some flake on src/features/LoadBalancers/LoadBalancerCreate/LoadBalancerSummary/EditLoadBalancerConfigurations/EditRoutes/EditRouteDrawer.test.tsx. Maybe it needs some timeout ๐Ÿ˜ž

https://github.com/linode/manager/actions/runs/8282874790/job/22664807267

@bnussman-akamai I have seen this one too! I spent a little time troubleshooting it, but couldn't reproduce it consistently enough to make any progress. Iirc its failure message indicated something other than a timeout, like an event not firing or something like that, but there wasn't a ton to go on!

If I have some time before this gets more approvals I'll try to take a second look, but if not I'll open a new ticket to investigate further so this doesn't slip through the cracks!

jdamore-linode avatar Mar 14 '24 15:03 jdamore-linode

I'm still seeing some flaky tests but this is still an improvement ๐Ÿงน (Results screenshot)

Thanks @hana-linode, I'll check these out Monday morning -- the one test there timed out even though we increased the timeout to 10 seconds! Wonder if we need to increase it even more...

jdamore-linode avatar Mar 14 '24 21:03 jdamore-linode

@jdamore-linode Awesome work! This should help alleviate some of the issues and improve developer experience. Looks like you've got some merge conflicts to resolve.

carrillo-erik avatar Mar 20 '24 13:03 carrillo-erik