test: Improve unit test stability when running locally #10278
Conversation
…not configured to support act" errors
jdamore-linode
requested review from
carrillo-erik and
jaalah-akamai
and removed request for
a team
|
@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! |
There was a problem hiding this comment.
Resolves these warnings:
There was a problem hiding this comment.
Resolves:
We're calling userEvent.type and userEvent.click inside of act which triggers the warning since these functions call act themselves.
| it('should have an Add Client button', async () => { | ||
| const { findByText } = renderWithTheme(<LongviewLanding {...props} />); | ||
| const addButton = await findByText('Add Client'); | ||
| expect(addButton).toBeInTheDocument(); |
There was a problem hiding this comment.
This occasionally fails because queryByText('Add Client') returns null and the expect fails. I'm not sure why it fails to find the button (I don't see any logic that would delay the button being rendered), but using findByText allows the test to wait for it to appear.
There was a problem hiding this comment.
I spent the most time troubleshooting this test and found that it fails by timing out nearly every time when running alongside the rest of the test suite.
(The quickest way to reproduce I've found is to run yarn test src/features/PlacementGroups src/features/Profile, but this may vary from machine-to-machine).
What I've found is that the test, interactions, etc. all work exactly as expected, but when running alongside other tests some of the interactions (specifically and surprisingly the userEvent.click(submit)) take a really long time to execute. I don't think it has anything to do with the component itself.
My (purely speculative) guess is that when transpiling/running a lot of tests concurrently, the CPU is taking such a hit that it's slowing down JSDom to the point that the tests are timing out. I opened a ticket to reassess Happy DOM which might prove or disprove this theory, but in the meantime increasing the timeout seems to be a reliable (if unfortunate) mitigation.
|
Coverage Report: ❌ |
yarn.lock
Outdated
| @@ -13621,7 +13688,7 @@ vite@^5.0.0, vite@^5.0.12: | |||
| optionalDependencies: | |||
| fsevents "~2.3.3" | |||
|
|
|||
| vitest@^1.0.1, vitest@^1.2.0: | |||
| vitest@^1.0.1: | |||
There was a problem hiding this comment.
I wonder why vitest is showing up twice in our lockfile.
Do we need to bump the vitest version in api-v4 to resolve this?
There was a problem hiding this comment.
@bnussman-akamai That's exactly it, thanks for the catch!
|
I think I just got some flake on 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! |
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 Awesome work! This should help alleviate some of the issues and improve developer experience. |
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 🔄
CreateCertificateDrawer.test.tsx,CreateAPITokenDrawer.test.tsx, andImageAndPassword.test.tsxact()tests to resolve some failures and warningsact()inside of our ownact()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:
As an Author I have considered 🤔
Check all that apply