feat(range): remove Range from the API (#2882)

Remove `Mutant.range` from the plugin API. Plugin creators should rely on `location` instead (which has `start` and `end` positions). See changes in the TypeScript checker for an example. The `range` property was a way to identify changed code. It was a tuple defined as `[start: number, end: number]`. As plugin creator, you had the opportunity to use this instead of `location`. We've chosen to remove it because having 2 ways to identify location is redundant and this will allow us to implement #322 in the future. BREAKING CHANGE: The `range` property is no longer present on a `mutant`. Note, this is a breaking change for plugin creators only. Co-authored-by: Simon de Lang <[email protected]>
stryker-mutator · May 11, 2021 · b578b22 · b578b22
1 parent 9936c91
commit b578b22
Show file tree

Hide file tree

Showing 16 changed files with 84 additions and 71 deletions.
diff --git a/packages/api/src/core/index.ts b/packages/api/src/core/index.ts
@@ -1,7 +1,6 @@
 export { File } from './file';
 export { Position } from './position';
 export { Location } from './location';
-export { Range } from './range';
 export * from './mutant';
 export * from '../../src-generated/stryker-core';
 export * from './report-types';

diff --git a/packages/api/src/core/mutant.ts b/packages/api/src/core/mutant.ts
@@ -1,7 +1,5 @@
 import * as schema from 'mutation-testing-report-schema';
 
-import { Range } from './range';
-
 export { MutantStatus } from 'mutation-testing-report-schema';
 
 // We're reusing the `MutantResult` interface here to acquire uniformity.
@@ -14,11 +12,6 @@ export interface Mutant extends Pick<schema.MutantResult, 'id' | 'location' | 'm
    * The file name from which this mutant originated
    */
   fileName: string;
-  /**
-   * The range of this mutant (from/to within the file)
-   * deprecate?
-   */
-  range: Range;
   /**
    * Actual mutation that has been applied.
    */

diff --git a/packages/api/src/core/range.ts b/packages/api/src/core/range.ts
diff --git a/packages/core/src/reporters/mutation-test-report-helper.ts b/packages/core/src/reporters/mutation-test-report-helper.ts
@@ -222,7 +222,7 @@ export class MutationTestReportHelper {
   }
 
   private toMutantResult(mutantResult: MutantResult, remapTestIds: (ids: string[] | undefined) => string[] | undefined): schema.MutantResult {
-    const { range, fileName, location, killedBy, coveredBy, ...apiMutant } = mutantResult;
+    const { fileName, location, killedBy, coveredBy, ...apiMutant } = mutantResult;
     return {
       ...apiMutant,
       killedBy: remapTestIds(killedBy),

diff --git a/packages/core/test/unit/mutants/find-mutant-test-coverage.spec.ts b/packages/core/test/unit/mutants/find-mutant-test-coverage.spec.ts
@@ -85,8 +85,20 @@ describe(sut.name, () => {
     it('should report onAllMutantsMatchedWithTests', () => {
       // Arrange
       const mutants = [
-        factory.mutant({ id: '1', fileName: 'foo.js', mutatorName: 'fooMutator', replacement: '<=', range: [0, 1] }),
-        factory.mutant({ id: '2', fileName: 'bar.js', mutatorName: 'barMutator', replacement: '{}', range: [2, 3] }),
+        factory.mutant({
+          id: '1',
+          fileName: 'foo.js',
+          mutatorName: 'fooMutator',
+          replacement: '<=',
+          location: { start: { line: 0, column: 0 }, end: { line: 0, column: 1 } },
+        }),
+        factory.mutant({
+          id: '2',
+          fileName: 'bar.js',
+          mutatorName: 'barMutator',
+          replacement: '{}',
+          location: { start: { line: 0, column: 2 }, end: { line: 0, column: 3 } },
+        }),
       ];
       const dryRunResult = factory.completeDryRunResult({
         tests: [factory.successTestResult({ timeSpentMs: 20 }), factory.successTestResult({ timeSpentMs: 22 })],
@@ -105,7 +117,7 @@ describe(sut.name, () => {
           replacement: '<=',
           static: true,
           estimatedNetTime: 42,
-          range: [0, 1],
+          location: { start: { line: 0, column: 0 }, end: { line: 0, column: 1 } },
         }),
         factory.mutantTestCoverage({
           id: '2',
@@ -114,7 +126,7 @@ describe(sut.name, () => {
           replacement: '{}',
           static: true,
           estimatedNetTime: 42,
-          range: [2, 3],
+          location: { start: { line: 0, column: 2 }, end: { line: 0, column: 3 } },
         }),
       ]);
     });

diff --git a/packages/core/test/unit/reporters/mutation-test-report-helper.spec.ts b/packages/core/test/unit/reporters/mutation-test-report-helper.spec.ts
@@ -1,5 +1,5 @@
 import sinon from 'sinon';
-import { File, Location, MutantResult, MutantStatus, Range, schema } from '@stryker-mutator/api/core';
+import { File, Location, MutantResult, MutantStatus, schema } from '@stryker-mutator/api/core';
 import { Reporter } from '@stryker-mutator/api/report';
 import { factory, testInjector } from '@stryker-mutator/test-helpers';
 import * as strykerUtil from '@stryker-mutator/util';
@@ -182,7 +182,6 @@ describe(MutationTestReportHelper.name, () => {
           description: 'this is mutant foo',
           duration: 42,
           location: factory.location(),
-          range: [1, 2],
           static: true,
           statusReason: 'smacked on the head',
           testsCompleted: 32,
@@ -370,7 +369,6 @@ describe(MutationTestReportHelper.name, () => {
           mutatorName: 'Foo',
           fileName: 'foo.js',
           status: MutantStatus.Killed,
-          range: [1, 2],
           location: { start: { line: 1, column: 2 }, end: { line: 4, column: 5 } },
           replacement: '+',
           id: '1',
@@ -432,7 +430,6 @@ describe(MutationTestReportHelper.name, () => {
         // Arrange
         const sut = createSut();
         const location: Location = Object.freeze({ start: Object.freeze({ line: 1, column: 5 }), end: Object.freeze({ line: 3, column: 1 }) });
-        const range: Range = [21, 35];
 
         // Act
         const actual = sut.reportCheckFailed(
@@ -442,7 +439,6 @@ describe(MutationTestReportHelper.name, () => {
             location,
             replacement: '{}',
             mutatorName: 'fooMutator',
-            range,
           }),
           factory.failedCheckResult()
         );
@@ -452,7 +448,6 @@ describe(MutationTestReportHelper.name, () => {
           id: '32',
           location,
           mutatorName: 'fooMutator',
-          range,
           fileName: 'add.js',
           replacement: '{}',
         };
@@ -486,7 +481,6 @@ describe(MutationTestReportHelper.name, () => {
           id: '3',
           location: factory.location(),
           mutatorName: 'fooMutator',
-          range: [1, 3],
           replacement: '"bar"',
           coveredBy: ['1'],
           static: false,
@@ -503,7 +497,6 @@ describe(MutationTestReportHelper.name, () => {
           id: '3',
           location: factory.location(),
           mutatorName: 'fooMutator',
-          range: [1, 3],
           replacement: '"bar"',
           coveredBy: ['1'],
           static: false,

diff --git a/packages/instrumenter/src/disable-type-checks.ts b/packages/instrumenter/src/disable-type-checks.ts
@@ -1,5 +1,5 @@
 import { types } from '@babel/core';
-import { File, Range } from '@stryker-mutator/api/core';
+import { File } from '@stryker-mutator/api/core';
 import { notEmpty } from '@stryker-mutator/util';
 
 import { createParser, getFormat, ParserOptions } from './parsers';
@@ -69,13 +69,13 @@ function removeTSDirectives(text: string, comments: Array<types.CommentBlock | t
   const directiveRanges = comments
     ?.map(tryParseTSDirective)
     .filter(notEmpty)
-    .sort((a, b) => a[0] - b[0]);
+    .sort((a, b) => a.startPos - b.startPos);
   if (directiveRanges) {
     let currentIndex = 0;
     let pruned = '';
     for (const directiveRange of directiveRanges) {
-      pruned += text.substring(currentIndex, directiveRange[0]);
-      currentIndex = directiveRange[1];
+      pruned += text.substring(currentIndex, directiveRange.startPos);
+      currentIndex = directiveRange.endPos;
     }
     pruned += text.substr(currentIndex);
     return pruned;
@@ -84,11 +84,11 @@ function removeTSDirectives(text: string, comments: Array<types.CommentBlock | t
   }
 }
 
-function tryParseTSDirective(comment: types.CommentBlock | types.CommentLine): Range | undefined {
+function tryParseTSDirective(comment: types.CommentBlock | types.CommentLine): { startPos: number; endPos: number } | undefined {
   const match = commentDirectiveRegEx.exec(comment.value);
   if (match) {
     const directiveStartPos = comment.start + match[1].length + 2; // +2 to account for the `//` or `/*` start character
-    return [directiveStartPos, directiveStartPos + match[2].length + 1];
+    return { startPos: directiveStartPos, endPos: directiveStartPos + match[2].length + 1 };
   }
   return undefined;
 }
diff --git a/packages/instrumenter/src/mutant.ts b/packages/instrumenter/src/mutant.ts
@@ -38,7 +38,6 @@ export class Mutant {
       id: this.id,
       location: toApiLocation(this.original.loc!, this.lineOffset),
       mutatorName: this.mutatorName,
-      range: [this.original.start! + this.positionOffset, this.original.end! + this.positionOffset],
       replacement: this.replacementCode,
       statusReason: this.ignoreReason,
       status: this.ignoreReason ? MutantStatus.Ignored : undefined,

diff --git a/packages/instrumenter/test/unit/mutant.spec.ts b/packages/instrumenter/test/unit/mutant.spec.ts
@@ -1,7 +1,6 @@
 import { types } from '@babel/core';
-
 import { Mutant as MutantApi, MutantStatus } from '@stryker-mutator/api/core';
-
+import { factory } from '@stryker-mutator/test-helpers';
 import { expect } from 'chai';
 
 import { Mutant } from '../../src/mutant';
@@ -33,7 +32,7 @@ describe(Mutant.name, () => {
         mutatorName: 'fooMutator',
         ignoreReason: 'ignore',
       });
-      mutant.original.loc = { start: { column: 0, line: 0 }, end: { column: 0, line: 0 } };
+      mutant.original.loc = factory.location();
       const expected: Partial<MutantApi> = {
         fileName: 'file.js',
         id: '2',
@@ -51,7 +50,7 @@ describe(Mutant.name, () => {
         replacement: types.stringLiteral('Stryker was here!'),
         mutatorName: 'fooMutator',
       });
-      mutant.original.loc = { start: { column: 0, line: 0 }, end: { column: 0, line: 0 } };
+      mutant.original.loc = factory.location();
       const expected: Partial<MutantApi> = {
         fileName: 'file.js',
         id: '2',
@@ -74,7 +73,6 @@ describe(Mutant.name, () => {
 
       // Assert
       expect(actual.location).deep.eq({ start: { line: 4, column: 3 }, end: { line: 4, column: 8 } });
-      expect(actual.range).deep.eq([45, 50]);
     });
   });
 });
diff --git a/packages/test-helpers/src/factory.ts b/packages/test-helpers/src/factory.ts
@@ -154,7 +154,6 @@ export const mutant = factoryMethod<Mutant>(() => ({
   id: '42',
   fileName: 'file',
   mutatorName: 'foobarMutator',
-  range: [0, 0],
   location: location(),
   replacement: 'replacement',
 }));
@@ -343,7 +342,6 @@ export const mutantTestCoverage = factoryMethod<MutantTestCoverage>(() => ({
   replacement: '',
   location: location(),
   estimatedNetTime: 42,
-  range: [0, 1],
 }));
 
 export function injector(): sinon.SinonStubbedInstance<Injector> {

diff --git a/packages/typescript-checker/src/fs/hybrid-file-system.ts b/packages/typescript-checker/src/fs/hybrid-file-system.ts
@@ -32,7 +32,7 @@ export class HybridFileSystem {
     }
   }
 
-  public mutate(mutant: Pick<Mutant, 'fileName' | 'range' | 'replacement'>): void {
+  public mutate(mutant: Pick<Mutant, 'fileName' | 'location' | 'replacement'>): void {
     const fileName = toPosixFileName(mutant.fileName);
     const file = this.files.get(fileName);
     if (!file) {

diff --git a/packages/typescript-checker/src/fs/script-file.ts b/packages/typescript-checker/src/fs/script-file.ts
@@ -1,8 +1,9 @@
 import ts from 'typescript';
-import { Mutant } from '@stryker-mutator/api/core';
+import { Mutant, Position } from '@stryker-mutator/api/core';
 
 export class ScriptFile {
   private readonly originalContent: string;
+  private sourceFile: ts.SourceFile | undefined;
   constructor(public content: string, public fileName: string, public modifiedTime = new Date()) {
     this.originalContent = content;
   }
@@ -14,12 +15,22 @@ export class ScriptFile {
 
   public watcher: ts.FileWatcherCallback | undefined;
 
-  public mutate(mutant: Pick<Mutant, 'range' | 'replacement'>): void {
+  public mutate(mutant: Pick<Mutant, 'location' | 'replacement'>): void {
     this.guardMutationIsWatched();
-    this.content = `${this.originalContent.substr(0, mutant.range[0])}${mutant.replacement}${this.originalContent.substr(mutant.range[1])}`;
+
+    const start = this.getOffset(mutant.location.start);
+    const end = this.getOffset(mutant.location.end);
+    this.content = `${this.originalContent.substr(0, start)}${mutant.replacement}${this.originalContent.substr(end)}`;
     this.touch();
   }
 
+  private getOffset(pos: Position): number {
+    if (!this.sourceFile) {
+      this.sourceFile = ts.createSourceFile(this.fileName, this.content, ts.ScriptTarget.Latest, false, undefined);
+    }
+    return this.sourceFile.getPositionOfLineAndCharacter(pos.line, pos.column);
+  }
+
   public resetMutant(): void {
     this.guardMutationIsWatched();
     this.content = this.originalContent;

diff --git a/packages/typescript-checker/test/integration/project-references.it.spec.ts b/packages/typescript-checker/test/integration/project-references.it.spec.ts
@@ -1,8 +1,9 @@
 import path from 'path';
 import fs from 'fs';
+import os from 'os';
 
 import { expect } from 'chai';
-import { Mutant, Range } from '@stryker-mutator/api/core';
+import { Location, Mutant } from '@stryker-mutator/api/core';
 import { CheckResult, CheckStatus } from '@stryker-mutator/api/check';
 import { testInjector, factory } from '@stryker-mutator/test-helpers';
 
@@ -57,15 +58,20 @@ const fileContents = Object.freeze({
 });
 
 function createMutant(fileName: 'src/todo.ts' | 'test/todo.spec.ts', findText: string, replacement: string, offset = 0): Mutant {
-  const originalOffset: number = fileContents[fileName].indexOf(findText);
-  if (originalOffset === -1) {
+  const lines = fileContents[fileName].split(os.EOL);
+  const lineNumber = lines.findIndex((l) => l.includes(findText));
+  if (lineNumber === -1) {
     throw new Error(`Cannot find ${findText} in ${fileName}`);
   }
-  const range: Range = [originalOffset + offset, originalOffset + findText.length];
+  const textColumn = lines[lineNumber].indexOf(findText);
+  const location: Location = {
+    start: { line: lineNumber, column: textColumn + offset },
+    end: { line: lineNumber, column: textColumn + findText.length },
+  };
   return factory.mutant({
-    fileName: resolveTestResource(fileName),
+    fileName: resolveTestResource('src', fileName),
     mutatorName: 'foo-mutator',
-    range,
+    location,
     replacement,
   });
 }
diff --git a/packages/typescript-checker/test/integration/single-project.it.spec.ts b/packages/typescript-checker/test/integration/single-project.it.spec.ts
@@ -3,7 +3,7 @@ import fs from 'fs';
 
 import { testInjector, factory, assertions } from '@stryker-mutator/test-helpers';
 import { expect } from 'chai';
-import { Mutant, Range } from '@stryker-mutator/api/core';
+import { Location, Mutant } from '@stryker-mutator/api/core';
 import { CheckResult, CheckStatus } from '@stryker-mutator/api/check';
 
 import { createTypescriptChecker } from '../../src';
@@ -34,9 +34,7 @@ describe('Typescript checker on a single project', () => {
 
   it('should be able to validate a mutant that does not result in an error', async () => {
     const mutant = createMutant('todo.ts', 'TodoList.allTodos.push(newItem)', 'newItem? 42: 43');
-    const expectedResult: CheckResult = {
-      status: CheckStatus.Passed,
-    };
+    const expectedResult: CheckResult = { status: CheckStatus.Passed };
     const actual = await sut.check(mutant);
     expect(actual).deep.eq(expectedResult);
   });
@@ -110,15 +108,20 @@ const fileContents = Object.freeze({
 });
 
 function createMutant(fileName: 'not-type-checked.js' | 'todo.spec.ts' | 'todo.ts', findText: string, replacement: string, offset = 0): Mutant {
-  const originalOffset: number = fileContents[fileName].indexOf(findText);
-  if (originalOffset === -1) {
+  const lines = fileContents[fileName].split('\n');
+  const lineNumber = lines.findIndex((line) => line.includes(findText));
+  if (lineNumber === -1) {
     throw new Error(`Cannot find ${findText} in ${fileName}`);
   }
-  const range: Range = [originalOffset + offset, originalOffset + findText.length];
+  const textColumn = lines[lineNumber].indexOf(findText);
+  const location: Location = {
+    start: { line: lineNumber, column: textColumn + offset },
+    end: { line: lineNumber, column: textColumn + findText.length },
+  };
   return factory.mutant({
     fileName: resolveTestResource('src', fileName),
     mutatorName: 'foo-mutator',
-    range,
+    location,
     replacement,
   });
 }