feat(@schematics/angular): add option to migrate fakeAsync to Vitest fake timers#33111
feat(@schematics/angular): add option to migrate fakeAsync to Vitest fake timers#33111yjaaidi wants to merge 5 commits intoangular:mainfrom
fakeAsync to Vitest fake timers#33111Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for migrating Angular fakeAsync tests to Vitest by introducing a new fakeAsync option and corresponding transformers for tick, flush, and flushMicrotasks. The implementation converts these utilities to Vitest's vi fake timer API. Reviewers suggested scoping the fake timer hooks more narrowly to avoid side effects on sibling tests, disabling shouldAdvanceTime to better match fakeAsync semantics, and refining the flush transformer's logic for detecting discarded return values.
e90eef2 to
fbd8e15
Compare
cc415cf to
24627cd
Compare
| * @param methodeName The name of the vitest method to call. | ||
| * @returns The created identifier node. | ||
| */ | ||
| export function createViCallExpression( |
There was a problem hiding this comment.
I allowed my self to move this from ast-helpers to refactor-helpers so that it automatically adds pending vitest imports.
| `, | ||
| }, | ||
| { | ||
| description: 'should not replace `fakeAsync` if not used within a describe block', |
There was a problem hiding this comment.
This exact scenario is not likely within Jasmine but fakeAsync could be used in some test factory or something similar.
In that case, we just bail.
| // > beforeAll(() => { | ||
| // > vi.useFakeTimers({ | ||
| // > advanceTimeDelta: 1, | ||
| // > shouldAdvanceTime: true |
There was a problem hiding this comment.
Probably the best alternative to use fake timers without freezing other tests in the suite which rely on rely timers.
fakeAsync to Vitest fake timers
There was a problem hiding this comment.
Code Review
This pull request introduces the fakeAsync transformation to the jasmine-vitest schematic, enabling the migration of Angular fakeAsync tests to Vitest's fake timers. It adds specific transformers for flush, flushMicrotasks, tick, and the fakeAsync wrapper, supported by new utility functions for AST manipulation. Review feedback highlights several areas for improvement: ensuring tick() arguments are preserved even when they are not numeric literals, maintaining parameters and type parameters when converting callbacks to async functions, adjusting Vitest timer settings to better match standard fakeAsync behavior, and correcting misleading comments regarding vi.useRealTimers().
| const durationNumericLiteral = | ||
| node.arguments.length > 0 && ts.isNumericLiteral(node.arguments[0]) | ||
| ? node.arguments[0] | ||
| : ts.factory.createNumericLiteral(0); | ||
|
|
||
| return ts.factory.createAwaitExpression( | ||
| createViCallExpression(ctx, 'advanceTimersByTimeAsync', [durationNumericLiteral]), | ||
| ); |
There was a problem hiding this comment.
The current implementation only preserves the duration if it is a numeric literal. If a variable or expression is passed to tick(), it is incorrectly transformed to 0. The expression should be preserved regardless of its type to ensure the test logic remains correct.
const duration =
node.arguments.length > 0 ? node.arguments[0] : ts.factory.createNumericLiteral(0);
return ts.factory.createAwaitExpression(
createViCallExpression(ctx, 'advanceTimersByTimeAsync', [duration]),
);There was a problem hiding this comment.
This looks legitimate. Can you address?
| return ts.factory.createArrowFunction( | ||
| [ts.factory.createModifier(ts.SyntaxKind.AsyncKeyword)], | ||
| undefined, | ||
| [], | ||
| undefined, | ||
| ts.factory.createToken(ts.SyntaxKind.EqualsGreaterThanToken), | ||
| ts.factory.createBlock(callbackBody.statements), | ||
| ); |
There was a problem hiding this comment.
When transforming the fakeAsync callback to an async function, any existing parameters or type parameters should be preserved. Although fakeAsync callbacks typically don't have parameters, preserving them ensures compatibility with any custom wrappers or edge cases where types are explicitly defined.
| return ts.factory.createArrowFunction( | |
| [ts.factory.createModifier(ts.SyntaxKind.AsyncKeyword)], | |
| undefined, | |
| [], | |
| undefined, | |
| ts.factory.createToken(ts.SyntaxKind.EqualsGreaterThanToken), | |
| ts.factory.createBlock(callbackBody.statements), | |
| ); | |
| return ts.factory.createArrowFunction( | |
| [ts.factory.createModifier(ts.SyntaxKind.AsyncKeyword)], | |
| fakeAsyncCallback.typeParameters, | |
| fakeAsyncCallback.parameters, | |
| fakeAsyncCallback.type, | |
| ts.factory.createToken(ts.SyntaxKind.EqualsGreaterThanToken), | |
| ts.factory.createBlock(callbackBody.statements), | |
| ); |
There was a problem hiding this comment.
Fixed by 3707db7
But not keeping the return type as it would be invalid given the function is turned into an async function. Transforming the return type to a Promise would be over-engineering as I can't imagine a scenario where such typing would be useful.
20329da to
e3ff0a2
Compare
874cbe9 to
1ed014b
Compare
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
There is no migration from
fakeAsyncto Vitest fake timer APIs.Issue Number: N/A
What is the new behavior?
The current migration transforms the following test:
into:
Does this PR introduce a breaking change?
Open Questions
This migration could be part of the jasmine-vitest migration. Should I moved it there and add an opt-out?
Moved to jasmine-vitest migration under
fakeAsyncoption which is false by defaultflushMicrotaskscan only be partially reproduced (i.e. explicitqueueMicrotaskcalls), transforming it have high chances of producing a broken test. Should we just skip transforming tests that use it?Migrated to
await vi.advanceTimersByTimeAsync(0);.Should we generate code that measures time "manually" when the return value of
flushis used?Generates a TODO.
Should we generate an "ad-hoc" function in the test file that reproduces
flush'smaxTurnsoption?Generates a TODO.
What should we do about
processNewMacroTasksSynchronouslyoption?Ignored.
This only migrates
flushandtickif used insidefakeAsyncbut it's probably better to just replace in the whole file as users might create some wrappers. What do you think?Replaced everywhere.