Skip to content

Commit b8c99aa

Browse files
committed
fix(@schematics/angular): improve safety of done callback transformation
This commit adds a safety check to the `done` callback transformation in the `jasmine-vitest` schematic. Previously, if a `done` callback was used in an unhandled way (e.g., passed as an argument to a helper function), the schematic would remove the `done` parameter but leave the usage, causing runtime errors. Now, the transformer counts the total usages of the `done` callback and compares it to the number of usages it successfully transformed. If there are unhandled usages, it aborts the transformation for that specific test and adds a TODO comment, ensuring that no broken code is generated. (cherry picked from commit 2b99ce5)
1 parent c973bb9 commit b8c99aa

File tree

3 files changed

+53
-0
lines changed

3 files changed

+53
-0
lines changed

packages/schematics/angular/refactor/jasmine-vitest/transformers/jasmine-lifecycle.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,19 @@ function transformPromiseBasedDone(
282282
return undefined;
283283
}
284284

285+
function countDoneUsages(node: ts.Node, doneIdentifier: ts.Identifier): number {
286+
let count = 0;
287+
const visitor = (n: ts.Node) => {
288+
if (ts.isIdentifier(n) && n.text === doneIdentifier.text) {
289+
count++;
290+
}
291+
ts.forEachChild(n, visitor);
292+
};
293+
ts.forEachChild(node, visitor);
294+
295+
return count;
296+
}
297+
285298
export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContext): ts.Node {
286299
const { sourceFile, reporter, tsContext } = refactorCtx;
287300
if (
@@ -309,12 +322,17 @@ export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContex
309322
return node;
310323
}
311324
const doneIdentifier = doneParam.name;
325+
326+
// Count total usages of 'done' in the body
327+
const totalUsages = countDoneUsages(functionArg.body, doneIdentifier);
328+
let handledUsages = 0;
312329
let doneWasUsed = false;
313330

314331
const bodyVisitor = (bodyNode: ts.Node): ts.Node | ts.Node[] | undefined => {
315332
const complexTransformed = transformComplexDoneCallback(bodyNode, doneIdentifier, refactorCtx);
316333
if (complexTransformed !== bodyNode) {
317334
doneWasUsed = true;
335+
handledUsages++; // complex transform handles one usage
318336

319337
return complexTransformed;
320338
}
@@ -330,6 +348,7 @@ export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContex
330348
callExpr.expression.name.text === 'fail'
331349
) {
332350
doneWasUsed = true;
351+
handledUsages++;
333352
reporter.reportTransformation(
334353
sourceFile,
335354
bodyNode,
@@ -350,6 +369,7 @@ export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContex
350369
const promiseTransformed = transformPromiseBasedDone(callExpr, doneIdentifier, refactorCtx);
351370
if (promiseTransformed) {
352371
doneWasUsed = true;
372+
handledUsages++;
353373

354374
return promiseTransformed;
355375
}
@@ -360,6 +380,7 @@ export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContex
360380
callExpr.expression.text === doneIdentifier.text
361381
) {
362382
doneWasUsed = true;
383+
handledUsages++;
363384

364385
return ts.setTextRange(ts.factory.createEmptyStatement(), callExpr.expression);
365386
}
@@ -383,6 +404,20 @@ export function transformDoneCallback(node: ts.Node, refactorCtx: RefactorContex
383404
return bodyVisitor(node);
384405
});
385406

407+
// Safety check: if we found usages but didn't handle all of them, abort.
408+
if (handledUsages < totalUsages) {
409+
reporter.reportTransformation(
410+
sourceFile,
411+
node,
412+
`Found unhandled usage of \`${doneIdentifier.text}\` callback. Skipping transformation.`,
413+
);
414+
const category = 'unhandled-done-usage';
415+
reporter.recordTodo(category);
416+
addTodoComment(node, category);
417+
418+
return node;
419+
}
420+
386421
if (!doneWasUsed) {
387422
return node;
388423
}

packages/schematics/angular/refactor/jasmine-vitest/transformers/jasmine-lifecycle_spec.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ describe('Jasmine to Vitest Transformer', () => {
132132
});
133133
`,
134134
expected: `
135+
// TODO: vitest-migration: The 'done' callback was used in an unhandled way. Please migrate manually.
135136
it('should not transform a function with a parameter that is not a done callback', (value) => {
136137
expect(value).toBe(true);
137138
});
@@ -157,6 +158,20 @@ describe('Jasmine to Vitest Transformer', () => {
157158
});
158159
`,
159160
},
161+
{
162+
description: 'should add a TODO for unhandled done usage',
163+
input: `
164+
it('should do something with helper', (done) => {
165+
someHelper(done);
166+
});
167+
`,
168+
expected: `
169+
// TODO: vitest-migration: The 'done' callback was used in an unhandled way. Please migrate manually.
170+
it('should do something with helper', (done) => {
171+
someHelper(done);
172+
});
173+
`,
174+
},
160175
];
161176

162177
testCases.forEach(({ description, input, expected }) => {

packages/schematics/angular/refactor/jasmine-vitest/utils/todo-notes.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,9 @@ export const TODO_NOTES = {
120120
' Please refactor to access .args directly or use vi.mocked(spy).mock.lastCall.',
121121
url: 'https://vitest.dev/api/mocked.html#mock-lastcall',
122122
},
123+
'unhandled-done-usage': {
124+
message: "The 'done' callback was used in an unhandled way. Please migrate manually.",
125+
},
123126
} as const;
124127

125128
/**

0 commit comments

Comments
 (0)