-
Notifications
You must be signed in to change notification settings - Fork 471
fix(waitFor): handle odd timing issue with fake timers #667
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
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 739afef:
|
Codecov Report
@@ Coverage Diff @@
## master #667 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 593 593
Branches 148 148
=========================================
Hits 593 593
Continue to review full report at Codecov.
|
🎉 This PR is included in version 7.17.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
@kentcdodds Do you remember which tests were impacting each other? It's totally fine if this only reproduces in a full test suite. But some repro would be nice since this change is quite odd considering we're scheduling more work from |
If this change is causing issues, then let's change it back. I'm pretty sure this was in the bookshelf app, but I don't have time to reproduce things and the latest version of react-query eliminates all the problems that I experienced with those tests anyway so we're probably ok to revert this. |
#1073 has a test that breaks with the behavior introduced in this PR (#667). I can fix the problematic behavior with a less invasive fix. But I can also just revert this change. I'm more comfortable with just reverting this change. In any case, I'll try both fixes in the bookshelf app so reduce likelyhood of unintended breakage. Thanks for the pointer! |
See #1073 (comment) for conclusion of the thread started with #667 (comment) |
What: fix(waitFor): handle odd timing issue with fake timers
Why: It's super hard to explain and I honestly don't really understand the issue completely myself. All I know is, before moving the
checkCallback
function call above the promise flush line, I had issues with tests impacting each other. After this change, that issue goes away.I also added a comment explaining why we shouldn't use
jest.advanceTimersToNextTimer
along with a test to demonstrate the issue.How: Change the order of operations
Checklist:
- [ ] Documentation added to the docs site N/A- [ ] Typescript definitions updated N/A