-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
chore(tests): don't rely on jest fake timers scheduling real timers #14003
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,10 +18,7 @@ let ReactCache; | |
function initEnvForAsyncTesting() { | ||
// Boilerplate copied from ReactDOMRoot-test | ||
// TODO pull this into helper method, reduce repetition. | ||
const originalDateNow = Date.now; | ||
global.Date.now = function() { | ||
return originalDateNow(); | ||
}; | ||
// TODO remove `requestAnimationFrame` when upgrading to Jest 24 with Lolex | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using Jest 24, the test passes without faking |
||
global.requestAnimationFrame = function(cb) { | ||
return setTimeout(() => { | ||
cb(Date.now()); | ||
|
@@ -140,7 +137,7 @@ describe('ProfilerDOM', () => { | |
|
||
// Evaluate in an unwrapped callback, | ||
// Because trace/wrap won't decrement the count within the wrapped callback. | ||
setImmediate(() => { | ||
Promise.resolve().then(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I kind of wish this could be re-used, but it's unclear where something like that could go. . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could do an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naw, I think this is fine. |
||
expect(onInteractionTraced).toHaveBeenCalledTimes(1); | ||
expect( | ||
onInteractionScheduledWorkCompleted, | ||
|
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.
Lolex will fake this.
The test also passed when removing it when using Jest 23, not sure what it was supposed to do?
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.
This is curious! I'm not sure what this was for. I'm guessing that this was just an artifact copy/pasting a snippet from ReactDOMRoot-test. It's quite a bit more sophisticated:
react/packages/react-dom/src/__tests__/ReactDOMRoot-test.js
Lines 23 to 45 in c84b9bf
I think this is safe to remove.