From 08caeadddb20fae33cb2604c9e791ef170e69f35 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 8 Oct 2021 10:44:24 +0200 Subject: [PATCH] fix(core): avoid duplicating comments in TestBed teardown migration (#43776) Currently the TestBed teardown migration is set up in a similar way to all other migrations where we take a `CallExpression`, add a parameter to it, print it, and replace the existing call. The problem is that doing so while preserving the `expression` of the original `CallExpression` can cause comments to be duplicated. This can happen quite frequently, because by default the CLI generates comments before `initTestModule` calls. To work around it, these changes make the migration more precise by inserting a new parameter or replacing and existing one using string manipulation. This requires a bit more code, but it's more reliable than the following alternatives: 1. Using `getFullStart` and `getFullWidth` to replace the node. This would work with our current setup, but the problem is that `getFullStart` also includes whitespace and newlines before the leading comment. This can cause us to mess up the user's formatting and figuring out which whitespace to keep and which one to remove is tricky. 2. Recreating the `CallExpression.expression` when constructing the new node. This would also work since it'll drop any existing comments, but the problem is that `CallExpression.expression` can be a wide variety of nodes which we would have to account for. We can't use `getMutableClone`, because it preserves the comments. Fixes #43739. PR Close #43776 --- .../migrations/google3/testbedTeardownRule.ts | 32 ++++++++-------- .../migrations/testbed-teardown/index.ts | 34 +++++++++-------- .../migrations/testbed-teardown/util.ts | 37 ++++++++++++++----- .../test/google3/testbed_teardown_spec.ts | 28 ++++++++++++++ .../schematics/test/testbed_teardown_spec.ts | 27 ++++++++++++++ 5 files changed, 117 insertions(+), 41 deletions(-) diff --git a/packages/core/schematics/migrations/google3/testbedTeardownRule.ts b/packages/core/schematics/migrations/google3/testbedTeardownRule.ts index b30da384d84..4277395da47 100644 --- a/packages/core/schematics/migrations/google3/testbedTeardownRule.ts +++ b/packages/core/schematics/migrations/google3/testbedTeardownRule.ts @@ -8,7 +8,7 @@ import {Replacement, RuleFailure, Rules} from 'tslint'; import ts from 'typescript'; -import {findInitTestEnvironmentCalls, findTestModuleMetadataNodes, InitTestEnvironmentAnalysis, migrateInitTestEnvironment, migrateTestModuleMetadataLiteral} from '../testbed-teardown/util'; +import {findInitTestEnvironmentCalls, findTestModuleMetadataNodes, getInitTestEnvironmentLiteralReplacement, InitTestEnvironmentAnalysis, migrateTestModuleMetadataLiteral} from '../testbed-teardown/util'; /** TSLint rule that adds the `teardown` flag to `TestBed` calls. */ export class Rule extends Rules.TypedRule { @@ -42,31 +42,31 @@ export class Rule extends Rules.TypedRule { if (initTestEnvironmentResult.totalCalls > 0) { // Migrate all of the unmigrated calls `initTestEnvironment` in this file. This could be zero // if the user has already opted into the new teardown behavior themselves. - initTestEnvironmentResult.callsToMigrate.forEach(call => { + initTestEnvironmentResult.callsToMigrate.forEach(node => { // This analysis is global so we need to check that the call is within this file. - if (call.getSourceFile() === sourceFile) { - failures.push(this._getFailure(call, migrateInitTestEnvironment, printer)); + if (node.getSourceFile() === sourceFile) { + const {span, text} = getInitTestEnvironmentLiteralReplacement(node, printer); + + failures.push(new RuleFailure( + sourceFile, span.start, span.end, 'Teardown behavior has to be configured.', + this.ruleName, new Replacement(span.start, span.length, text))); } }); } else { // Otherwise migrate the metadata passed into the `configureTestingModule` and `withModule` // calls. This scenario is less likely, but it could happen if `initTestEnvironment` has been // abstracted away or is inside a .js file. - findTestModuleMetadataNodes(typeChecker, sourceFile).forEach(literal => { - failures.push(this._getFailure(literal, migrateTestModuleMetadataLiteral, printer)); + findTestModuleMetadataNodes(typeChecker, sourceFile).forEach(node => { + const sourceFile = node.getSourceFile(); + const migrated = migrateTestModuleMetadataLiteral(node); + const replacementText = printer.printNode(ts.EmitHint.Unspecified, migrated, sourceFile); + + failures.push(new RuleFailure( + sourceFile, node.getStart(), node.getEnd(), 'Teardown behavior has to be configured.', + this.ruleName, new Replacement(node.getStart(), node.getWidth(), replacementText))); }); } return failures; } - - private _getFailure(node: T, migrator: (node: T) => T, printer: ts.Printer) { - const sourceFile = node.getSourceFile(); - const migrated = migrator(node); - const replacementText = printer.printNode(ts.EmitHint.Unspecified, migrated, sourceFile); - - return new RuleFailure( - sourceFile, node.getStart(), node.getEnd(), 'Teardown behavior has to be configured.', - this.ruleName, new Replacement(node.getStart(), node.getWidth(), replacementText)); - } } diff --git a/packages/core/schematics/migrations/testbed-teardown/index.ts b/packages/core/schematics/migrations/testbed-teardown/index.ts index 898ae1613ac..e741614e7b6 100644 --- a/packages/core/schematics/migrations/testbed-teardown/index.ts +++ b/packages/core/schematics/migrations/testbed-teardown/index.ts @@ -12,7 +12,7 @@ import ts from 'typescript'; import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; import {canMigrateFile, createMigrationProgram} from '../../utils/typescript/compiler_host'; -import {findInitTestEnvironmentCalls, findTestModuleMetadataNodes, migrateInitTestEnvironment, migrateTestModuleMetadataLiteral} from './util'; +import {findInitTestEnvironmentCalls, findTestModuleMetadataNodes, getInitTestEnvironmentLiteralReplacement, migrateTestModuleMetadataLiteral} from './util'; /** Migration that adds the `teardown` flag to `TestBed` calls. */ @@ -48,28 +48,30 @@ function runTestbedTeardownMigration(tree: Tree, tsconfigPath: string, basePath: if (initTestEnvironmentResult.totalCalls > 0) { // Migrate all of the unmigrated calls `initTestEnvironment`. This could be zero // if the user has already opted into the new teardown behavior themselves. - initTestEnvironmentResult.callsToMigrate.forEach(call => { - migrate(call, migrateInitTestEnvironment, tree, basePath, printer); + initTestEnvironmentResult.callsToMigrate.forEach(node => { + const {span, text} = getInitTestEnvironmentLiteralReplacement(node, printer); + const update = tree.beginUpdate(relative(basePath, node.getSourceFile().fileName)); + // The update appears to break if we try to call `remove` with a zero length. + if (span.length > 0) { + update.remove(span.start, span.length); + } + update.insertRight(span.start, text); + tree.commitUpdate(update); }); } else { // Otherwise migrate the metadata passed into the `configureTestingModule` and `withModule` // calls. This scenario is less likely, but it could happen if `initTestEnvironment` has been // abstracted away or is inside a .js file. sourceFiles.forEach(sourceFile => { - findTestModuleMetadataNodes(typeChecker, sourceFile).forEach(literal => { - migrate(literal, migrateTestModuleMetadataLiteral, tree, basePath, printer); + findTestModuleMetadataNodes(typeChecker, sourceFile).forEach(node => { + const migrated = migrateTestModuleMetadataLiteral(node); + const update = tree.beginUpdate(relative(basePath, node.getSourceFile().fileName)); + update.remove(node.getStart(), node.getWidth()); + update.insertRight( + node.getStart(), + printer.printNode(ts.EmitHint.Unspecified, migrated, node.getSourceFile())); + tree.commitUpdate(update); }); }); } } - - -function migrate( - node: T, migrator: (node: T) => T, tree: Tree, basePath: string, printer: ts.Printer) { - const migrated = migrator(node); - const update = tree.beginUpdate(relative(basePath, node.getSourceFile().fileName)); - update.remove(node.getStart(), node.getWidth()); - update.insertRight( - node.getStart(), printer.printNode(ts.EmitHint.Unspecified, migrated, node.getSourceFile())); - tree.commitUpdate(update); -} diff --git a/packages/core/schematics/migrations/testbed-teardown/util.ts b/packages/core/schematics/migrations/testbed-teardown/util.ts index 3a062afcb98..ac59492f07c 100644 --- a/packages/core/schematics/migrations/testbed-teardown/util.ts +++ b/packages/core/schematics/migrations/testbed-teardown/util.ts @@ -82,26 +82,45 @@ export function findTestModuleMetadataNodes( return sortInReverseSourceOrder(Array.from(testModuleMetadataLiterals)); } -/** Migrates a call to `TestBed.initTestEnvironment`. */ -export function migrateInitTestEnvironment(node: ts.CallExpression): ts.CallExpression { +/** + * Gets data that can be used to migrate a call to `TestBed.initTestEnvironment`. + * The returned `span` is used to mark the text that should be replaced while the `text` + * is the code that should be inserted instead. + */ +export function getInitTestEnvironmentLiteralReplacement( + node: ts.CallExpression, printer: ts.Printer) { const literalProperties: ts.ObjectLiteralElementLike[] = []; + const lastArg = node.arguments[node.arguments.length - 1]; + let span: {start: number, end: number, length: number}; + let prefix: string; if (node.arguments.length > 2) { - if (isFunction(node.arguments[2])) { + if (isFunction(lastArg)) { // If the last argument is a function, add the function as the `aotSummaries` property. - literalProperties.push(ts.createPropertyAssignment('aotSummaries', node.arguments[2])); - } else if (ts.isObjectLiteralExpression(node.arguments[2])) { + literalProperties.push(ts.createPropertyAssignment('aotSummaries', lastArg)); + } else if (ts.isObjectLiteralExpression(lastArg)) { // If the property is an object literal, copy over all the properties. - literalProperties.push(...node.arguments[2].properties); + literalProperties.push(...lastArg.properties); } + + prefix = ''; + span = {start: lastArg.getStart(), end: lastArg.getEnd(), length: lastArg.getWidth()}; + } else { + const start = lastArg.getEnd(); + prefix = ', '; + span = {start, end: start, length: 0}; } // Finally push the teardown object so that it appears last. literalProperties.push(createTeardownAssignment()); - return ts.createCall( - node.expression, node.typeArguments, - [...node.arguments.slice(0, 2), ts.createObjectLiteral(literalProperties, true)]); + return { + span, + text: prefix + + printer.printNode( + ts.EmitHint.Unspecified, ts.createObjectLiteral(literalProperties, true), + node.getSourceFile()) + }; } /** Migrates an object literal that is passed into `configureTestingModule` or `withModule`. */ diff --git a/packages/core/schematics/test/google3/testbed_teardown_spec.ts b/packages/core/schematics/test/google3/testbed_teardown_spec.ts index 79d45f84b75..876efece807 100644 --- a/packages/core/schematics/test/google3/testbed_teardown_spec.ts +++ b/packages/core/schematics/test/google3/testbed_teardown_spec.ts @@ -704,4 +704,32 @@ describe('Google3 TestBed teardown TSLint rule', () => { })); `)); }); + + it('should not duplicate comments on initTestEnvironment calls', () => { + writeFile('/index.ts', ` + import { TestBed } from '@angular/core/testing'; + import { + BrowserDynamicTestingModule, + platformBrowserDynamicTesting + } from '@angular/platform-browser-dynamic/testing'; + + // Hello + TestBed.initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting()); + `); + + runTSLint(true); + + expect(stripWhitespace(getFile('/index.ts'))).toContain(stripWhitespace(` + import { TestBed } from '@angular/core/testing'; + import { + BrowserDynamicTestingModule, + platformBrowserDynamicTesting + } from '@angular/platform-browser-dynamic/testing'; + + // Hello + TestBed.initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), { + teardown: { destroyAfterEach: false } + }); + `)); + }); }); diff --git a/packages/core/schematics/test/testbed_teardown_spec.ts b/packages/core/schematics/test/testbed_teardown_spec.ts index 4448fcc2581..7048aceef2a 100644 --- a/packages/core/schematics/test/testbed_teardown_spec.ts +++ b/packages/core/schematics/test/testbed_teardown_spec.ts @@ -649,6 +649,33 @@ describe('TestBed teardown migration', () => { `)); }); + it('should not duplicate comments on initTestEnvironment calls', async () => { + writeFile('/index.ts', ` + import { TestBed } from '@angular/core/testing'; + import { + BrowserDynamicTestingModule, + platformBrowserDynamicTesting + } from '@angular/platform-browser-dynamic/testing'; + + // Hello + TestBed.initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting()); + `); + + await runMigration(); + + expect(stripWhitespace(tree.readContent('/index.ts'))).toContain(stripWhitespace(` + import { TestBed } from '@angular/core/testing'; + import { + BrowserDynamicTestingModule, + platformBrowserDynamicTesting + } from '@angular/platform-browser-dynamic/testing'; + + // Hello + TestBed.initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting(), { + teardown: { destroyAfterEach: false } + }); + `)); + }); function writeFile(filePath: string, contents: string) { host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents));