Skip to content

fix(@schematics/angular): preserve Jasmine stub-by-default behavior f… #33263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spliffone wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(@schematics/angular): preserve Jasmine stub-by-default behavior f… #33263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions packages/schematics/angular/refactor/jasmine-vitest/index_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe('Jasmine to Vitest Schematic', () => {
);

const newContent = tree.readContent(specFilePath);
expect(newContent).toContain(`vi.spyOn(service, 'myMethod');`);
expect(newContent).toContain(`vi.spyOn(service, 'myMethod').mockReturnValue(undefined);`);
});

it('should only transform files matching the fileSuffix option', async () => {
Expand Down Expand Up @@ -94,7 +94,7 @@ describe('Jasmine to Vitest Schematic', () => {
expect(unchangedContent).not.toContain(`vi.spyOn(window, 'alert');`);

const changedContent = tree.readContent(testFilePath);
expect(changedContent).toContain(`vi.spyOn(window, 'confirm');`);
expect(changedContent).toContain(`vi.spyOn(window, 'confirm').mockReturnValue(undefined);`);
});

it('should print verbose logs when the verbose option is true', async () => {
Expand All @@ -120,7 +120,11 @@ describe('Jasmine to Vitest Schematic', () => {

expect(logs).toContain('Detailed Transformation Log:');
expect(logs).toContain(`Processing: /${specFilePath}`);
expect(logs.some((log) => log.includes('Transformed `spyOn` to `vi.spyOn`'))).toBe(true);
expect(
logs.some((log) =>
log.includes('Transformed `spyOn` to `vi.spyOn(...).mockReturnValue(undefined)`'),
),
).toBe(true);
});

describe('with `include` option', () => {
Expand All @@ -144,7 +148,7 @@ describe('Jasmine to Vitest Schematic', () => {
);

const changedContent = tree.readContent('projects/bar/src/app/nested/nested.spec.ts');
expect(changedContent).toContain(`vi.spyOn(window, 'confirm');`);
expect(changedContent).toContain(`vi.spyOn(window, 'confirm').mockReturnValue(undefined);`);

const unchangedContent = tree.readContent('projects/bar/src/app/app.spec.ts');
expect(unchangedContent).toContain(`spyOn(window, 'alert');`);
Expand All @@ -158,7 +162,7 @@ describe('Jasmine to Vitest Schematic', () => {
);

const changedContent = tree.readContent('projects/bar/src/app/nested/nested.spec.ts');
expect(changedContent).toContain(`vi.spyOn(window, 'confirm');`);
expect(changedContent).toContain(`vi.spyOn(window, 'confirm').mockReturnValue(undefined);`);

const unchangedContent = tree.readContent('projects/bar/src/app/app.spec.ts');
expect(unchangedContent).toContain(`spyOn(window, 'alert');`);
Expand All @@ -177,10 +181,12 @@ describe('Jasmine to Vitest Schematic', () => {
);

const changedAppContent = tree.readContent('projects/bar/src/app/app.spec.ts');
expect(changedAppContent).toContain(`vi.spyOn(window, 'alert');`);
expect(changedAppContent).toContain(`vi.spyOn(window, 'alert').mockReturnValue(undefined);`);

const changedNestedContent = tree.readContent('projects/bar/src/app/nested/nested.spec.ts');
expect(changedNestedContent).toContain(`vi.spyOn(window, 'confirm');`);
expect(changedNestedContent).toContain(
`vi.spyOn(window, 'confirm').mockReturnValue(undefined);`,
);

const unchangedContent = tree.readContent('projects/bar/src/other/other.spec.ts');
expect(unchangedContent).toContain(`spyOn(window, 'close');`);
Expand All @@ -194,10 +200,12 @@ describe('Jasmine to Vitest Schematic', () => {
);

const changedAppContent = tree.readContent('projects/bar/src/app/app.spec.ts');
expect(changedAppContent).toContain(`vi.spyOn(window, 'alert');`);
expect(changedAppContent).toContain(`vi.spyOn(window, 'alert').mockReturnValue(undefined);`);

const changedNestedContent = tree.readContent('projects/bar/src/app/nested/nested.spec.ts');
expect(changedNestedContent).toContain(`vi.spyOn(window, 'confirm');`);
expect(changedNestedContent).toContain(
`vi.spyOn(window, 'confirm').mockReturnValue(undefined);`,
);
});

it('should throw if the include path does not exist', async () => {
Expand Down
2 changes: 1 addition & 1 deletion ...ages/schematics/angular/refactor/jasmine-vitest/test-file-transformer.integration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ describe('Jasmine to Vitest Transformer - Integration Tests', () => {
});
it('should handle user click', () => {
vi.spyOn(window, 'alert');
vi.spyOn(window, 'alert').mockReturnValue(undefined);
const button = fixture.nativeElement.querySelector('button');
button.click();
fixture.detectChanges();
Expand Down
6 changes: 3 additions & 3 deletions ...ages/schematics/angular/refactor/jasmine-vitest/test-file-transformer_add-imports_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('Jasmine to Vitest Transformer - addImports option', () => {
const input = `spyOn(foo, 'bar');`;
const expected = `
import { vi } from 'vitest';
vi.spyOn(foo, 'bar');
vi.spyOn(foo, 'bar').mockReturnValue(undefined);
`;
await expectTransformation(input, expected, true);
});
Expand All @@ -27,7 +27,7 @@ describe('Jasmine to Vitest Transformer - addImports option', () => {
import { type Mock, vi } from 'vitest';

let mySpy: Mock;
vi.spyOn(foo, 'bar');
vi.spyOn(foo, 'bar').mockReturnValue(undefined);
`;
await expectTransformation(input, expected, true);
});
Expand All @@ -41,7 +41,7 @@ describe('Jasmine to Vitest Transformer - addImports option', () => {
import type { Mock } from 'vitest';

let mySpy: Mock;
vi.spyOn(foo, 'bar');
vi.spyOn(foo, 'bar').mockReturnValue(undefined);
`;
await expectTransformation(input, expected, false);
});
Expand Down
108 changes: 92 additions & 16 deletions packages/schematics/angular/refactor/jasmine-vitest/transformers/jasmine-spy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,7 @@ export function transformSpies(node: ts.Node, refactorCtx: RefactorContext): ts.
ts.isIdentifier(node.expression) &&
(node.expression.text === 'spyOn' || node.expression.text === 'spyOnProperty')
) {
addVitestValueImport(pendingVitestValueImports, 'vi');
reporter.reportTransformation(
sourceFile,
node,
`Transformed \`${node.expression.text}\` to \`vi.spyOn\`.`,
);

return ts.factory.updateCallExpression(
node,
createPropertyAccess('vi', 'spyOn'),
node.typeArguments,
node.arguments,
);
return transformSpyExpression(node, refactorCtx);
}

if (ts.isPropertyAccessExpression(node.expression)) {
Expand Down Expand Up @@ -93,8 +81,8 @@ export function transformSpies(node: ts.Node, refactorCtx: RefactorContext): ts.
);
const returnValues = node.arguments;
if (returnValues.length === 0) {
// No values, so it's a no-op. Just transform the spyOn call.
return transformSpies(spyCall, refactorCtx);
// No values, preserve Jasmine's stub-by-default behavior.
return transformSpyExpression(spyCall, refactorCtx, true);
}
Comment thread
spliffone marked this conversation as resolved.
// spy.and.returnValues(a, b) -> spy.mockReturnValueOnce(a).mockReturnValueOnce(b)
let chainedCall: ts.Expression = spyCall;
Expand All @@ -119,7 +107,8 @@ export function transformSpies(node: ts.Node, refactorCtx: RefactorContext): ts.
'Removed redundant `.and.callThrough()` call.',
);

return transformSpies(spyCall, refactorCtx); // .and.callThrough() is redundant, just transform spyOn.
// .and.callThrough() is redundant, just transform spyOn.
return transformSpyExpression(spyCall, refactorCtx, false);
case 'stub': {
reporter.reportTransformation(
sourceFile,
Expand Down Expand Up @@ -219,6 +208,67 @@ export function transformSpies(node: ts.Node, refactorCtx: RefactorContext): ts.
return node;
}

function transformSpyExpression(
node: ts.Expression,
refactorCtx: RefactorContext,
forceStubBehavior?: boolean,
): ts.Expression {
const { sourceFile, reporter, pendingVitestValueImports } = refactorCtx;

if (
ts.isCallExpression(node) &&
ts.isIdentifier(node.expression) &&
(node.expression.text === 'spyOn' || node.expression.text === 'spyOnProperty')
) {
const spyFnName = node.expression.text;
addVitestValueImport(pendingVitestValueImports, 'vi');

const viSpyOnCall = ts.factory.updateCallExpression(
node,
createPropertyAccess('vi', 'spyOn'),
node.typeArguments,
node.arguments,
);

const shouldStub = forceStubBehavior ?? shouldStubBareSpyOn(node);

if (shouldStub) {
reporter.reportTransformation(
sourceFile,
node,
`Transformed \`${spyFnName}\` to \`vi.spyOn(...).mockReturnValue(undefined)\` ` +
`to preserve Jasmine's stub-by-default behavior.`,
);

return ts.factory.createCallExpression(
createPropertyAccess(viSpyOnCall, 'mockReturnValue'),
undefined,
[ts.factory.createIdentifier('undefined')],
);
}

reporter.reportTransformation(
sourceFile,
node,
`Transformed \`${spyFnName}\` to \`vi.spyOn\`.`,
);

return viSpyOnCall;
}

// Variable reference (e.g. `spy` from `spy.and.returnValues()`).
// There is no spyOn call to transform — only stub-wrapping is relevant here.
if (forceStubBehavior) {
return ts.factory.createCallExpression(
createPropertyAccess(node, 'mockReturnValue'),
undefined,
[ts.factory.createIdentifier('undefined')],
);
}

return node;
}

export function transformCreateSpy(node: ts.Node, ctx: RefactorContext): ts.Node {
const { reporter, sourceFile, pendingVitestValueImports } = ctx;
if (!isJasmineCallExpression(node, 'createSpy')) {
Expand Down Expand Up @@ -395,6 +445,32 @@ export function transformSpyReset(
return node;
}

function shouldStubBareSpyOn(node: ts.CallExpression): boolean {
if (
ts.isIdentifier(node.expression) &&
node.expression.text === 'spyOnProperty' &&
node.arguments.length >= 3
) {
const accessType = node.arguments[2];
if (ts.isStringLiteralLike(accessType) && accessType.text === 'set') {
return false;
}
}

const parent = node.parent;
if (
parent &&
ts.isPropertyAccessExpression(parent) &&
parent.expression === node &&
ts.isIdentifier(parent.name) &&
parent.name.text === 'and'
) {
return false;
}
Comment thread
spliffone marked this conversation as resolved.

return true;
}

function getSpyIdentifierFromCalls(node: ts.PropertyAccessExpression): ts.Expression | undefined {
if (ts.isIdentifier(node.name) && node.name.text === 'calls') {
return node.expression;
Expand Down
56 changes: 50 additions & 6 deletions packages/schematics/angular/refactor/jasmine-vitest/transformers/jasmine-spy_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ import { expectTransformation } from '../test-helpers';
describe('Jasmine to Vitest Transformer - transformSpies', () => {
const testCases = [
{
description: 'should transform spyOn(object, "method") to vi.spyOn(object, "method")',
description:
'should transform bare spyOn to vi.spyOn(...).mockReturnValue(undefined) to preserve Jasmine stub-by-default semantics',
input: `spyOn(service, 'myMethod');`,
expected: `vi.spyOn(service, 'myMethod');`,
expected: `vi.spyOn(service, 'myMethod').mockReturnValue(undefined);`,
},
{
description: 'should transform .and.returnValue(...) to .mockReturnValue(...)',
Expand Down Expand Up @@ -58,9 +59,10 @@ describe('Jasmine to Vitest Transformer - transformSpies', () => {
expected: `const mySpy = vi.fn(() => 'foo').mockName('mySpy');`,
},
{
description: 'should transform spyOnProperty(object, "prop") to vi.spyOn(object, "prop")',
description:
'should transform bare spyOnProperty to vi.spyOn(...).mockReturnValue(undefined) to preserve Jasmine stub-by-default semantics',
input: `spyOnProperty(service, 'myProp');`,
expected: `vi.spyOn(service, 'myProp');`,
expected: `vi.spyOn(service, 'myProp').mockReturnValue(undefined);`,
},
{
description: 'should transform .and.stub() to .mockImplementation(() => {})',
Expand All @@ -80,9 +82,11 @@ describe('Jasmine to Vitest Transformer - transformSpies', () => {
expected: `const mySpy = vi.fn().mockName('mySpy').mockReturnValue(true);`,
},
{
description: 'should handle .and.returnValues() with no arguments',
description:
'should transform .and.returnValues() with no args to vi.spyOn(...).mockReturnValue(undefined) ' +
'because Jasmine reverts to stub-by-default',
input: `spyOn(service, 'myMethod').and.returnValues();`,
expected: `vi.spyOn(service, 'myMethod');`,
expected: `vi.spyOn(service, 'myMethod').mockReturnValue(undefined);`,
},
{
description:
Expand Down Expand Up @@ -117,6 +121,46 @@ describe('Jasmine to Vitest Transformer - transformSpies', () => {
expected: `// TODO: vitest-migration: Unsupported spy strategy ".and.unknownStrategy()" found. Please migrate this manually. See: https://vitest.dev/api/mocked.html#mock
vi.spyOn(service, 'myMethod').and.unknownStrategy();`,
},
{
description: 'should preserve stub-by-default semantics for spyOn assigned to a variable',
input: `const spy = spyOn(service, 'myMethod');`,
expected: `const spy = vi.spyOn(service, 'myMethod').mockReturnValue(undefined);`,
},
{
description:
'should wrap spyOnProperty with explicit "get" access type to preserve stub-by-default semantics',
input: `spyOnProperty(service, 'myProp', 'get');`,
expected: `vi.spyOn(service, 'myProp', 'get').mockReturnValue(undefined);`,
},
{
description:
'should NOT wrap spyOnProperty with "set" access type because setter semantics already match',
input: `spyOnProperty(service, 'myProp', 'set');`,
expected: `vi.spyOn(service, 'myProp', 'set');`,
},
{
description:
'should wrap spyOn used as an expression argument to preserve stub-by-default semantics',
input: `expect(spyOn(service, 'myMethod')).toBeDefined();`,
expected: `expect(vi.spyOn(service, 'myMethod').mockReturnValue(undefined)).toBeDefined();`,
},
Comment thread
spliffone marked this conversation as resolved.
{
description:
'should wrap a spy variable with .and.returnValues() with no args to preserve stub-by-default semantics',
input: `spy.and.returnValues();`,
expected: `spy.mockReturnValue(undefined);`,
},
{
description: 'should remove .and.callThrough() on a spy variable',
input: `spy.and.callThrough();`,
expected: `spy;`,
},
{
description:
'should preserve stub-by-default on spyOn with .calls.reset() chained (not an .and accessor)',
input: `spyOn(service, 'myMethod').calls.reset();`,
expected: `vi.spyOn(service, 'myMethod').mockReturnValue(undefined).mockClear();`,
},
];

testCases.forEach(({ description, input, expected }) => {
Expand Down
Loading