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.

Sign up for GitHub

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

Jump to bottom

Improve flaky useQuery tests by not hard-coding request counts #10697

Merged
merged 2 commits into from
Mar 30, 2023
Merged

Improve flaky useQuery tests by not hard-coding request counts #10697

merged 2 commits into from
Mar 30, 2023

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Mar 30, 2023

I've noticed intermittent test failures on the main branch lately (for example, commit cdb98ae with output) that seem at least partially due to the testing anti-pattern of hard-coding request counts, which can change unexpectedly when the event loop is saturated during CI testing (so more 10ms polling intervals happen than expected).

@changeset-bot
Copy link

changeset-bot bot commented Mar 30, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: c50f44f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@benjamn
Make eslint happy.
c50f44f 
The linting rule "Avoid using multiple assertions within `waitFor`
callback testing-library/no-wait-for-multiple-assertions" seems to be
encouraging slightly worse code here, though I'm willing to play along.
Is there a good reason for that rule?
@netlify
Copy link

netlify bot commented Mar 30, 2023

✅ Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit fc49a85
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/6425cea65f18080008dae300
😎 Deploy Preview https://deploy-preview-10697--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@benjamn benjamn self-assigned this Mar 30, 2023
@alessbell
Copy link
Contributor

I was just looking at this file after the recent string of test flakes having discovered CCI's test insights 😅 Some heartening results here since the tests passed despite high CPU usage for most of the run:

CleanShot 2023-03-30 at 14 12 52

Re: testing-library/no-wait-for-multiple-assertions it's a pretty low-priority rule based on the fact that assertions inside a waitFor won't fail until after the timeout (default 1000ms) and not every assertion needs a waitFor.

I went with the defaults when adding eslint-plugin-testing-library, and many of the rules are helpful in avoiding genuine footguns, but I definitely don't have a strong opinion on disabling this rule 👍

alessbell
alessbell approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

A big win for test reliability 🎉

@benjamn benjamn merged commit fc2b6f4 into main Mar 30, 2023
@benjamn benjamn deleted the improve-flaky-requestSpy-tests branch March 30, 2023 18:42
lovasoa pushed a commit to lovasoa/apollo-client that referenced this pull request Mar 30, 2023
@benjamn @lovasoa
Improve flaky useQuery tests by not hard-coding request counts (apoll…
206e028 
…ographql#10697)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alessbell alessbell alessbell approved these changes

Assignees

@benjamn benjamn

Labels
🧞‍♂️ enhancement 🧩 implementation-detail 🔬 testing-utilities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@benjamn @alessbell