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.

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

Merged
merged 2 commits into from
Nov 2, 2018

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Oct 27, 2018

I'm currently working on replacing Jest's custom fake timers implementation with Lolex (jestjs/jest#5171). As part of that, I wanted to test that React's tests still work. They didn't...

A quirk of Jest's current implementation is that we schedule process.nextTick and setImmediate on the real timers, even though they're faked. Then we just make sure to do nothing if Jest invoked the function when advancing time, or vice versa if Node triggered it before us. That behavior is intentionally not re-implemented in the new version.

This PR changes from using setImmediate to Promise.resolve().then() to "wait 1 tick" which behaves the same.

global.Date.now = function() {
return originalDateNow();
};
// TODO remove `requestAnimationFrame` when upgrading to Jest 24 with Lolex
Copy link
Contributor Author

@SimenB SimenB Oct 27, 2018

Choose a reason for hiding this comment

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

Using Jest 24, the test passes without faking global.requestAnimationFrame. So whenever the upgrade lands, just remove the implementation here

@@ -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() {
Copy link
Contributor Author

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?

Copy link
Contributor

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:

container = document.createElement('div');
// TODO pull this into helper method, reduce repetition.
// mock the browser APIs which are used in schedule:
// - requestAnimationFrame should pass the DOMHighResTimeStamp argument
// - calling 'window.postMessage' should actually fire postmessage handlers
// - must allow artificially changing time returned by Date.now
// Performance.now is not supported in the test environment
const originalDateNow = Date.now;
let advancedTime = null;
global.Date.now = function() {
if (advancedTime) {
return originalDateNow() + advancedTime;
}
return originalDateNow();
};
advanceCurrentTime = function(amount) {
advancedTime = amount;
};
global.requestAnimationFrame = function(cb) {
return setTimeout(() => {
cb(Date.now());
});
};

I think this is safe to remove.

@sizebot
Copy link

sizebot commented Oct 27, 2018

Details of bundled changes.

Comparing: ae4f3f0...5029cba

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@@ -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(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The 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. .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do an await, then you're guaranteed a tick even if promises are faked

Copy link
Contributor

Choose a reason for hiding this comment

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

Naw, I think this is fine.

Copy link
Contributor

@nhunzaker nhunzaker left a comment

Choose a reason for hiding this comment

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

This looks good to me. What does the timing of this look like? Should we keep this open until the change in Jest publishes?

@SimenB
Copy link
Contributor Author

SimenB commented Nov 2, 2018

Seeing as the tests still pass, there's no reason not to merge, imo :)
Timeline is fuzzy, personally I hope for e.g. December 1st

@nhunzaker
Copy link
Contributor

Sure. Let's run with it. Thanks!

@nhunzaker nhunzaker merged commit 8eca0ef into facebook:master Nov 2, 2018
@SimenB SimenB deleted the lolex-compliance branch November 2, 2018 21:59
@acdlite
Copy link
Collaborator

acdlite commented Nov 4, 2018

@nhunzaker In the future, please at least tag the person who wrote the code that's being changed. Brian or me, in this case. Unless it's to unbreak master. Sometimes even small changes like this could have unforeseen consequences.

@nhunzaker
Copy link
Contributor

Sorry 😥. Sure thing!

@SimenB
Copy link
Contributor Author

SimenB commented Nov 4, 2018

Did it break anything, or is that just a precaution?

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…acebook#14003)

* chore: don't rely on jest fake timers scheduling real timers

* re-add one part not working with Jest 23
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
…acebook#14003)

* chore: don't rely on jest fake timers scheduling real timers

* re-add one part not working with Jest 23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants