Skip to content

Commit b8b6ed8

Browse files
baerjaysoo
andauthored
fix(testing): use surgical text replacement in Jest matcher alias migration (#34350)
## Current Behavior The `replace-removed-matcher-aliases` migration uses `tsquery.replace()` which reprints the entire AST through TypeScript's Printer. This causes two problems: 1. **Syntax corruption**: Valid TypeScript files are mangled: - Destructuring patterns: `{ result }` becomes `{result}:` - Arrow functions: missing opening braces - Nested callbacks: collapsed/merged code blocks 2. **Unnecessary file changes**: Every test file is written back to disk even when no matchers are replaced. This triggers `formatFiles()` to reformat unchanged files, creating large whitespace-only diffs. In large codebases, this can result in hundreds or thousands of files being modified unnecessarily, making the migration PR difficult to review. **Why I care a Lot** I was running this on a multi-million-LOC monorepo and ran into two issues: * I got ~10k modified files with whitespace-only changes from the removal of newlines. These changes couldn't be fixed with Prettier because it didn't care about the number of newlines, so the diff was unmergeable. * I got ~8 files with malformed Typescript, causing commit hooks, CI, etc. to fail without manual intervention. ## Expected Behavior The migration should: 1. Only replace the deprecated matcher names (e.g., `toBeCalled` → `toHaveBeenCalled`) 2. Preserve all surrounding code exactly as written 5. Only touch files that actually contain deprecated matchers ## Solution Replace AST-reprinting with surgical text replacement: - Use `tsquery.query()` to find matching AST nodes - Collect text positions (start/end) for each node to replace - Apply replacements in reverse order using string slicing - Only write files that actually changed This pattern is already used successfully in other Nx migrations (e.g., `rename-cy-exec-code-property.ts` in the Cypress package). **Additional improvements:** - Single AST parse with regex selector vs. 11 separate passes - Quick string check skips parsing files without deprecated matchers - New regression test covers complex patterns that triggered corruption ## Related Issue(s) Fixes #32062 --------- Co-authored-by: Jack Hsu <jack.hsu@gmail.com>
1 parent bdc61b5 commit b8b6ed8

File tree

2 files changed

+181
-9
lines changed

2 files changed

+181
-9
lines changed

packages/jest/src/migrations/update-21-3-0/replace-removed-matcher-aliases.spec.ts

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,135 @@ describe('test', () => {
193193
`);
194194
});
195195

196+
it('should preserve complex code patterns that could be corrupted by AST reprinting', async () => {
197+
// This test covers patterns that were corrupted by the old implementation which used
198+
// tsquery.replace() that reprints the entire AST using TypeScript's Printer.
199+
// The patterns include: destructuring, arrow functions, nested callbacks, and object literals.
200+
writeFile(
201+
tree,
202+
'apps/app1/jest.config.js',
203+
`module.exports = {};
204+
`
205+
);
206+
const complexCode = `interface Config<T> {
207+
buildVariables: (p: { payGroupId: string; value: T }) => unknown;
208+
initialValue: number;
209+
}
210+
211+
const mockAutoSave = jest.fn();
212+
213+
function useAutoSave<T>(config: Config<T>) {
214+
return { autoSave: mockAutoSave, lastAttemptedValue: config.initialValue };
215+
}
216+
217+
describe('useAutoSave', () => {
218+
beforeEach(() => {
219+
mockAutoSave.mockClear();
220+
});
221+
222+
it('should handle complex callback patterns', async () => {
223+
const { result } = useAutoSave({
224+
buildVariables: ({ payGroupId, value }) => ({ payGroupId, value }),
225+
initialValue: 0,
226+
});
227+
228+
expect(mockAutoSave).not.toBeCalled();
229+
230+
mockAutoSave.mockImplementation(async (value: number) => {
231+
return { autoSave: mockAutoSave, lastAttemptedValue: value };
232+
});
233+
234+
await result.autoSave(42);
235+
expect(mockAutoSave).toBeCalledWith(42);
236+
expect(mockAutoSave).toBeCalledTimes(1);
237+
});
238+
239+
it('should work with nested arrow functions', () => {
240+
const config = {
241+
buildVariables: (p: { id: string }) => p,
242+
initialValue: 0,
243+
};
244+
245+
const { result } = useAutoSave(config);
246+
247+
expect(mockAutoSave).not.toBeCalled();
248+
});
249+
});
250+
`;
251+
252+
writeFile(tree, 'apps/app1/src/useAutoSave.spec.ts', complexCode);
253+
254+
await migration(tree);
255+
256+
const result = tree.read('apps/app1/src/useAutoSave.spec.ts', 'utf-8');
257+
258+
// Verify the output is valid TypeScript by checking key patterns weren't corrupted
259+
// The old buggy implementation would produce patterns like:
260+
// - "{buildVariables}: (p:" instead of "{ buildVariables: (p:"
261+
// - Missing opening braces after arrow functions
262+
// - Collapsed/merged code blocks
263+
// expect(result).not.toContain('toBeCalled');
264+
expect(result).not.toContain('toBeCalledWith');
265+
expect(result).not.toContain('toBeCalledTimes');
266+
expect(result).toContain('{ payGroupId, value }');
267+
expect(result).toContain('{ result }');
268+
expect(result).not.toMatch(/\{payGroupId\}:/);
269+
expect(result).not.toMatch(/\{result\}:/);
270+
271+
// Verify interface syntax is preserved
272+
expect(result).toContain('{ payGroupId: string; value: T }');
273+
expect(result).not.toMatch(/\{payGroupId\}: string/);
274+
275+
// Snapshot test to verify the overall structure and formatting is preserved
276+
expect(result).toMatchInlineSnapshot(`
277+
"interface Config<T> {
278+
buildVariables: (p: { payGroupId: string; value: T }) => unknown;
279+
initialValue: number;
280+
}
281+
282+
const mockAutoSave = jest.fn();
283+
284+
function useAutoSave<T>(config: Config<T>) {
285+
return { autoSave: mockAutoSave, lastAttemptedValue: config.initialValue };
286+
}
287+
288+
describe('useAutoSave', () => {
289+
beforeEach(() => {
290+
mockAutoSave.mockClear();
291+
});
292+
293+
it('should handle complex callback patterns', async () => {
294+
const { result } = useAutoSave({
295+
buildVariables: ({ payGroupId, value }) => ({ payGroupId, value }),
296+
initialValue: 0,
297+
});
298+
299+
expect(mockAutoSave).not.toHaveBeenCalled();
300+
301+
mockAutoSave.mockImplementation(async (value: number) => {
302+
return { autoSave: mockAutoSave, lastAttemptedValue: value };
303+
});
304+
305+
await result.autoSave(42);
306+
expect(mockAutoSave).toHaveBeenCalledWith(42);
307+
expect(mockAutoSave).toHaveBeenCalledTimes(1);
308+
});
309+
310+
it('should work with nested arrow functions', () => {
311+
const config = {
312+
buildVariables: (p: { id: string }) => p,
313+
initialValue: 0,
314+
};
315+
316+
const { result } = useAutoSave(config);
317+
318+
expect(mockAutoSave).not.toHaveBeenCalled();
319+
});
320+
});
321+
"
322+
`);
323+
});
324+
196325
function writeFile(tree: Tree, path: string, content: string): void {
197326
tree.write(path, content);
198327
fs.createFileSync(path, content);

packages/jest/src/migrations/update-21-3-0/replace-removed-matcher-aliases.ts

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { formatFiles, globAsync, type Tree } from '@nx/devkit';
2-
import { replace } from '@phenomnomnominal/tsquery';
2+
import { ast, query } from '@phenomnomnominal/tsquery';
33
import { SearchSource } from 'jest';
44
import { readConfig } from 'jest-config';
55
import Runtime from 'jest-runtime';
@@ -24,20 +24,63 @@ const matcherAliasesMap = new Map<string, string>([
2424
export default async function (tree: Tree) {
2525
const testFilePaths = await getTestFilePaths(tree);
2626
for (const testFilePath of testFilePaths) {
27-
let testFileContent = tree.read(testFilePath, 'utf-8');
28-
for (const [alias, matcher] of matcherAliasesMap) {
29-
testFileContent = replace(
30-
testFileContent,
31-
`CallExpression PropertyAccessExpression:has(CallExpression Identifier[name=expect]) Identifier[name=${alias}]`,
32-
(_node: Identifier) => matcher
33-
);
27+
const testFileContent = tree.read(testFilePath, 'utf-8');
28+
const updatedContent = replaceMatcherAliases(testFileContent);
29+
if (updatedContent !== testFileContent) {
30+
tree.write(testFilePath, updatedContent);
3431
}
35-
tree.write(testFilePath, testFileContent);
3632
}
3733

3834
await formatFiles(tree);
3935
}
4036

37+
function replaceMatcherAliases(fileContent: string): string {
38+
// Build a selector that matches any of the deprecated matcher aliases
39+
const aliasNames = Array.from(matcherAliasesMap.keys());
40+
const aliasPattern = aliasNames.join('|');
41+
42+
// Quick check to avoid parsing files that don't contain any aliases
43+
const hasAnyAlias = aliasNames.some((alias) => fileContent.includes(alias));
44+
if (!hasAnyAlias) {
45+
return fileContent;
46+
}
47+
48+
const sourceFile = ast(fileContent);
49+
const updates: Array<{ start: number; end: number; text: string }> = [];
50+
51+
// Query for all deprecated matcher identifiers in expect() chains
52+
// The selector matches: expect(...).toBeCalled(), expect(...).not.toBeCalled(), etc.
53+
const selector = `CallExpression PropertyAccessExpression:has(CallExpression Identifier[name=expect]) Identifier[name=/^(${aliasPattern})$/]`;
54+
const matchedNodes = query<Identifier>(sourceFile, selector);
55+
56+
for (const node of matchedNodes) {
57+
const alias = node.text;
58+
const replacement = matcherAliasesMap.get(alias);
59+
if (replacement) {
60+
updates.push({
61+
start: node.getStart(sourceFile),
62+
end: node.getEnd(),
63+
text: replacement,
64+
});
65+
}
66+
}
67+
68+
if (!updates.length) {
69+
return fileContent;
70+
}
71+
72+
// Apply updates in reverse order to preserve positions
73+
let updatedContent = fileContent;
74+
for (const update of updates.sort((a, b) => b.start - a.start)) {
75+
updatedContent =
76+
updatedContent.slice(0, update.start) +
77+
update.text +
78+
updatedContent.slice(update.end);
79+
}
80+
81+
return updatedContent;
82+
}
83+
4184
async function getTestFilePaths(tree: Tree): Promise<string[]> {
4285
const jestConfigFiles = await globAsync(tree, [
4386
'**/jest.config.{cjs,mjs,js,cts,mts,ts}',

0 commit comments

Comments
 (0)