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

Author: crisbeto

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<T extends ts.Node>(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));
-  }
 }

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<T extends ts.Node>(
-    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);
-}

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`. */

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 }
+      });
+    `));
+  });
 });

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));