Skip to content

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

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

kentcdodds
Copy link
Member

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

  • Tests
    - [ ] Typescript definitions updated N/A
  • Ready to be merged

@codesandbox-ci
Copy link

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:

Sandbox Source
eloquent-thompson-fg3fr Configuration

@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #667 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #667   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          593       593           
  Branches       148       148           
=========================================
  Hits           593       593           
Impacted Files Coverage Δ
src/wait-for.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fe177f...739afef. Read the comment docs.

@kentcdodds kentcdodds merged commit 4ad3673 into master Jun 23, 2020
@kentcdodds kentcdodds deleted the pr/fix-wait-for-fake-timers branch June 23, 2020 21:19
@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 7.17.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@eps1lon
Copy link
Member

eps1lon commented Nov 10, 2021

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.

@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 waitFor after the waitFor promise is resolved.

@kentcdodds
Copy link
Member Author

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.

@eps1lon
Copy link
Member

eps1lon commented Nov 10, 2021

#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!

@eps1lon
Copy link
Member

eps1lon commented Nov 10, 2021

See #1073 (comment) for conclusion of the thread started with #667 (comment)

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

Successfully merging this pull request may close these issues.

2 participants