fix: fake all supported timers by default #323

SimenB · 2020-05-11T13:56:22Z

Purpose (TL;DR) - mandatory

It's very weird to me that this method is excluded from default mocking. I've read #126 and to me the solution seems to be for users to specify a toFake themselves, not force other people to specify "yes, when I say fake timers I do mean all timers".

Note that this should be considered a breaking change.

Whenever we start supporting some new timer, it should by default not be mocked, but I think when we later make a new major release it should be mocked by default again.

SimenB · 2020-05-11T13:56:41Z

src/fake-timers-src.js

@@ -1262,10 +1262,7 @@ function withGlobal(_global) {
        clock.methods = config.toFake || [];

        if (clock.methods.length === 0) {
-            // do not fake nextTick by default - GitHub#126
-            clock.methods = keys(timers).filter(function(key) {
-                return key !== "nextTick" && key !== "queueMicrotask";


seems docs were lying 🤷 queueMicrotask was also not mocked by default

codecov · 2020-05-11T13:57:58Z

Codecov Report

Merging #323 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
- Coverage   92.75%   92.71%   -0.04%     
==========================================
  Files           1        1              
  Lines         552      549       -3     
==========================================
- Hits          512      509       -3     
  Misses         40       40

Flag	Coverage Δ
#unit	`92.71% <100.00%> (-0.04%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/fake-timers-src.js	`92.71% <100.00%> (-0.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f8322e...21d726f. Read the comment docs.

SimenB · 2020-05-11T14:01:10Z

src/fake-timers-src.js

-                return key !== "nextTick" && key !== "queueMicrotask";
-            });
-        }
+        clock.methods = config.toFake || keys(timers);


another technically breaking change I guess - if people passed toFake: [] before, we faked everything. Seems like a bug fix to me to respect it, but maybe not 😀 If not, should just revert the last commit

Yeah I think this and the other one should be a major version if we land them.

Can open a separate PR for it?

Can open a separate PR for it?

Yes

benjamingr · 2020-05-11T14:01:42Z

So here's the thing: promise libraries and other libraries use nextTick and microticks for scheduling so this is a pretty big breaking change.

In principle I agree that we should mock this by default.

benjamingr · 2020-05-11T14:02:16Z

test/fake-timers-test.js

@@ -4504,19 +4504,6 @@ describe("FakeTimers", function() {
            assert(called);
            clock.uninstall();
        });
-
-        it("does not install by default - GitHub#126", function(done) {


Would it be possible to change this test to assert nextTick is mocked if the behavior changes?

changed the test above, I think that's fine?

changed the test above, I think that's fine?

Yes, this is fine. It's implicitly true, as the code won't work unless nextTick is synchronous.

SimenB · 2020-05-11T14:04:12Z

So here's the thing: promise libraries and other libraries use nextTick and microticks for scheduling so this is a pretty big breaking change.

Good point, should probably call it out in the readme.

In principle I agree that we should mock this by default.

👍

SimenB · 2020-05-11T14:12:06Z

@benjamingr added a section to the readme, feedback appreciated 😀

benjamingr · 2020-05-11T14:13:41Z

@SimenB I am +1 on this change in principle but I know it will break pretty much every fake-timers install "in the wild" that uses the current API.

SimenB · 2020-05-11T14:19:36Z

Should work if people use the builtin Promise and async-await, no? If it breaks builtin Promise global I'd consider that a bug in node.

My main issue is that to enable "fake everything" I have to enumerate all timers from the outside, which is not obvious I think. Maybe we could rather add a fakeNextTick explicit option (defaulting to false I guess) instead of piggy-backing toFake?

benjamingr · 2020-05-11T14:23:07Z

Should work if people use the builtin Promise and async-await, no? If it breaks builtin Promise global I'd consider that a bug in node.

It doesn't break the built in promises but mostly because we don't mock its timings at the moment (we probably should with async_hooks). In practice this means that it's a lot harder than it has to to test promises with fake-timers (I recall a lot of discussions about this + async versions of timers somewhere?)

I don't mind seeing what breaks with mocking nextTick on some large codebases I maintain - though I assume I know the answer is "every place that relies on nextTick executing and doesn't .tick or runMicrotasks before that :]

SimenB · 2020-05-11T14:25:52Z

As a data point, in a work project where I migrated to use Jest's fake timers now that Jest ships with Lolex it broke node-fetch (and/or integration with nock, not entirely sure, I just added a runMicrotasks call)

stale · 2020-07-18T06:00:06Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mroderick · 2020-07-27T14:09:17Z

Stale bot?!

fatso83 · 2020-08-13T11:23:52Z

This makes my head hurt and you guys know best anyhow. Not sure why it has stalled, though?

fatso83

To me, this looks good. @SimenB anything you want addressed (apart from the conflicts)?

fatso83 · 2021-05-28T05:13:27Z

B,B,B,B,bump? It's not super problematic to release a major version, as long as you also document how to deal with the fallout. Like using node-fetch as an example case, show what needs to be done to fix the test cases. @SimenB

fatso83 · 2021-06-09T22:19:20Z

src/fake-timers-src.js

-                return key !== "nextTick" && key !== "queueMicrotask";
-            });
-        }
+        clock.methods = config.toFake || keys(timers);


Can open a separate PR for it?

Yes

fatso83 · 2021-06-09T22:21:02Z

test/fake-timers-test.js

@@ -4504,19 +4504,6 @@ describe("FakeTimers", function() {
            assert(called);
            clock.uninstall();
        });
-
-        it("does not install by default - GitHub#126", function(done) {


changed the test above, I think that's fine?

Yes, this is fine. It's implicitly true, as the code won't work unless nextTick is synchronous.

mroderick · 2021-07-13T07:28:58Z

This needs a rebase to resolve the conflicts.

@SimenB do you have time to take this across the finish line? If not, would you be ok with someone else doing it?

fatso83 · 2024-02-10T11:46:47Z

I want to finish this (and do #490) and release a new breaking major. Soonish

I don't mind seeing what breaks with mocking nextTick on some large codebases I maintain

There's been 4 years. I guess the number of places that rely on non-native Promises are a lot fewer and further between. Maybe it's not that bad? I think we would just need to add something to the changelog.

fatso83 · 2024-03-22T11:34:44Z

Since I closed #490 and I haven't heard anything in months, I think I'll just pick this up and release a new breaking major.

fix: fake all supported timers by default

a8a8f7b

SimenB requested review from mroderick, fatso83 and benjamingr May 11, 2020 13:56

SimenB commented May 11, 2020

View reviewed changes

simplify

108aeb9

SimenB commented May 11, 2020

View reviewed changes

benjamingr reviewed May 11, 2020

View reviewed changes

SimenB added 3 commits May 11, 2020 16:06

re-add test

33398de

re-write test

8d021c8

call out promises

21d726f

mroderick added the major/breaking label May 15, 2020

chrisbobbe mentioned this pull request Jul 7, 2020

"Modern" fake-timer implementation doesn't work with PromiseJS. jestjs/jest#10221

Closed

stale bot added the stale label Jul 18, 2020

mroderick added pinned and removed stale labels Jul 21, 2020

stale bot closed this Jul 25, 2020

mroderick reopened this Jul 27, 2020

fatso83 approved these changes Feb 16, 2021

View reviewed changes

fatso83 added the awaiting response label May 28, 2021

fatso83 approved these changes Jun 9, 2021

View reviewed changes

fatso83 mentioned this pull request Feb 10, 2024

install({ toFake: ["setImmediate"] }) fails when setImmediate is not available in context (such as browser environment) #490

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fake all supported timers by default #323

fix: fake all supported timers by default #323

SimenB commented May 11, 2020

SimenB May 11, 2020 •

edited

codecov bot commented May 11, 2020 •

edited

SimenB May 11, 2020

benjamingr May 11, 2020

SimenB May 11, 2020

fatso83 Jun 9, 2021

benjamingr commented May 11, 2020

benjamingr May 11, 2020

SimenB May 11, 2020

fatso83 Jun 9, 2021

SimenB commented May 11, 2020

SimenB commented May 11, 2020

benjamingr commented May 11, 2020

SimenB commented May 11, 2020

benjamingr commented May 11, 2020

SimenB commented May 11, 2020 •

edited

stale bot commented Jul 18, 2020

mroderick commented Jul 27, 2020

fatso83 commented Aug 13, 2020

fatso83 left a comment

fatso83 commented May 28, 2021

fatso83 Jun 9, 2021

fatso83 Jun 9, 2021

mroderick commented Jul 13, 2021

fatso83 commented Feb 10, 2024

fatso83 commented Mar 22, 2024

fix: fake all supported timers by default #323

Are you sure you want to change the base?

fix: fake all supported timers by default #323

Conversation

SimenB commented May 11, 2020

Purpose (TL;DR) - mandatory

SimenB May 11, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented May 11, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamingr commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented May 11, 2020

SimenB commented May 11, 2020

benjamingr commented May 11, 2020

SimenB commented May 11, 2020

benjamingr commented May 11, 2020

SimenB commented May 11, 2020 • edited

stale bot commented Jul 18, 2020

mroderick commented Jul 27, 2020

fatso83 commented Aug 13, 2020

fatso83 left a comment

Choose a reason for hiding this comment

fatso83 commented May 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick commented Jul 13, 2021

fatso83 commented Feb 10, 2024

fatso83 commented Mar 22, 2024

SimenB May 11, 2020 •

edited

codecov bot commented May 11, 2020 •

edited

SimenB commented May 11, 2020 •

edited