fix(vitest): decouple fake/real time in waitFor()'s check #6802
Conversation
When running with fake timers, and when polling for promise to complete, make sure we do the poll in a short *real* time interval, independent of the fake time interval. This makes it possible to use long time intervals in fake time without needing to wait for the corresponding amount of real time passing. A unit test is added to illustrate this behaviour: with the previous code, this unit test would have taken over a minute to complete, now it is near instantaneous.
samavos
changed the title
samavos
changed the title
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Please, don't drop the PR template: Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
The first point is especially important as we can't evaluate the usefulness of this change without examples and a proper discussion. |
Apologies, I created the PR with I was about to raise an issue, but thought the issue was very clear from a unit test, which I've included. If it's helpful to raise a issue and move the discussion there I'm happy to do that. But as it stands, there is a unit test and description here which would be the information I'd put in the issue. Would you like to consider discuss here, or shall I move this to an issue? |
You can start here.
The reasoning doesn't really make it clear to me. The test is very artificial. The |
The test is artificial, but it's based on a real use-case in my project. I've just simplified it. The current behaviour causes real problems in a (closed) project I'm working on. I believe the purpose of the I'd be interested in knowing what your definition of the A way to change this to not introduce any change in behaviour is to add another configuration option for this case; but that of course increases the size of the API. I'd be equally happy with such an approach in my use-case. |
You can already provide your own |
OK, fair enough.
I'm not entirely sure I follow that suggestion. In my case, I have the following scenario:
I believe the right answer here is: In the few cases I'd like to use Are you suggesting there is a way to use the API to solve for this case today without any modifications? I haven't quite been able to figure out what that is based on your last comment, sorry. |
Why do you even need such a big interval? It means the callback will be called once every 30 seconds. Don't you need a
Yeah, what you did in this PR looks like this in user code: vi.waitFor(() => {}, {
interval: vi.isFakeTimers() ? 40 : 30000
}) |
Because we're testing code that really does, in the real world, use long intervals. One case that is hopefully easy to describe: (something using) an exponential backoff timer. Consider something with a backoff that quickly results in the backoff time being a long duration after some error conditions are hit. It is reasonable to check that something will succeed after some amount of backing off.
Sorry, I don't agree. Are we perhaps talking at cross purposes? There are two separate things here: the amount of fake time that is increased per time we check, and the amount of real time that needs to pass. Consider the unit test code here: vi.useFakeTimers()
await vi.waitFor(() => {
return new Promise<void>((resolve) => {
setTimeout(() => {
resolve()
}, 60000)
})
}, { interval: 30000 })Your suggestion is that we change the interval there to read: In this trivialised case, this will obviously set the interval to 40.
I had a look at |
sheremet-va
added
the
p2-to-be-discussed
|
We talked within the team about this issue. We still don't understand the use case. Can you give an actual code example that helps understand this? And can you explain why you need to use |
|
Sorry for the slow reply, it required a bit of concentration to pull something cohesive together. Here is a full test file showing one real-ish use-case, and I'll add some commentary after. codeimport { describe, it, expect, vi, beforeEach } from 'vitest'
import { waitFor as waitForHack } from './waitForHack'
class Backoff {
constructor(private errors = 0, private lastError = 0) {}
finish(ok: boolean) {
if (ok) {
this.errors = 0;
} else {
this.errors++;
this.lastError = Date.now();
}
}
permitted() {
return Date.now() > this.getDelayUntil();
}
getDelayUntil() {
if (this.errors === 0) return 0;
return this.lastError + Math.max(200, Math.pow(2, this.errors - 1) * 500);
}
}
async function delay(ms: number, signal?: AbortSignal): Promise<void> {
return new Promise((resolve, reject) => {
const timeout = setTimeout(resolve, ms);
if (signal) {
signal.onabort = () => {
clearTimeout(timeout);
reject(new Error("Aborted"));
}
}
});
}
async function retryingFetch(
...args: Parameters<typeof fetch>
): Promise<any> {
const backoffManager = new Backoff();
let haveFailed = false;
const startTime = Date.now();
while (true) {
try {
const response = await fetch(...args);
if (response.ok) {
return response;
}
haveFailed = true;
if (response.status >= 400 && response.status < 500) {
return response;
}
}
catch (e) {
if (e instanceof Error && e.name === "AbortError") {
throw e;
}
}
backoffManager.finish(false);
if (!backoffManager.permitted()) {
const next = backoffManager.getDelayUntil();
// console.log(`${Date.now()}: Waiting ${next - Date.now()}ms before retrying`);
await delay(next - Date.now(), args[1]?.signal || undefined);
}
}
}
describe("waitFor experiments", () => {
beforeEach(() => {
vi.useFakeTimers();
return () => {
vi.useRealTimers();
}
});
it.each([
["original waitFor", vi.waitFor],
["hacky waitFor", waitForHack],
])("should wait for one error: %s", async (_name, waitFor) => {
vi.spyOn(globalThis, 'fetch')
.mockImplementationOnce(() => { throw new Error('Network error'); })
.mockImplementationOnce(() => { return Promise.resolve(new Response(null, { status: 200 })); });
const response = await waitFor(async () => {
return await retryingFetch('https://example.com');
})
expect(response.ok).toBe(true);
});
it.each([
["original waitFor", vi.waitFor],
["hacky waitFor", waitForHack],
])("should wait for 4 errors: %s", async (_name, waitFor) => {
const networkError = () => { throw new Error('Network error'); };
let requests = 0;
vi.spyOn(globalThis, 'fetch')
.mockImplementation(() => {
requests++;
if (requests < 4) networkError();
return Promise.resolve(new Response(null, { status: 200 }));
});
const response = await waitFor(async () => {
return await retryingFetch('https://example.com');
}, {
interval: 1000,
})
expect(response.ok).toBe(true);
});
it("should wait for 4 errors: manual version", async () => {
const networkError = () => { throw new Error('Network error'); };
let requests = 0;
vi.spyOn(globalThis, 'fetch')
.mockImplementation(() => {
requests++;
if (requests < 4) networkError();
return Promise.resolve(new Response(null, { status: 200 }));
});
let finished = false;
let response: Response | undefined;
retryingFetch('https://example.com')
.then(resp => {
response = resp;
finished = true;
});
while (!finished) {
await vi.advanceTimersByTime(1000);
}
expect(response!.ok).toBe(true);
});
})The file running the codescenario descriptionHere I've thrown together a hopefully illustrative example of a wrapper around The exact thing being fetched in this example is trivial, but in a more real-life example it might be API requests buried between various layers of code. That is what we have in one of our private repos. Tests want to assert that API requests eventually succeed, even in the case of errors seen. This allows us to assert something like "yes, this action really is configured in the right way such that it handles network errors reasonably" when thinking about how some module is invoked in a test scenario. commentaryintroI think the
In this scenario we're only looking at (2) above. mainMy view of the output and code is that we can see the following things:
For (3), I tried to show this in In essence, my argument here pans out to the following:
addendumI completely understand if this isn't a scenario you want to support; if so let's close down this PR. Apologies again for not raising it as a discussion topic rather than a PR to start with. From our perspective in our workplace, we've ended up having our own |
|
Thank you for the detailed explanation of your use case.
This doesn't seem correct. If you need to await an asynchronous operation, just use From your example, it seems like Overall, the team doesn't think this is a valid use case for |
|
Ok, thanks for taking the time to consider and explaining the rationale @sheremet-va . For what it's worth, the guidance "just use |
When running with fake timers, and when polling for promise to complete, make sure we do the poll in a short real time interval, independent of the fake time interval.
This makes it possible to use long time intervals in fake time without needing to wait for the corresponding amount of real time passing. A unit test is added to illustrate this behaviour: with the previous code, this unit test would have taken over a minute to complete, now it is near instantaneous.