-
Notifications
You must be signed in to change notification settings - Fork 470
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
feat(waitFor): add complete and transparent support for fake timers #662
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 9aab321:
|
Elegant :) |
Build is busted thanks to: #663 I'm planning on doing a beta release of this locally so I can test it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm feeling pretty good about this, but I would still love someone else to give this a look.
describe('timers', () => { | ||
const expectElementToBeRemoved = async () => { | ||
const importedWaitForElementToBeRemoved = importModule() | ||
|
||
const {queryAllByTestId} = renderIntoDocument(` | ||
<div data-testid="div"></div> | ||
<div data-testid="div"></div> | ||
`) | ||
const divs = queryAllByTestId('div') | ||
// first mutation | ||
setTimeout(() => { | ||
divs.forEach(d => d.setAttribute('id', 'mutated')) | ||
}) | ||
// removal | ||
setTimeout(() => { | ||
divs.forEach(div => div.parentElement.removeChild(div)) | ||
}, 100) | ||
|
||
const promise = importedWaitForElementToBeRemoved( | ||
() => queryAllByTestId('div'), | ||
{ | ||
timeout: 200, | ||
}, | ||
) | ||
|
||
if (setTimeout._isMockFunction) { | ||
jest.advanceTimersByTime(110) | ||
} | ||
|
||
await promise | ||
} | ||
|
||
it('works with real timers', async () => { | ||
jest.useRealTimers() | ||
await expectElementToBeRemoved() | ||
}) | ||
it('works with fake timers', async () => { | ||
jest.useFakeTimers() | ||
await expectElementToBeRemoved() | ||
}) | ||
}) | ||
|
||
test("doesn't change jest's timers value when importing the module", () => { | ||
jest.useFakeTimers() | ||
importModule() | ||
|
||
expect(window.setTimeout._isMockFunction).toEqual(true) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these tests because waitForElementToBeRemoved
is built on top of waitFor
and it has its own tests.
@@ -117,4 +125,5 @@ export { | |||
setTimeoutFn as setTimeout, | |||
runWithRealTimers, | |||
checkContainerType, | |||
jestFakeTimersAreEnabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much all the changes in this file are refactors to facilitate this export.
This is published as |
And I published these changes in |
Works like a charm! :chefs-kiss: |
Dang it.
🤦♂️ investigating.... |
src/wait-for.js
Outdated
// waiting or when we've timed out. | ||
// eslint-disable-next-line no-unmodified-loop-condition | ||
while (!finished) { | ||
jest.runAllTimers() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! This was supposed to be jest.advanceTimersByTime(interval)
😅
229dea2
to
93977ad
Compare
I think this is ready. I made a handful of changes to how we test some of these utilities (especially the deprecated ones). |
93977ad
to
abbfdbd
Compare
Codecov Report
@@ Coverage Diff @@
## master #662 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 583 593 +10
Branches 149 148 -1
=========================================
+ Hits 583 593 +10
Continue to review full report at Codecov.
|
abbfdbd
to
b32e2fd
Compare
b32e2fd
to
9aab321
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, this is ready to go as soon as the build passes.
const {MutationObserver} = getWindowFromNode(container) | ||
const observer = new MutationObserver(checkCallback) | ||
runWithRealTimers(() => | ||
observer.observe(container, mutationObserverOptions), | ||
) | ||
checkCallback() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to run this with real timers because we only run it when we're not using fake timers.
Note: I can't guarantee that these changes won't break people who had to work around the limitations in their tests. So if someone was using our async utilities with fake timers, it's possible that we'll break them depending on how they worked around things. I'll add this to the release notes, but if you're here reading this because your tests broke as a result of this change, just know that you shouldn't have to worry about advancing the fake timers manually anymore. |
🎉 This PR is included in version 7.17.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
What: feat(waitFor): add complete and transparent support for fake timers
Why: Closes #661
How:
Basically, we detect if we're in an environment that's faking out timers and if we are then we do a
while
loop that advances the jest fake timers by the interval until the callback passes. We do still support a timeout so it can time out eventually and won't loop forever.Checklist:
- [ ] Documentation added to the docs site N/A- [ ] Typescript definitions updated N/A