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

feat: Add additional auto advance time controls #509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

atscott
Copy link

@atscott atscott commented Sep 26, 2024

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.

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.
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fatso83
Copy link
Contributor

fatso83 commented Sep 27, 2024

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
Copy link
Author

atscott commented Sep 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants