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

Test fixes related to changes in Jest's Fake Timer behavior #29011

Closed
wants to merge 2 commits into from

Conversation

makitake2013
Copy link
Contributor

Summary

Fixed tests related to changing Jest's fake timer behavior.
And some of the test code in LogBoxData-test.js itself was incorrect so I am fixing it.

LogBoxData-test.js fails when I try to build a new local development environment and fork the React Native source to run a JavaScript test.
The error message is:
runAllImmediates is not available when using modern timers

After checking, runAllImmediates could not be used with the following modification.
jestjs/jest@71631f6

Changelog

[Internal] [Fixed] - Test fixes related to changes in Jest's Fake Timer behavior

Test Plan

Ran JavaScript Tests locally.

@facebook-github-bot
Copy link
Contributor

Hi @makitake2013!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 30, 2020
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@analysis-bot
Copy link

analysis-bot commented May 30, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,781,848 20,680
android hermes armeabi-v7a 6,446,039 21,913
android hermes x86 7,168,925 20,111
android hermes x86_64 7,060,257 21,530
android jsc arm64-v8a 8,949,753 16,902
android jsc armeabi-v7a 8,606,340 18,162
android jsc x86 8,779,964 16,355
android jsc x86_64 9,356,913 17,767

Base commit: 46d6e7f

@analysis-bot
Copy link

analysis-bot commented May 30, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 10728720 n/a

Base commit: 46d6e7f

Copy link
Contributor

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Modern FakeTimers is not yet turned on by default, but I'm not against this change.

@@ -311,7 +307,6 @@ describe('LogBoxData', () => {
addFatalErrors(['C']);

// Order maters for timeout before symbolication.
jest.runAllTimers();
flushLogs();
Copy link
Member

@rickhanlonii rickhanlonii Jun 8, 2020

Choose a reason for hiding this comment

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

The code here is:

// Order maters for timeout before symbolication.
jest.runAllTimers();	
flushLogs();

The first line jest.runAllTimers() runs the first timer to expiration. This should trigger a log to get scheduled for addition, which schedules a new timer to update the logs data. This requires another jest.runAllTimers() to run the new timers created when the old timers ran. This is done with flushLogs().

This is basically the same principle for all the changes here. I'm not sure what changed in Jest, but this test should fail with the changes here (so either the tests are wrong, or Jest is wrong).

Copy link
Member

@rickhanlonii rickhanlonii Jun 8, 2020

Choose a reason for hiding this comment

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

@thymikee in the new timer system, do we run all timers including timers that are created when a timer is run?

E.g. what should this print?

setTimeout(() => {
  console.log(1);
  setTimeout(() => {
      console.log(2);
  }, 0);
}, 0);

jest.runAllTimers();

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer that to @SimenB

Copy link
Contributor

Choose a reason for hiding this comment

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

That should log 1 then 2, for both legacy and modern timers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickhanlonii @thymikee
Thank you for your reviewing.
I confirmed the behavior of the timer system,
2 was output by runAllTimers run once.

スクリーンショット 2020-06-22 18 28 23

スクリーンショット 2020-06-22 18 37 54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickhanlonii
Thank you for your reply.
I also have some concerns.
Is this test still working properly in your development environment?

When I forked the latest version of the React Native source and run jest, only this test case failed.
I wonder if existing developers or the CI environment have old version of Jest in , but is it also my environment configuration issue?
If possible, please reinstall node_modules and give it a try.

I think it should not be fixed it if it's a configuration issue, and if this problem will be occurred when fork the latest version and create a new development environment, it should be fixed, thought.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are behavioral changes between "modern" and "legacy" timers, mostly laid out in the OP here: jestjs/jest#5171.

I don't really follow this discussion - could any of you put together a snippet that changes behavior with modern timers in a breaking way you think is wrong?

Copy link
Member

Choose a reason for hiding this comment

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

@SimenB sorry for the confusion, the snippets concerned is:

setTimeout(() => {
  console.log(1);
  setTimeout(() => {
      console.log(2);
  }, 0);
}, 0);

After reading up, it sounds like we changed it so that jest.runAllTimers runs any scheduled timers as well. That seems to make sense, when you runAllTimers you want to flush all of them (even the new ones scheduled).

For these test though, we need a way to run only the pending timers, so we can advance a nested timer as in:

// Schedule a nested timer.
jest.runOnlyPendingTimers();

// Advance the newly scheduled timer.
jest.advanceTimersByTime(1001); 

// Flush remaining timers.
jest.runAllTimers(); 

So my new concern is from what you linked:

[BREAKING] runAllPendingTimers in Jest currently do not trigger new timers scheduled while running to the last scheduled one. Lolex does (which matches what would happen in real code).

Did you mean to say runAllTimers here? If we changed runAllPendingTimers then the docs say the opposite of what it does and I don't think I can do the above.

Copy link
Member

Choose a reason for hiding this comment

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

@makitake2013 I pushed a branch with how I recommend that we fix this here.

Do you want to copy these changes over and test?

To fix, I updated the test so that:

  • The flushes mostly use runOnlyPendingTimers
  • Remove runAllImmediates
  • Remove dependencies on runAllTimers behavior with scheduled timers
  • Added flushToObservers for when we were calling flushLogs just to update observers
  • Updated so the right number of flushes are done automatically for each log (e.g. errors need 2)
  • Updated the optimistic symbolication tests to work with Jest 26 timers

This should work with the Jest 26 timers but I didn't test myself.

Copy link
Contributor Author

@makitake2013 makitake2013 Jul 11, 2020

Choose a reason for hiding this comment

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

@rickhanlonii
I confirmed that the test you modified works on Jest 26 and updated.
I understand what you were concerned about.
Thank you so much for your reviewing.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@rickhanlonii has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @makitake2013 in 320398a.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 12, 2020
@rickhanlonii
Copy link
Member

Thanks @makitake2013!

@makitake2013
Copy link
Contributor Author

Thanks @rickhanlonii !
This was my first pull request. I learned a lot and it was fun, so I would like to try again!

@rickhanlonii
Copy link
Member

Please do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants