feat: Add additional auto advance time controls

[atscott]

Purpose (TL;DR)

This adds a new mode for automatically advancing time that moves more quickly than the existing shouldAdvanceTime, which uses real time. It also adds the ability to modify the initial values that were used for shouldAdvanceTime and advanceTimeDelta. shouldAdvanceTime is useful in some situations, but is not useful for solving the problem that testers usually install mock clocks for (to speed up time).

Background

Testing with mock clocks can often turn into a real struggle when dealing with situations where some work in the test is truly async and other work is captured by the mock clock.

In addition, when using mock clocks, testers are always forced to write tests with intimate knowledge of when the mock clock needs to be ticked. Oftentimes, the purpose of using a mock clock is to speed up the execution time of the test when there are timeouts involved. It is not often a goal to test the exact timeout values. This can cause tests to be riddled with manual advancements of fake time. It ideal for test code to be written in a way that is independent of whether a mock clock is installed or which mock clock library is used. For example:

document.getElementById('submit');
// https://testing-library.com/docs/dom-testing-library/api-async/#waitfor
await waitFor(() => expect(mockAPI).toHaveBeenCalledTimes(1))

When mock clocks are involved, the above may not be possible if there is some delay involved between the click and the request to the API. Instead, developers would need to manually tick the clock beyond the delay to trigger the API call.

This is different from the existing shouldAdvanceTime in the following ways:

shouldAdvanceTime is essentially setInterval(() => clock.tick(ms), ms) while this feature is const loop = () => setTimeout(() => clock.nextAsync().then(() => loop()), 0);

There are two key differences between these two:

1. shouldAdvanceTime uses clock.tick(ms) so it synchronously runs all timers inside the "ms" of the clock queue. This doesn't allow the microtask queue to empty between the macrotask timers in the clock whereas something like tickAsync(ms) (or a loop around nextAsync) would. This could arguably be considered a fixable bug in its implementation
2. shouldAdvanceTime uses real time to advance the same amount of real time in the mock clock. The way I understand it, this feels somewhat like "real time with the opportunity to advance more quickly by manually advancing time". This would be quite different: It advances time as quickly possible and as far as necessary. Without manual ticks, shouldAdvanceTime would only be capabale of automatically advancing as far as the timeout of the test and take the whole real time of the test timeout. In contrast, setTickMode({mode: "nextAsync"}) can theoretically advance infinitely far, limited only by processing speed. Somewhat similar to the --virtual-time-budget feature of headless chrome.

In addition to the "quick mode" of shouldAdvanceTime, this also adds the ability to modify the initially configured values for shouldAdvanceTime and advanceTimeDelta.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.59%. Comparing base (6f90a44) to head (8bc8b1d). Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #509      +/-   ##
==========================================
+ Coverage   97.56%   97.59%   +0.02%     
==========================================
  Files          16       18       +2     
  Lines        4430     4606     +176     
==========================================
+ Hits         4322     4495     +173     
- Misses        108      111       +3     

Flag	Coverage Δ
unit	97.59% <100.00%> (+0.02%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[fatso83]

Thanks for this, I will have to check out the code at a later time.

testers are always forced to write tests with intimate knowledge of when the mock clock needs to be ticke

just to check ... You are aware of await clock.runAllAsync() and await clock.runToLastAsync(), right? They often solve the issues of needing intimate knowledge about when timers are being fired and when Promises are needing to be resolved.

[atscott]

just to check ... You are aware of await clock.runAllAsync() and await clock.runToLastAsync(), right? They often solve the issues of needing intimate knowledge about when timers are being fired and when Promises are needing to be resolved.

Yes, these are quite useful! Here are some of my thoughts where these still cause some test friction that this feature would help resolve:

When mock clocks are installed, it's not "safe" to await just anything

When writing tests, you have to know that whatever you're awaiting will not wait for any timers to complete. Either that, or you need to make sure that you flush timers before awaiting any promises. It's especially difficult if the function causes a timer to be queued and then waits for it. The way around this would be to save the promise instead of awaiting it, then flush timers, then await the promise (pretty awkward).

When mock clocks are installed, test utilities need to know about it

Today, test utilities that perform async actions need to be written to account for mock clocks. In the above situation, if the function we're talking about was some test harness or test utility, we could tell it how to tick the clock so it can advance time internally rather than us having to do it in the test body.

Concrete example: Testing Gmail's "undo" send button

Consider testing the "undo" send button feature in Gmail. Conceptually, what you want to do in the test is "click send, wait for the toast to appear, then click the 'undo' button". The undo button doesn't appear synchronously and would require running some timers to make it appear. Using runAllAsync would advance time to the point where the email is fully sent and the toast disappears. You'd have to write the test to tick some amount of time for it to appear, but not long enough for it to disappear.

await sendButton.click();
await clock.tickAsync(100);
const undoButton = await page.undoButton();
await undoButton.click();
// follow-up expectations

With the auto-clicking mock clock, this test could instead look much more like it would in an e2e environment (and in fact, could even use the same test body, with the page harnesses swapped out underneath which uses WebDriver or the framework testing library depending on environment):

await sendButton.click();
// Maybe undoButton getter polls for the button with a timeout
const undoButton = await page.undoButton();
await undoButton.click();

---

In each of the above situations, an automatically advancing mock clock would make testing much more reasonable. It's certainly not always the answer and it may also still be useful to manually flush timers (i.e. in the "undo" button example, flushing timers after clicking undo and then asserting that the RPC to send the message never happened). While runAllAsync and runToLastAsync are absolutely useful, they still require the test author to actually call them to advance time. It's less intimate knowledge of the timer, but it is still needing to know that the promise doesn't resolve in the event loop.

[atscott]

Hi @fatso83, is there anything more you need from me here?

We feel like this is quite a powerful feature, unlocking all the benefits of a fast mock clock without needing to change anything else about how a test is written. One might discover a slow test suite and only need to install an auto-ticking mock clock to speed things up.

[fatso83]

Sorry about that. Was hoping someone else on the maintainer team would pick it up, and I have simply forgotten about it 😄 I will have a new look tomorrow (and remind me if there's no update by next week), as I no longer remember the details ...

[atscott]

@fatso83 I updated the commit to split the new "setTickMode" feature out from the new "nextAsync" mode. Hopefully this makes it easier to review (and apologies if you had pending review comments that were invalidated by the rebase).

[atscott]

@fatso83 Is there any existing/accepted way in the Sinon fake timers code to check if it's running in node? setImmediate is a pretty fairly significant improvement over setTimeout. I found that it's often several times faster than setTimeout(0) and made that change to the jasmine version of this PR (https://github.com/jasmine/jasmine/pull/2058)

[fatso83]

@atscott Hi, Scott. Seeing this hanging for such a long time is embarrassing, but I have almost not been touching JS for the last six months, so it's been hard justifying working on this. Hopefully, you have been able to fork this for your own needs in the mean time or patch the code to not be blocked. I'll try to give it some love soon.

I am on my phone now, so giving great feedback is awkward, but we already do feature detection in the first few lines. That means you don't need to know if you are on node to use setImmediate, you can just use the existing check to see if it's present and just use setImmediate directly.

[atscott]

Seeing this hanging for such a long time is embarrassing, but I have almost not been touching JS for the last six months, so it's been hard justifying working on this. Hopefully, you have been able to fork this for your own needs in the mean time or patch the code to not be blocked. I'll try to give it some love soon.

No worries, I totally get it.

you can just use the existing check to see if it's present and just setImmediate directly

I had some concern with browsers that happened to implement it but I suppose it would probably still just work as intended if they do. Plus, those very few that have it are likely far out of support.

I don't think it's a particularly good feature since it breaks order and correctness

Would you mind expanding on this a bit more? Do you have an example where this breaks down in a place that it doesn’t already with existing APIs?

It's also a big patch

That’s fair. Quite a bit of the change is in the first commit that only adds the ability to update the ticking mode between the two options that already exist.

The other large portion comes from the need for the MessageChannel workaround for timeout throttling. Aside from that, it’s mostly white space changes around the async tick methods. Have you tried turning off the white space in the diff settings?

If you prefer, I could likely reduce the raw diff by removing the ability to change modes after the clock was created and also remove some longer explanatory comments.

[Uzlopak]

I wanted to implement something like this today.

Basically I did:

const timer = setTimeout(() => {
  clock?.tick(100)
  timer.refresh()
}, 1).unref()

const clock = FakeTimers.install()

This works reliable. But I was intrigued by runAllAsync(). So I tried it, and it was unreliable and made the tests randomly hang.

[fatso83]

I have forgotten about this, so this was a great reminder. I stopped for the same reasons that Andrew Asked about: I was uncertain about the order and correctness.

@Uzlopak , can you expand on your comment: "it was unreliable and made the tests randomly hang."?

I assume this meant you have checkout out this fork and used it to test out the new API. Yes?

Can you please post some example code that uses the new time controls that will show this randomly failing behavior? Without this, it is very unclear if this is user error (wrong use of runAllAsync) or an inherent design issue.

[Uzlopak]

I did not use your code. I used runAllAsync..

https://github.com/nodejs/undici/pull/4526/files#diff-bf38571af1c6adc939fb9ea1e5af94cd46ae0933dd9298aaa6cef7b5835fd2a3

Replacing

  const timer = setTimeout(() => {
    clock?.tick(100)
    timer.refresh()
  }, 0).unref()

  const clock = FakeTimers.install()
  after(() => clock.uninstall())

with

  const clock = FakeTimers.install()
  clock.runAllAsync()

and then running the test file multiple times, than one of the runs will get stuck.

[atscott]

IIUC, the tests rely on truly async behavior that cannot be mocked by sinon. For example, in this test, you can create a timeout error for the test by calling clock.runAll() or call clock.next() 21 times, which flushes the timeout on the request and all its allotted retries prior to the real async request being finished.

The reason the clock?.tick(100) "works" is because it never ends up advancing time far enough to cause the retries to fail since it would take 10e3/100*21 of the setTimeout(..., 0) calls to burn through all the request timeouts and retries.

Yes, this is a situation where the auto ticking isn't a good fit. I don't think that means this is a bad feature on account of it not being "one size fits all".

For my part, I've spent time increasing adoption for auto tick usage where it's available (jasmine). In the Angular Router tests, enabling it reduced test time by nearly half, without needing to do anything other than turn it on (no other changes to test bodies to manually advance time).

[atscott]

By the way, you can get the same behavior as the setTimeout(() => clock.tick(100); timer.refresh();) version with autoTick/runAllAsync by adding a no-op timer every 100ms setTimeout(() => { timer.refresh(); }, 100). This achieves the exact same behavior by having auto tick advance 100ms automatically (IMO it’s actually better because it’s more like await tickAsync(100); timer.refresh(), which allows microtask draining between timers)

So I do think again that the “ordering” issues can be hit with existing APIs and also that autoTick can be used to achieve various outcomes. Cool to see someone discover this PR organically. The implementation from @Uzlopak is auto tick, and achieves the speed benefits described here, but with a 100ms interval thrown in to slow it down a bit for the network requests.

[Uzlopak]

To be honest, the problem with ticking is, that at the moment you program the feature, you have somewhat knowledge on how it works and you know how to fast forward the time. You know how you probably fix the some async testing issues, maybe even change the actual code to make it better testable.

Now these test files are huge, old. and they take a loong time to finish. Well it takes a long time for me. One of these retry-handler.js tests takes about 20 seconds. If my PR gets accepted it will take less than 1 second. So the auto fast forward helps to speed up the test and to avoid to dig deep into the code base to actually tick the time really by millisecond precision.

I would actually prefer the precision ticking and not auto ticking. But the gain is now kind of insignificant compared to what auto ticking already achieved. Lets say, the test runs now for 700ms instead of 20 seconds. Programming a precision ticking maybe gives me 300 ms. Sure, A 50% speed gain. But compared to 20 seconds it is neglectable.

It would be cool if there was a builtin functionality. I have no problem, if there was some warning, that says, that you should know your code and use it at your own risk.

E.g. I know how the internals of undici work and that a stepping of 100ms should be not critical. Also by using setTimeout it should actually result that the event loop gets released after the run.

[johncrim]

I for one would love to see autoTick integrated into sinon.js. I believe it would be useful for many testing use cases.

I have migrated some of our angular tests using jasmine to use the autoTick that @atscott created, and it made the tests more maintainable by removing internal timing knowledge from the test. (internal timing knowledge is still useful in some tests if you're verifying the timing, but in the general case for browser tests that is not the case). For our code base autoTick should be the default.

We also have a lot of tests using jest, and would like to migrate to vitest; and as you know both are dependent on this project.

[jitterbox]

I for one would love to see autoTick integrated into sinon.js. I believe it would be useful for many testing use cases.

Agree, +1 for the sake of our vitest Angular testing now that zoneless is stable and we're having to replace zonejs-dependent methods; please try to reach an internal conclusion on this as it's coming up on 1 year under consideration.

[fatso83]

This has had enough time to mature and enough eyes on its that I feel somewhat OK in merging this. Even if I don't understand it fully 🙈 The test suite and docs seems solid, and people have had time to raise any objections. The reason this has stalled is that I no longer have any time to take deep dives into this, as I have not been coding any javascript for the last year or so, and so cannot justify spending free time in quality reviews. So that leaves about none with the combo of sufficient time, will and skillset to review solid stuff like this. I saw Benji mention "breaks order and correctness"; I have not seen traces of what this actually entails with any examples, which leaves me clueless, but knowing his endavours on the main NodeJS projects, I have no issues believing him. Still, many see great value in this feature and seems to have tested it, so I'll merge and release it ASAP.

Sorry for keeping it open for such a long time - hope you see why. And if you are wondering; yes, we do need to attract some new (active) maintainers, even if this is a pretty robust little lib with a quite stable API 😄

[atscott]

Sorry for keeping it open for such a long time - hope you see why.

No worries at all! As a maintainer of an open source library myself, I appreciate the thoughtfulness and caution. I could have easily understood waiting another year, even if only to see how things play out in the jasmine version (jasmine.clock().autoTick()) that was released in 5.7.