Repository navigation
feat: Add additional auto advance time controls - #509
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thanks for this, I will have to check out the code at a later time.
just to check ... You are aware of |
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
|
|
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. |
|
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 ... |
…ock is created This adds a `setTickMode` method to the Clock to allow developers to configure whether time should auto advance and what the delta should be after the clock has been created. Prior to this commit, it could only be done at creation time with `shouldAdvanceTime: true`.
TL;DR This adds a new mode for automatically advancing time that moves
more quickly than the existing shouldAdvanceTime, which uses real time.
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](https://developer.chrome.com/docs/chromium/headless#--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.
|
@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). |
|
@fatso83 Is there any existing/accepted way in the Sinon fake timers code to check if it's running in node? |
|
@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 |
benjamingr
left a comment
There was a problem hiding this comment.
The actual code is fine but I don't think it's a particularly good feature since it breaks order and correctness. It's also a big patch.
I'm not blocking.
No worries, I totally get it.
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.
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?
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. |
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers.
…sync (#62135) Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers. PR Close #62135
…sync (#62135) Benefits of auto-ticking mock clocks have been described in other PRs, such as jasmine/jasmine#2042 and sinonjs/fake-timers#509. In short, `fakeAsync` cannot work when some tasks are required to be truly async, such as XHRs or observers like ResizeObserver. In addition, auto ticking mock clocks can be applied to tests without the tests then needing to update everything to manually flush timers. PR Close #62135
|
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. |
|
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 |
|
I did not use your code. I used runAllAsync.. 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. |
|
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 The reason the 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). |
|
By the way, you can get the same behavior as the 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. |
|
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. |
|
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. |
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. |
|
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 😄 |
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 ( |
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.
shouldAdvanceTimeis 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:
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
shouldAdvanceTimein the following ways:shouldAdvanceTimeis essentiallysetInterval(() => clock.tick(ms), ms)while this feature isconst loop = () => setTimeout(() => clock.nextAsync().then(() => loop()), 0);There are two key differences between these two:
shouldAdvanceTimeusesclock.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 liketickAsync(ms)(or a loop aroundnextAsync) would. This could arguably be considered a fixable bug in its implementationshouldAdvanceTimeuses 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,shouldAdvanceTimewould 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.