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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

API for timer mocks that are safely isolated between tests #5750

Open
4 tasks done
AlexandrHoroshih opened this issue May 18, 2024 · 3 comments
Open
4 tasks done

API for timer mocks that are safely isolated between tests #5750

AlexandrHoroshih opened this issue May 18, 2024 · 3 comments
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)

Comments

@AlexandrHoroshih
Copy link

AlexandrHoroshih commented May 18, 2024

Clear and concise description of the problem

Hello and thanks for the great project! 馃挋

Small disclaimer

I have seen comments in the #5665 and i'm aware that Vitest

cannot use AsyncLocalStorage in core since it's a Node.js API

But i think that this API idea might be useful, once AsyncContext proposal will be implemented in the browsers sometime in the future, so i decided to propose it anyway 馃槄

The problem

Current time mocks API has a noticeable disadvantage:
The vi.useFakeTimers()/useRealTimers() switch is global, at least for all tests in the current worker thread/child process

This, for example, may lead to problems, especially when running tests concurrently - since fake timers are not isolated between tests, they may leak between those and cause unexpected and confusing behaviour

As a side note: i probably would prefer, if vitest had banned using global useFakeTimers() switch along with test.concurrent in the same test suite 馃

And even if we rule out .concurrent tests as a special case, the global switch behaviour is still may be quite confusing at times, since there are cases like

  • libraries saving separate reference to e.g. original setTimeout before any mocking code was run, so user ends up with two concurrent time flows in the same test 馃く
  • if useRealTimers switch was not called at the end of the test, next one also gets fake time anyway, which also may lead to unexpected and confusing behaviour, depending on the useFakeTimers config

Suggested solution

So, since the core of the problem is the "global switch" nature of current fake timers API, the alternative must be "local" for the each test

I suggest API like this:

import { test, runWithFakeTime } from 'vitest'

test.concurrent('my-test-1', async () => {
 await runWithFakeTime((timeControls) => {
     const promise = startSomeTimeRelatedOperations()
     
     timeControls.advanceTimeBy(42)
     
     await p;
     
     expect(...).toBe(...)
  })
})

test.concurrent('my-test-2', async () => {
  // does not get affected by time mocks in previous test
  // even if was run concurrently
})

鈽濓笍 In this example it is expected, that time would be mocked only inside runWithFakeTime callback and any of event-loop tasks which were produced by it, which obviously requires API like AsyncContext

To do that all of the time-related APIs must be mocked in a way, that they use fake implementation if called inside runWithFakeTime context and native one if called outside of it
And i think, this mock should happpen before any other userland code in the suite was initialized, so any code, that saves separate reference to e.g. setTimeout, still gets mocked as expected

Unlike global switch, this API would allow for clear isolation of time mocks from any other code in the same suite, since "fake time zone" is separated into its own async context

Here is a little Proof-of-Concept example, which uses Node.js AsyncLocalStorage for this purpose:
https://stackblitz.com/edit/vitejs-vite-4hqcwk?file=tests%2Flib.js

Alternative

No response

Additional context

I have seen comments in the #5665 and i'm aware that Vitest

cannot use AsyncLocalStorage in core since it's a Node.js API

But i think that this API idea might be useful, once AsyncContext proposal will be implemented in the browsers sometime in the future

So i guess, this idea can wait until then

Validations

@sheremet-va
Copy link
Member

We are using @sinon/fake-timers for timers mocking, so it would have to be implemented by them because we don't override global timers access. Or we would have to reimplement mocking ourselves which is also an option.

In any case, we can't do anything until the AsyncContext API is implemented at least in one of the browsers and Node.js.

@sheremet-va sheremet-va added p2-nice-to-have Not breaking anything but nice to have (priority) and removed enhancement: pending triage labels May 18, 2024
@victordidenko
Copy link

because we don't override global timers access

Sorry, but vitest definitely overrides global functions. Using @sinon/fake-timers's withGlobal, but with globalThis nonetheless.

const timers = () => _timers ||= new FakeTimers({
global: globalThis,
config: workerState.config.fakeTimers,
})

@sheremet-va
Copy link
Member

sheremet-va commented May 18, 2024

because we don't override global timers access

Sorry, but vitest definitely overrides global functions. Using @sinon/fake-timers's withGlobal, but with globalThis nonetheless.

const timers = () => _timers ||= new FakeTimers({
global: globalThis,
config: workerState.config.fakeTimers,
})

What I mean is that we don't override it (Vitest in its source code doesn't reassign globalThis.setTimeout to control how it works, so we can't inject the context there), not that we don't override it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

No branches or pull requests

3 participants