feat(coverage analysis): Support transpiled code #559

nicojs · 2017-12-24T01:08:05Z

Support coverage analysis while having one transpiler enabled. Remove the old "keepSourceMaps" boolean on transpiler options in favor of "produceSourceMaps". This allows for the source map resolving logic to be moved to stryker itself, removing the need to implement it in every transpiler plugin.

TODO:

~~Enable source map for the stryker-babel-transpiler~~ Throw an error if the stryker-babel-transpiler is asked to produce source maps (for now)
Add unit tests to the SourceMapper class
Add integration test for SourceMapping
Find a way to remove the warnings if a mutant has no javascript representation. For example: mutating a TypeScript interface. Use this file as an example: "/src/isolated-runner/MessageProtocol.ts"
Refactor the MutantTestMatcher class. (too complex)
Write an awesome blog article (PR here: docs(blog): Add blog about typescript coverage analysis stryker-mutator.github.io#53)

Done:

Stryker API

Rename keepSourceMaps -> produceSourceMaps on TranspilerOptions
Remove getMappedLocation method from Transpiler
Remove FileLocation interface from the transpiler api (not needed anymore)
Add an id field to MutantResult and MatchedMutant. This is used in the progress reporter for now to identify mutants without test coverage.

Stryker

Add dependency to 'source-map'.
Add class SourceMapper responsible for mapping original code to transpiled code.
Use SourceMapper in the MutantTestMatcher in order to locate a mutant in the js output file.
Use the mutant id in the progress reporter to identify mutants without test coverage. Using MutantResult.NoCoverage for this is not enough anymore as mutants that are not matched to a test can now also have the status TranspileError.
MutationTestExecutor: Report a mutant as survived early if a mutant results in the same transpiled code (i.e. mutating a TypeScript interface)
MutantTestMatcher: Delay reporting a warning if the statement could not be found in the statement map. Instead mark the mutant as TestSelectionResult.Failed and later log the warning if it is ran in the Sandbox. Don't report if an early result prevented the mutant from being tested.

Stryker TypeScript

Remove old source-mapping logic from stryker-typescript, it was never used
Support produceSourceMaps transpiler option

Stryker babel transpiler

throw error when asked to produce source maps (for now)

First attempt implement coverage analysis while having one transpiler enabled. Removed the old "keepSourceMaps" boolean on transpiler options in favor of "produceSourceMaps". This allows for the source map resolving logic to be moved to stryker itself. *Stryker API* * Rename `keepSourceMaps` -> `produceSourceMaps` on `TranspilerOptions` * Remove `getMappedLocation` method from `Transpiler` * Remove `FileLocation` interface from the transpiler api (not needed anymore) * Add an id` field to `MutantResult` and `MatchedMutant`. This is used in the progress reporter for now to identify mutants without test coverage. *Stryker* * Add dependency to 'source-map'. * Add class `SourceMapper` responsible for mapping original code to transpiled code. * Use `SourceMapper` in the `MutantTestMatcher` in order to locate a mutant in the js output file. * Use the mutant id in the progress reporter to identify mutants without test coverage. Using `MutantResult.NoCoverage` for this is not enough anymore as mutants that are not matched to a test can now also have the status `TranspileError`. *Stryker TypeScript* * Remove old source-mapping logic from stryker-typescript, it was never used

nicojs · 2017-12-24T01:09:05Z

@Archcry could you add babel source map production to this branch? You can use stryker-typescript as an example.

* Rename `StatementLocation` > `StatementIndex` * Move location methods to its own class `LocationHelper` * Remove an if statement

nicojs · 2017-12-27T13:19:45Z

Find a way to remove the warnings if a mutant has no javascript representation. For example: mutating a TypeScript interface. Use this file as an example: "/src/isolated-runner/MessageProtocol.ts"

@simondel what do you think about this solution:

Delay printing the warning until after the mutant is transpiled. If the transpiled code results in the same code is the same as without the mutant (for example: mutating a typescript interface), we don't even run any tests, as we know for sure they will pass. We just mark the mutant as survived, never printing a warning.

EDIT: This is what i have implemented in my latest commit.

* Don't run mutant in a sandbox when the transpiled result is the same as the not mutated transpiled result * When the tests for a mutant could not be selected because the statement or function could not be found in the maps, delay reporting the warning untill we know for sure a mutant does not have an early result.

nicojs · 2017-12-28T06:30:26Z

Running Stryker on Stryker now takes 6 minutes on my machine (max resources allocated). Saving about 10 minutes.

Ran 41.88 tests per mutant on average.
[2017-12-28 07:26:48.679] [INFO] Stryker - Done in 6 minutes 4 seconds.

simondel

Here is part one of the feedback. I mainly find the SourceMapper to be quite confusing

simondel · 2018-01-12T12:39:42Z

packages/stryker-api/src/transpile/TranspilerOptions.ts

@@ -10,5 +10,5 @@ export default interface TranspilerOptions {
   * Indicates whether or not the source maps need to be kept.


This comment seems to be out of date

simondel · 2018-01-15T13:46:22Z

packages/stryker-typescript/src/TypescriptTranspiler.ts

-      const resultFiles = filterNotEmpty(allFiles.map(file => {
-        if (file.transpiled && isTypescriptFile(file)) {
-          return outputFiles[file.name];
+      let moreOutput = true;


Perhaps isSingleOutput = false would be nicer. (Note that this turns the implementation around and you will also have to reverse the if (moreOutput) check and you have to swap the way you set the variable after emitting.

simondel · 2018-01-15T13:50:23Z

packages/stryker-typescript/src/transpiler/TranspilingLanguageService.ts

-          transpiled: sourceFile.transpiled,
+  emit(fileDescriptor: FileDescriptor): EmitOutput {
+    const emittedFiles = this.languageService.getEmitOutput(fileDescriptor.name).outputFiles;
+    const jsFile = emittedFiles.find(file => file.name.endsWith('.js'));


Is there a chance that this outputs '.jsx' files?

simondel · 2018-01-15T13:55:51Z

packages/stryker-typescript/test/unit/TypescriptTranspilerSpec.ts

@@ -133,4 +123,8 @@ describe('TypescriptTranspiler', () => {
      expect(result.outputFiles).lengthOf(0);
    });
  });
+
+  describe('transpile with source maps', () => {


Is this supposed to be empty?

euhm... 😒

simondel · 2018-01-15T14:13:31Z

packages/stryker/src/TestableMutant.ts

-    runResult.tests.forEach((testResult, id) => this.addTestResult(id, testResult));
+  public selectAllTests(runResult: RunResult, testSelectionResult: TestSelectionResult) {
+    this.testSelectionResult = testSelectionResult;
+    runResult.tests.forEach((testResult, id) => this.selectTest(id, testResult));


It would be cleaner if we would swap the params around so they are consistent. Right now we swap the params around when we call selectTest

simondel · 2018-01-15T14:26:46Z

packages/stryker/src/Sandbox.ts

@@ -65,6 +65,9 @@ export default class Sandbox {

  public async runMutant(transpiledMutant: TranspiledMutant): Promise<RunResult> {
    const mutantFiles = transpiledMutant.transpileResult.outputFiles;
+    if (transpiledMutant.mutant.testSelectionResult === TestSelectionResult.Failed) {


Can't we check this in the MutantTestMatcher?

And if we move this check to the MutantTestMatcher, we may be able to remove the TestSelectionResult all together.

No. I added it in this PR. See my comment here: #559 (comment)

If you have a better idea, please let me know :/

simondel · 2018-01-15T14:56:42Z

packages/stryker/src/transpiler/SourceMapper.ts

+    SOURCE_MAP_URL_REGEX.lastIndex = 0;
+    let currentMatch: RegExpExecArray | null;
+    let lastMatch: RegExpExecArray | null = null;
+    while (currentMatch = SOURCE_MAP_URL_REGEX.exec(transpiledFile.content)) {


What does this do?

it retrieves the final sourceMappingURL comment in a given file. I've added a comment to it 👍

simondel · 2018-01-15T14:57:47Z

packages/stryker/src/transpiler/SourceMapper.ts

+  }
+
+  private getSourceMapForFile(transpiledFile: TextFile) {
+    const sourceMappingUrl = this.getSourceMapUrl(transpiledFile);


Is it an URL that we get?

Yes. It is either a data url (data:application/json;base64,ABC...) or an external url (main.js.map).

I've renamed some methods:
getSourceMapFromTranspiledFiles -> getExternalSourceMap
getSourceMapFromDataUrl -> getInlineSourceMap

Is that more clear?

simondel · 2018-01-15T15:00:38Z

packages/stryker/src/transpiler/SourceMapper.ts

+  }
+}
+
+export class TranspiledSourceMapper extends SourceMapper {


This class is quite difficult to read for me. I have the feeling that I get sent all over the place and I have no idea why the code does what it does.

I've refactored a bunch of code and made it more object-oriented by adding a SourceMap class (instead of a SourceMapInfo interface). Hope this is a bit clearer.

simondel · 2018-01-15T15:02:08Z

packages/stryker/src/MutantTestMatcher.ts

@@ -55,45 +65,52 @@ export default class MutantTestMatcher {
  }

  enrichWithCoveredTests(testableMutant: TestableMutant) {


This is only used privately (except for when testing)

Good catch. Yes, it is public as a convenience for unit testing. Ideally I don't like to make these methods public, but it already was the case and I didn't feel like refactoring that part as well.

nicojs · 2018-01-19T23:10:37Z

@simondel Thanks for your feedback! I think I've commented on/fixed all remarks. Could you please take another look.

nicojs added 4 commits December 24, 2017 13:36

test(SourceMapper): Add integration test for SourceMapper

eb50f13

test(initial test): Only produce source maps when needed

9c79c25

test(SourceMapper): Add source mapper unit tests

dd85bf4

refactor(MutantTestMatcher)

06aa187

* Rename `StatementLocation` > `StatementIndex` * Move location methods to its own class `LocationHelper` * Remove an if statement

nicojs changed the title ~~WIP: feat(coverage analysis): Implement coverage analysis with one~~ WIP: feat(coverage analysis): Support transliled code Dec 27, 2017

nicojs changed the title ~~WIP: feat(coverage analysis): Support transliled code~~ WIP: feat(coverage analysis): Support transpiled code Dec 27, 2017

nicojs assigned simondel Dec 28, 2017

fix(build): Fix compiler warning

07340ea

nicojs changed the title ~~WIP: feat(coverage analysis): Support transpiled code~~ feat(coverage analysis): Support transpiled code Dec 28, 2017

simondel requested changes Jan 16, 2018

View reviewed changes

nicojs added 2 commits January 19, 2018 18:41

refactor(typescript): review comments

5f401ec

refactor(stryker): Refactor source mapping logic

8a5260d

docs(stryker-api): Update jsdoc of produceSourceMaps

bef6025

simondel approved these changes Feb 3, 2018

View reviewed changes

simondel merged commit 7c351ad into master Feb 3, 2018

simondel deleted the feat/transpiler-coverage-analysis branch February 3, 2018 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coverage analysis): Support transpiled code #559

feat(coverage analysis): Support transpiled code #559

nicojs commented Dec 24, 2017 •

edited

Loading

nicojs commented Dec 24, 2017

nicojs commented Dec 27, 2017 •

edited

Loading

nicojs commented Dec 28, 2017 •

edited

Loading

simondel left a comment

simondel Jan 12, 2018

simondel Jan 15, 2018

simondel Jan 15, 2018

simondel Jan 15, 2018

nicojs Jan 19, 2018

simondel Jan 15, 2018

simondel Jan 15, 2018

simondel Jan 15, 2018

nicojs Jan 19, 2018

simondel Jan 15, 2018

nicojs Jan 19, 2018

simondel Jan 15, 2018

nicojs Jan 19, 2018 •

edited

Loading

simondel Jan 15, 2018

nicojs Jan 19, 2018

simondel Jan 15, 2018

nicojs Jan 19, 2018 •

edited

Loading

nicojs commented Jan 19, 2018

		@@ -10,5 +10,5 @@ export default interface TranspilerOptions {
		* Indicates whether or not the source maps need to be kept.

		@@ -55,45 +65,52 @@ export default class MutantTestMatcher {
		}

		enrichWithCoveredTests(testableMutant: TestableMutant) {

feat(coverage analysis): Support transpiled code #559

feat(coverage analysis): Support transpiled code #559

Conversation

nicojs commented Dec 24, 2017 • edited Loading

nicojs commented Dec 24, 2017

nicojs commented Dec 27, 2017 • edited Loading

nicojs commented Dec 28, 2017 • edited Loading

simondel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

nicojs commented Jan 19, 2018

nicojs commented Dec 24, 2017 •

edited

Loading

nicojs commented Dec 27, 2017 •

edited

Loading

nicojs commented Dec 28, 2017 •

edited

Loading

nicojs Jan 19, 2018 •

edited

Loading

nicojs Jan 19, 2018 •

edited

Loading