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(jest-fake-timers): Add feature to enable automatically advancing… #15300

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

atscott
Copy link

@atscott atscott commented Sep 11, 2024

… timers

Summary

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 commit attempts to resolve these issues by adding a feature which allows jest to advance timers automatically with the passage of time, just as clocks do without mocks installed.

Test plan

I wrote some unit tests but let me know if you need more.

Copy link

linux-foundation-easycla bot commented Sep 11, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Sep 11, 2024

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d0fe547
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/66f5a08e451f190008e0d312
😎 Deploy Preview https://deploy-preview-15300--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@atscott atscott force-pushed the autoAdvance branch 2 times, most recently from 835635d to fb18fa9 Compare September 11, 2024 23:07
@mrazauskas
Copy link
Contributor

Is the behaviour of this new method different from the advanceTimers option? If so, perhaps it would be good to mention that in documentation?

jest/docs/JestObjectAPI.md

Lines 883 to 889 in bd1c6db

type FakeTimersConfig = {
/**
* If set to `true` all timers will be advanced automatically by 20 milliseconds
* every 20 milliseconds. A custom time delta may be provided by passing a number.
* The default is `false`.
*/
advanceTimers?: boolean | number;

@atscott
Copy link
Author

atscott commented Sep 12, 2024

Is the behaviour of this new method different from the advanceTimers option? If so, perhaps it would be good to mention that in documentation?

Good question! Yes, it's quite different, both in correctness and in practicality. I'll update the documentation with some version of this explanation (and please correct me if any of this seems wrong):

advanceTimers 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. advanceTimers 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 advanceTimers
  2. advanceTimers 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". setAdvanceTimersAutomatically would be quite different: It advances time as quickly possible and as far as necessary. Without manual ticks, advanceTimers 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, setAdvanceTimersAutomatically can theoretically advance infinitely far, limited only by processing speed. Somewhat similar to the --virtual-time-budget feature of headless chrome.

Given that advanceTimers already exists in the config, it feels quite reasonable to me that this new feature should have a more unique name.

… timers

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 commit attempts to resolve these issues by adding a feature which allows jest to advance timers automatically with the passage of time, just as clocks do without mocks installed.
@mrazauskas
Copy link
Contributor

Interesting. Do I get it right that this new API is somewhat a variation of jest.advanceTimersToNextTimerAsync()? Or? I thought, perhaps instead of adding new one, it would be enough to extend the existing API:

  • jest.advanceTimersToNextTimerAsync('auto'),
  • jest.advanceTimersToNextTimerAsync('manual').

@atscott
Copy link
Author

atscott commented Sep 16, 2024

Do I get it right that this new API is somewhat a variation of jest.advanceTimersToNextTimerAsync()?

Yes, that's right!

I thought, perhaps instead of adding new one, it would be enough to extend the existing API:

  • jest.advanceTimersToNextTimerAsync('auto'),
  • jest.advanceTimersToNextTimerAsync('manual').

I think that's a pretty good idea for an alternative place for this API. Would jest.advanceTimersToNextTimerAsync('auto'/'manual') both advance the timer and update the internal setting/behavior? Or would it just change the behavior auto/manual behavior without advancing? It could be undesirable to not have the ability to change the behavior without also advancing to the next timer.

@mrazauskas
Copy link
Contributor

The idea I was talking about was to replace jest.setAdvanceTimersAutomatically(true) with jest.advanceTimersToNextTimerAsync('automatic') and setAdvanceTimersAutomatically(false) with jest.advanceTimersToNextTimerAsync('manual'). Or some such. Just an idea.

@atscott
Copy link
Author

atscott commented Sep 17, 2024

The idea I was talking about was to replace jest.setAdvanceTimersAutomatically(true) with jest.advanceTimersToNextTimerAsync('automatic') and setAdvanceTimersAutomatically(false) with jest.advanceTimersToNextTimerAsync('manual'). Or some such. Just an idea.

Ah, you mean that it the automatic/manual would entirely replace the existing steps parameter there? It might be worth considering, though I was hoping we could introduce this without making it a breaking change.

@atscott
Copy link
Author

atscott commented Sep 19, 2024

@mrazauskas happy to continue iterating on the API naming. Would you also be able to help with the CI failures? I’m not sure what’s going on there.

@mrazauskas
Copy link
Contributor

Hm.. I think those failures are not related with your code. GHA run all the tests and if they pass, I think all is fine.

@atscott atscott force-pushed the autoAdvance branch 10 times, most recently from 540e0b2 to 3213eef Compare September 20, 2024 23:45
@atscott
Copy link
Author

atscott commented Sep 21, 2024

@mrazauskas I've changed the implementation to overload advanceTimersToNextTimerAsync in a fixup commit. Let me know what you think of it this way.

Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps adding an example to the docs would be nice? The test that sets auto and switches back to manual looks like a good candidate. What you think?

packages/jest-environment/src/index.ts Outdated Show resolved Hide resolved
@yjaaidi
Copy link

yjaaidi commented Sep 23, 2024

Great idea! Thank you @atscott for putting this together.

In terms of API, I think it is better to rollback to setAdvanceTimersAutomatically() for the following reasons:

  1. The semantics are different. advanceTimersToNextTimerAsync() means "flush some microtasks/tasks and let me know when you are done" while setAdvanceTimersAutomatically() means "from now on and until told differently keep flushing all microtasks/tasks"
  2. advanceTimersToNextTimerAsync returns a promise which should be awaited for except when using auto. IDEs/linters warnings about the non-awaited promise will confuse users.
  3. it generally doesn't make sense to use advanceTimersToNextTimerAsync() (except for switching back to manual) once you set the timers to advance automatically so it would be nice to have two different methods. This would make it easier to implement lint rules that warn about the usage of advanceTimersToNextTimerAsync() when in 'auto' mode.

Finally, I feel that this feature can be confused with the advanceTime option.
Maybe something like instant could be more meaningful. What do you think?

{advanceTime: true} // deprecated
{advanceTime: false} // deprecated
{advanceTime: 'auto'} // current behavior of `advanceTime: true`
{advanceTime: 'manual'} // current behavior of `advanceTime: false`
{advanceTime: 'instant'} // the behavior introduced by this PR

// and a new function that allows switching during execution
jest.setAdvanceTime(...)

@mrazauskas
Copy link
Contributor

@yjaaidi Thanks for chiming in.

I like your proposed design. If you read the discussion, my first intuition was that this feature is somehow connected with the advanceTimers option.

Few details:

  • currently the option is named advanceTimers; do you suggest renaming to advanceTime or is that just a typo?
  • same regarding the method; should be jest.setAdvanceTimers(), or?
  • the advanceTimers option currently takes numbers too; so probably the type should be: 'auto' | 'instant' | 'manual' | number.

@yjaaidi
Copy link

yjaaidi commented Sep 23, 2024

I've read the discussion and I think that there was some kind of misunderstanding because indeed this feature can be seen as a variation of advanceTimers. That is why I commented to make sure that we are all aligned 😊

This is indeed a typo and I meant advanceTimers for both the option and the method.

I still wonder if number option should be kept or deprecated in favor of an options object with the following shape {mode: 'auto', delta: number} to make things more explicit and extensible.
Of course, all these deprecations and their alternatives can be added later.

What do you think?

@atscott
Copy link
Author

atscott commented Sep 23, 2024

I'm all for making this a new option on the advanceTimers config. One caveat is that the function new corresponding function for updating this after it's created, jest.setAdvanceTimers(...), would not have the ability to change the existing shouldAdvanceTime and advanceTimeDelta without upstream changes to sinon because it's only read and used on installation right now: https://github.com/sinonjs/fake-timers/blob/main/src/fake-timers-src.js#L1808C13-L1819

@atscott atscott force-pushed the autoAdvance branch 12 times, most recently from 16bc8d2 to a238999 Compare September 23, 2024 22:09
@atscott
Copy link
Author

atscott commented Sep 23, 2024

I've updated the implementation to extend the existing advanceTimers in the FakeTimersConfig and add a new setAdvanceTimers option with a warning about not being able to use it with the 'interval' option.

@atscott atscott force-pushed the autoAdvance branch 3 times, most recently from 5d84dbf to 20aa0e9 Compare September 26, 2024 17:54
@atscott
Copy link
Author

atscott commented Sep 26, 2024

I added some more tests to ensure the API works well with calls to the async tick methods. In doing this, I've started to feel that the auto tick machinery may be better implemented in sinon. This boils down to the need for a way to wait a macrotask before automatically advancing timers to prevent the act of updating the mode from also advancing to the next timer.

@mrazauskas
Copy link
Contributor

mrazauskas commented Sep 26, 2024

I've started to feel that the auto tick machinery may be better implemented in sinon.

Yep. This is what I was thinking earlier today. You did a lot of nice work here, so it didn’t felt right to tell that straight forward. But the feeling is that this machinery indeed belongs in Sinon. They would benefit from it as well. Also you know their codebase already (at least a bit), so perhaps that’s the way forward?

@atscott
Copy link
Author

atscott commented Sep 26, 2024

Yea, I'll convert this PR to draft until the story with implementing it in sinon becomes more clear since it'll still need to be exposed in the jest APIs and I think the tests and discussions around API are still valuable.

edit: sinonjs/fake-timers#509

@atscott atscott marked this pull request as draft September 26, 2024 18:22
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.

3 participants