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

makitake2013 · 2020-05-30T13:09:27Z

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 · 2020-05-30T13:09:35Z

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 · 2020-05-30T13:19:51Z

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 · 2020-05-30T13:31:09Z

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 · 2020-05-30T13:32:13Z

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

Base commit: 46d6e7f

thymikee

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

rickhanlonii · 2020-06-08T16:30:23Z

Libraries/LogBox/Data/__tests__/LogBoxData-test.js

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

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


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

@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();

I'll defer that to @SimenB

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

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

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

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?

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

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

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

rickhanlonii

LGTM

facebook-github-bot

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

react-native-bot · 2020-07-12T07:30:05Z

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

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

rickhanlonii · 2020-07-13T02:14:04Z

Thanks @makitake2013!

makitake2013 · 2020-07-14T14:16:17Z

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

rickhanlonii · 2020-07-14T20:00:29Z

Please do!

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

3c30108

react-native-bot added the Bug label May 30, 2020

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

thymikee approved these changes Jun 2, 2020

View reviewed changes

thymikee requested a review from rickhanlonii June 2, 2020 19:29

rickhanlonii requested changes Jun 8, 2020

View reviewed changes

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

ab4a991

rickhanlonii approved these changes Jul 11, 2020

View reviewed changes

facebook-github-bot reviewed Jul 11, 2020

View reviewed changes

facebook-github-bot closed this in 320398a Jul 12, 2020

react-native-bot added the Merged This PR has been merged. label Jul 12, 2020

chrisbobbe mentioned this pull request Aug 25, 2020

Start using Jest's "modern" fake-timer implementation. #29767

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

makitake2013 commented May 30, 2020

facebook-github-bot commented May 30, 2020

facebook-github-bot commented May 30, 2020

analysis-bot commented May 30, 2020 •

edited

analysis-bot commented May 30, 2020 •

edited

thymikee left a comment

rickhanlonii Jun 8, 2020 •

edited

rickhanlonii Jun 8, 2020 •

edited

thymikee Jun 9, 2020

SimenB Jun 9, 2020

makitake2013 Jun 22, 2020

makitake2013 Jul 4, 2020

SimenB Jul 4, 2020

rickhanlonii Jul 4, 2020

rickhanlonii Jul 4, 2020

makitake2013 Jul 11, 2020 •

edited

rickhanlonii left a comment

facebook-github-bot left a comment

react-native-bot commented Jul 12, 2020

rickhanlonii commented Jul 13, 2020

makitake2013 commented Jul 14, 2020

rickhanlonii commented Jul 14, 2020

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

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

Conversation

makitake2013 commented May 30, 2020

Summary

Changelog

Test Plan

facebook-github-bot commented May 30, 2020

facebook-github-bot commented May 30, 2020

analysis-bot commented May 30, 2020 • edited

analysis-bot commented May 30, 2020 • edited

thymikee left a comment

Choose a reason for hiding this comment

rickhanlonii Jun 8, 2020 • edited

Choose a reason for hiding this comment

rickhanlonii Jun 8, 2020 • edited

Choose a reason for hiding this comment

thymikee Jun 9, 2020

Choose a reason for hiding this comment

SimenB Jun 9, 2020

Choose a reason for hiding this comment

makitake2013 Jun 22, 2020

Choose a reason for hiding this comment

makitake2013 Jul 4, 2020

Choose a reason for hiding this comment

SimenB Jul 4, 2020

Choose a reason for hiding this comment

rickhanlonii Jul 4, 2020

Choose a reason for hiding this comment

rickhanlonii Jul 4, 2020

Choose a reason for hiding this comment

makitake2013 Jul 11, 2020 • edited

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Jul 12, 2020

rickhanlonii commented Jul 13, 2020

makitake2013 commented Jul 14, 2020

rickhanlonii commented Jul 14, 2020

analysis-bot commented May 30, 2020 •

edited

analysis-bot commented May 30, 2020 •

edited

rickhanlonii Jun 8, 2020 •

edited

rickhanlonii Jun 8, 2020 •

edited

makitake2013 Jul 11, 2020 •

edited