feat(@schematics/angular): add refactor-fake-async migration#32784
feat(@schematics/angular): add refactor-fake-async migration#32784yjaaidi wants to merge 2 commits intoangular:mainfrom
refactor-fake-async migration#32784Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new experimental schematic, refactor-fake-async, to migrate Angular's fakeAsync tests to use Vitest's fake timer APIs. The changes include the schematic implementation, transformers for fakeAsync, tick, and flush, and corresponding tests. Additionally, the findTestFiles utility function has been refactored into a shared location.
My review focuses on improving the robustness of the new transformers. I've suggested changes to ensure that the necessary vitest imports are always added when tick() or flush() are transformed, which will prevent potential issues with broken code after migration.
packages/schematics/angular/refactor/fake-async/transformers/tick.ts
Outdated
Show resolved
Hide resolved
atscott
left a comment
There was a problem hiding this comment.
- This migration could be part of the jasmine-vitest migration. Should I moved it there and add an opt-out?
Makes sense to me.
- flushMicrotasks can only be partially reproduced (i.e. explicit queueMicrotask calls), transforming it have high chances of producing a broken test. Should we just skip transforming tests that use it?
Hmm, wouldn't await vi.advanceTimersByTimeAsync(0) be close enough?
- Should we generate code that measures time "manually" when the return value of flush is used?
Like with Date? I suppose we could and that'd get us there. Do people really use the return value....? Maybe just add a TODO if people ask for it.
- Should we generate an "ad-hoc" function in the test file that reproduces flush's maxTurns option?
like with fakeTimers.loopLimit? Maybe this can also be a TODO and we just see if people really need it.
- What should we do about processNewMacroTasksSynchronously option?
Kill it 🔥
- This only migrates flush and tick if used inside fakeAsync but it's probably better to just replace in the whole file as users might create some wrappers. What do you think?
Yea, I had a comment about this and am seeing this note now. Probably should migrate it. They need to be in a fakeAsync somewhere up the chain.
|
|
||
| const newContent = await transformContent(` | ||
| it("should work", () => { | ||
| flush(); |
There was a problem hiding this comment.
can't flush be in a helper method? if the flush is the one from @angular/core/testing I would think it should be transformed since it would throw if it wasn't in fakeAsync somewhere
There was a problem hiding this comment.
This answers one on two subsequent questions:
- shouldn't we only transform identifiers if source is
@angular/core/testing? I assume that we should as there are higher chances of name collisions than users importing the realtick/flushthrough a custom barrel module (e.g.import { tick, flush } from 'my-custom-barrel-that-reexports-angular-core-testing') - no need to handle things like aliased imports of
tickandflush, right? Sounds too much of an edge-case to me.
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
flushMicrotaskscan 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?flushis used?flush'smaxTurnsoption?processNewMacroTasksSynchronouslyoption?flushandtickif used insidefakeAsyncbut it's probably better to just replace in the whole file as users might create some wrappers. What do you think?