fix(@schematics/angular): preserve Jasmine stub-by-default behavior f…#33263
fix(@schematics/angular): preserve Jasmine stub-by-default behavior f…#33263spliffone wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Jasmine-to-Vitest schematic to preserve Jasmine's stub-by-default behavior when transforming spyOn and spyOnProperty calls, mapping them to vi.spyOn(...).mockReturnValue(undefined). The feedback identifies critical issues where casting spyCall to ts.CallExpression can cause runtime crashes if the spy is referenced via a variable (e.g., spy.and.returnValues() or spy.and.callThrough()). Additionally, the logic for determining whether to stub a bare spy is too broad and should be restricted to property accesses on .and to avoid incorrectly stripping stub behavior on other methods like .calls.reset(). Finally, it is recommended to add test cases verifying these edge cases on spy variables.
| // .and.callThrough() is redundant, just transform spyOn. | ||
| return transformBareSpyOn(spyCall as ts.CallExpression, refactorCtx, false); |
There was a problem hiding this comment.
Similar to the returnValues case, if spyCall is a variable reference (e.g. spy.and.callThrough()), casting it to ts.CallExpression and passing it to transformBareSpyOn will cause a runtime crash. We should check if spyCall is a spyOn or spyOnProperty call, and if not, simply return spyCall as .and.callThrough() is redundant.
// .and.callThrough() is redundant, just transform spyOn.
if (
ts.isCallExpression(spyCall) &&
ts.isIdentifier(spyCall.expression) &&
(spyCall.expression.text === 'spyOn' || spyCall.expression.text === 'spyOnProperty')
) {
return transformBareSpyOn(spyCall, refactorCtx, false);
}
return spyCall;References
- Avoid placing a comma immediately after abbreviations like 'e.g.' in user-facing messages.
…or bare spyOn calls Bare `spyOn`/`spyOnProperty` calls in Jasmine create stubs that return `undefined` by default. Vitest's `vi.spyOn` passes through to the real implementation instead, changing observable behavior of migrated tests. Wrap bare spy calls with `.mockReturnValue(undefined)` to preserve the Jasmine semantics, except for `spyOnProperty(obj, prop, 'set')` where setter semantics already match.
0c85744 to
3164fc9
Compare
…or bare spyOn calls
Bare
spyOn/spyOnPropertycalls in Jasmine create stubs that returnundefinedby default. Vitest'svi.spyOnpasses through to the real implementation instead, changing observable behavior of migrated tests. Wrap bare spy calls with.mockReturnValue(undefined)to preserve the Jasmine semantics, except forspyOnProperty(obj, prop, 'set')where setter semantics already match.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?
The jasmine-to-vitest schematic transforms bare
spyOn(obj, 'method')andspyOnProperty(obj, 'prop')calls tovi.spyOn(obj, 'method')directly. However, Jasmine'sspyOncreates a stub that returnsundefinedby default (it does NOT call through to the real implementation), while Vitest'svi.spyOnpasses through to the real implementation by default. This means the migration changes the observable behavior of tests.Issue Number: #33253
What is the new behavior?
Bare
spyOn/spyOnPropertycalls (with no.and.*chain, or with.and.returnValues()and no arguments) are now wrapped with.mockReturnValue(undefined)to preserve Jasmine's stub-by-default semantics:Does this PR introduce a breaking change?
The output of the migration schematic changes - projects migrated with the old schematic that depended on bare spies passing through to real implementations will need re-migration, though this fix corrects the previous incorrect behavior that silently altered test semantics.
Other information