feat(Vue): add Vue mutator #723

simondel · 2018-04-26T13:12:40Z

Adds a new stryker plugin: stryker-vue-mutator. This plugin requires additional mutator plugins to function, as specified in the package's README. When using this plugin, you should set your mutator property to 'vue'.

Fixes #579

…ator

nicojs

Looks great! Just have some remarks on the implementation details.

nicojs · 2018-05-11T08:49:17Z

packages/stryker-vue-mutator/README.md

+
+# Stryker Vue mutator
+
+A mutator that supports JavaScript for [Stryker](https://stryker-mutator.io), the mutation testing framework for JavaScript and friends.


This still needs to be updated. We should make it clear that this plugin adds support for *.vue files.

nicojs · 2018-05-11T08:58:02Z

packages/stryker-vue-mutator/package.json

+  },
+  "peerDependencies": {
+    "stryker-api": "^0.17.0",
+    "vue-template-compiler": "^2.0.0"


This peerDep should be removed, right?

This plugin is used to extract the template code from the vue file. We need it in order to run but we need the version that is exactly the same as the version of vue itself. Else the plugin won't work.

Ok, in that case, please remove the dependency on "vue-template-compiler"

nicojs · 2018-05-11T08:58:39Z

packages/stryker-vue-mutator/src/VueMutator.ts

+        const script = compiler.parseComponent(file.textContent).script;
+        const mutator = this.getVueScriptMutator(script);
+        const vueFile = new File(
+          file.name + '.js', /* TODO: Remove this once all mutators understand that vue files are OK to mutate */


We should instead choose the extension based on the script tag. This is the most logical step IMHO.

nicojs · 2018-05-11T09:01:02Z

packages/stryker-vue-mutator/src/VueMutator.ts

+  constructor(private config: Config) { }
+
+  mutate(inputFiles: File[]): Mutant[] {
+    this.initializeMutators();


Can't we move this to the constructor. That way it works nicely with TS 2.7 --strictPropertyInitialization

nicojs · 2018-05-11T09:02:08Z

packages/stryker-vue-mutator/src/VueMutator.ts

+
+  private getVueScriptMutator(script: any): Mutator {
+    const lang: string | undefined = script.attrs.lang;
+    let mutatorName: string;


We can make it a const if we move the case switch to a separate method with return statements in the cases

I'd prefer we don't do this to keep the readability that we have right now

nicojs · 2018-05-11T09:02:25Z

packages/stryker-vue-mutator/src/VueMutator.ts

+        throw new Error(`Found unsupported language attribute 'lang="${lang}"' on a <script> block.`);
+    }
+
+    let mutator = this.mutators[mutatorName];


const please :)

nicojs · 2018-05-11T09:02:36Z

packages/stryker-vue-mutator/src/VueMutator.ts

+  }
+
+  private getMutator(file: File): Mutator {
+    let mutator = this.mutators['typescript'] || this.mutators['javascript'];


const please

nicojs · 2018-05-11T09:04:08Z

packages/stryker-vue-mutator/test/integration/VueMutator.it.ts

+
+      const mutants = sut.mutate(files);
+
+      expect(mutants.length).to.equal(8);


This test is pretty flaky, right? Whenever we update our js mutator, this might fail. Can we verify that some mutants that you expect are there?

I now test on the specific types of mutators that should exist

nicojs · 2018-05-11T09:09:02Z

packages/stryker-vue-mutator/test/unit/VueMutatorSpec.ts

+      expect(() => sut.mutate(files)).throws(`The 'javascript' mutator is required to mutate a <script> block but it was not found. Please read the README of this package for information on configuration.`);
+    });
+
+    describe('when the first file is not a Vue file', () => {


I think this describe is not helping with the explanation of what is tested in the it. Could we move the it to the top level and remove this describe?

nicojs · 2018-05-11T09:15:09Z

packages/stryker-vue-mutator/src/VueMutator.ts

+    return mutants;
+  }
+
+  private initializeMutators() {


Can we move this method to a separate file? That way the unit tests can be independent of the current MutatorFactory way of working. We will remove the mutator factory as part of #667 .

nicojs

Awesome. 1 last remark.

nicojs · 2018-05-11T12:31:15Z

packages/stryker-vue-mutator/src/helpers/MutatorHelpers.ts

+import { Mutator, MutatorFactory } from 'stryker-api/mutant';
+import { Config } from 'stryker-api/config';
+
+const generateMutators = (config: Config): { [name: string]: Mutator; } => {


This name... In what way is it actually generating mutators? More like discoverMutators right?

nicojs · 2018-08-04T20:52:43Z

Is this ready to be reviewed?

simondel · 2018-08-04T20:56:35Z

Allright, one integration test is done. Do we also want a project with jest and typescript @nicojs ?

nicojs · 2018-08-04T21:00:35Z

Hmm. What do you think? Couldn't hurt of course, but does it test something new?

simondel · 2018-08-04T21:04:08Z

It redirects the code to a different path because all the Vue mutator does is send code to the correct mutator.

simondel · 2018-08-04T21:05:22Z

packages/stryker-vue-mutator/package.json

+    "vue-template-compiler": "^2.0.0"
+  },
+  "peerDependencies": {
+    "stryker-api": "^0.17.0",


This has to be updated

simondel · 2018-08-04T21:06:10Z

The package itself does have tests to see if the code works though.

nicojs

Great work so far. Let's keep the momentum going 👍 .

I've got some overall small remarks.

nicojs · 2018-08-08T20:14:37Z

packages/stryker-vue-mutator/src/VueMutator.ts

+const compiler = require('vue-template-compiler');
+
+export default class VueMutator implements Mutator {
+  private mutators: { [name: string]: Mutator; };


Can we make this readonly?

nicojs · 2018-08-08T20:15:49Z

packages/stryker-vue-mutator/src/VueMutator.ts

+  }
+
+  mutate(inputFiles: File[]): Mutant[] {
+    let mutants: Mutant[] = [];


Can we declare it as a const to prevent incidental re-initialization?

nicojs · 2018-08-08T20:16:05Z

packages/stryker-vue-mutator/src/VueMutator.ts

+          mutant.range[0] += script.start;
+          mutant.range[1] += script.start;
+        });
+        mutants = mutants.concat(vueMutants);


Please use mutants.push(...vueMutants)

nicojs · 2018-08-08T20:16:11Z

packages/stryker-vue-mutator/src/VueMutator.ts

+        mutants = mutants.concat(vueMutants);
+      } else {
+        const mutator = this.getMutator(file);
+        mutants = mutants.concat(mutator.mutate([file]));


Please use mutants.push(...vueMutants)

nicojs · 2018-08-08T20:17:33Z

packages/stryker-vue-mutator/src/VueMutator.ts

+  }
+
+  private getMutator(file: File): Mutator {
+    const mutator = this.mutators['typescript'] || this.mutators['javascript'];


This feels strange to me. Will the typescript mutator always work for javascript as well? Can we create const strings or a string enum for 'typescript' and 'javascript' here?

Also, couldn't we fail faster? As soon as the mutate method is called and no mutators are registered for example?

I'll save the string names 'typescript' and 'javascript' somewhere. I think the typescript mutator should always be able to process javascript code. Javascript is typescript after all, right?

We could fail faster, but we would still have to check if the correct mutator is available for Vue code. For instance, we can't just say 'if either of the two mutators exists, all is fine' because the JavaScript mutator may be present while there is a Vue file with TypeScript code.

nicojs · 2018-08-08T20:19:09Z

packages/stryker-vue-mutator/test/integration/VueMutator.it.ts

+import VueMutator from '../../src/VueMutator';
+require('../../src/index');
+require('stryker-javascript-mutator');
+require('stryker-typescript');


Please change this to:

import '../../src/index'; import 'stryker-javascript-mutator'; import 'stryker-typescript';

nicojs · 2018-08-08T20:20:52Z

packages/stryker-vue-mutator/test/unit/VueMutatorSpec.ts

+  });
+
+  afterEach(() => {
+    sandbox.restore();


This is not needed. It is done in /stryker-vue-mutator/test/helpers/initSinon.ts

nicojs · 2018-08-08T20:22:44Z

packages/stryker-vue-mutator/test/unit/VueMutatorSpec.ts

+
+    it('should pass regular files to the TypeScript mutator, even if the JavaScript mutator is installed', () => {
+      const stubJavaScriptMutator = sandbox.createStubInstance<Mutator>(VueMutator);
+      const stubTypeScriptMutator = sandbox.createStubInstance<Mutator>(VueMutator);


Could we create these mocks/stubs once in before each? It seems that you repeat yourself here. We could also opt for an arrangeMutators() helper method of some kind.

nicojs · 2018-08-08T20:23:25Z

packages/stryker-vue-mutator/test/unit/VueMutatorSpec.ts

@@ -0,0 +1,327 @@
+import { expect } from 'chai';


I'm a big fan of the black-boxy unit tests here 👍

simondel · 2018-08-12T19:27:30Z

@nicojs Thanks for the review! I've refactored the code.

nicojs

Looks good to me. This comment is still valid:

This test is pretty flaky, right? Whenever we update our js mutator, this might fail. Can we verify that some mutants that you expect are there?

But that's not a blocker for me. Feel free to merge when green

simondel · 2018-08-12T19:52:18Z

I'll try to make them less flakey later this week 👍

simondel · 2018-08-15T05:25:05Z

I have updated the tests with my latest commit.

nicojs · 2018-08-16T10:39:27Z

I see you mocked away the mutators for unittests. Great. Unittests are a little wet (vs dry) for my taste, but ok to merge

simondel · 2018-08-16T11:24:02Z

@nicojs Wet as in the opposite of DRY (don't repeat yourself) or wet as in wet clothes? 🤔

nicojs · 2018-08-27T11:16:11Z

Wet as opposite of DRY indeed. I like my unit tests a little DAMP

simondel added 10 commits February 3, 2018 21:56

feat(Vue mutator): Add package folder

713bae8

Merge branch 'master' into vue-mutator

27b57e1

feat(VueMutator): Add initial implementation

42278f7

Merge branch 'master' into vue-mutator

97e5f51

feat(stryker-api): Support new File class

7f4613e

test(VueMutator): add unit tests

4973114

test(VueMutator): add test for correct mutants

5473140

Merge branch 'master' into vue-mutator

c44c636

refactor(VueMutatorSpec): add missing semicolon

acfb241

test(VueMutator): add integration test

104e17d

ghost assigned simondel Apr 26, 2018

ghost added the 🔎 Needs review label Apr 26, 2018

simondel added 2 commits April 26, 2018 15:16

fix(dependencies): add mutator devDependencies

3774082

docs(README): add readme

4354f74

simondel requested a review from nicojs May 11, 2018 08:18

simondel changed the title ~~WIP: feat(Vue): add Vue mutator~~ feat(Vue): add Vue mutator May 11, 2018

simondel added 2 commits May 11, 2018 10:20

docs(README): mention that all files are passed to the underlying mut…

b459534

…ator

Merge branch 'master' into vue-mutator

082d2c9

nicojs requested changes May 11, 2018

View reviewed changes

build(workspace): add vue mutator to workspace

19c3053

ghost assigned nicojs May 11, 2018

simondel added 2 commits May 11, 2018 13:52

refactor(VueMutator): implement review comments

e157b42

test(VueMutator): fix tests

39f622d

nicojs requested changes May 11, 2018

View reviewed changes

nicojs and others added 2 commits May 11, 2018 14:43

docs(readme): smooth out some of the wording in the vue readme

97e5fac

build: Don't depend directly on the vue-template-compiler

ecab64b

Tompage1994 mentioned this pull request May 15, 2018

Write tests to use Stryker to automate mutation testing and reporting through Jenkins rht-labs/enablement-docs#90

Closed

simondel added 3 commits June 8, 2018 14:04

test(Vue): add blank Vue JavaScript project

1a0c9eb

test(Vue): add stryker config

11b0e06

test: include vue-template-compiler as a devDependency

22eb6d0

simondel added 8 commits June 8, 2018 14:35

Merge branch 'master' into vue-mutator

cf3ea7d

test(Vue): add tsconfig.json for integration test

c86079b

Merge branch 'master' into vue-mutator

d79a939

Merge branch 'master' into vue-mutator

af86273

Remove package-lock.json

152038a

test(integration): Turn off coverageAnalysis

1138bb6

test(integration): Intergrate with correct stryker version

acf1f85

test(integration): Check if the correct mutation score is generated

122209b

simondel commented Aug 4, 2018

View reviewed changes

build: Update dependencies

6694ecc

nicojs requested changes Aug 8, 2018

View reviewed changes

refactor(Vue): clean up code

d9aa5d4

nicojs approved these changes Aug 12, 2018

View reviewed changes

test(Vue): test for specific mutators

20d2ee8

simondel merged commit bc28fb6 into master Aug 16, 2018

ghost removed the 🔎 Needs review label Aug 16, 2018

simondel deleted the vue-mutator branch August 16, 2018 11:24


		# Stryker Vue mutator

		A mutator that supports JavaScript for [Stryker](https://stryker-mutator.io), the mutation testing framework for JavaScript and friends.


		const mutants = sut.mutate(files);

		expect(mutants.length).to.equal(8);

feat(Vue): add Vue mutator #723

feat(Vue): add Vue mutator #723

Conversation

simondel commented Apr 26, 2018 • edited Loading

nicojs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs May 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs commented Aug 4, 2018

simondel commented Aug 4, 2018

nicojs commented Aug 4, 2018

simondel commented Aug 4, 2018

Choose a reason for hiding this comment

simondel commented Aug 4, 2018

nicojs 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

simondel commented Aug 12, 2018

nicojs left a comment

Choose a reason for hiding this comment

simondel commented Aug 12, 2018

simondel commented Aug 15, 2018

nicojs commented Aug 16, 2018

simondel commented Aug 16, 2018

nicojs commented Aug 27, 2018

simondel commented Apr 26, 2018 •

edited

Loading

nicojs May 11, 2018 •

edited

Loading