feat(Stryker CLI 'init'): Support for preset configuration during 'stryker init' #1248

Wmaarts · 2018-11-22T09:40:08Z

Implementation of #1049

? Are you using one of these frameworks? Then select a preset configuration. (Use arrow keys)
> angular
  react
  vueJs
  ──────────────
  none

Small disclaimer

(my first time contributing to an open-source project, hope I'm doing this right. Also, I'm new to the stryker project. 😄 )

Summary

Running 'stryker init' prompts the user to select a preset (or do custom setup).
Supports Angular, React and vuejs
Loads a fixed configuration and installs appropriate dependencies
Supports adding preset-specific questions
- For example: React preset prompts the user to select between JSX and TSX config
Some unit tests written
All based on the the stryker handbook

Implementation

Where?

Presets are defined in the stryker package under \src\initializer\presets.

Adding a new preset

Extends the StrykerPreset class like so:

export class MyPreset extends StrykerPreset { 
    public dependencies: string[] = [];        // preset-specific dependencies here
    public conf: string = '';                  // will be loaded into stryker.conf.js
    public async prompt(): Promise<void> { };  // will be called after selecting this preset
}

Add your preset to StrykerPresets.ts like so:

const strykerPresets: PresetOption[] = [
        {name: 'angular', create() { return new AngularPreset(); } },
        {name: 'react', create() { return new ReactPreset(); } },
        {name: 'vueJs', create() { return new VueJsPreset(); } },
        {name: 'myPreset', create() { return new MyPreset(); } } // <- add this line
        // Add new presets here
];

Done. 👍

Shortcomings

Has only been fully tested on a small Angular project (I had nothing else at hand... )
No integration tests yet
(nitpick) When using angular, the tests should be run using npx stryker run instead of stryker run, but when the angular preset is chosen the user is still told to use stryker run.

…ting it

nicojs

Looks great already! Thanks for picking this up! I've got a few remarks, but mostly small ones.

I like the idea of the different StrykerPreset classes being able to ask additional questions. It is a clean way to separate them from the StrykerInitializer class itself.

I would like to have a unit tests for the different Stryker initializers themselves. Can be small, just asserting a part of the configuration. If you want, I can help with that.

nicojs · 2018-11-22T19:07:30Z

packages/stryker-javascript-mutator/package.json

@@ -47,7 +47,6 @@
  "devDependencies": {
    "@babel/types": "~7.1.2",
    "@types/babel__generator": "^7.0.0",
-    "@types/babel__parser": "^7.0.0",


Why is this in the PR?

Had to do with this PR (#1243) and also (#1247). These changes were needed in order to make 'npm install' work and they accidentally ended up in this PR too.
I merged stryker/master into my fork branch, I think that fixes it.

nicojs · 2018-11-22T19:07:42Z

package.json

@@ -15,7 +15,7 @@
    "@types/mkdirp": "^0.5.2",
    "@types/mocha": "^2.2.44",
    "@types/rimraf": "2.0.2",
-    "@types/sinon": "^5.0.1",
+    "@types/sinon": "5.0.5",


Why is this in the PR?

same as above comment

nicojs · 2018-11-22T19:08:30Z

packages/stryker/package.json

@@ -83,7 +83,10 @@
    "@types/node": "~10.11.4",
    "@types/prettier": "~1.13.1",
    "@types/progress": "~2.0.1",
-    "stryker-api": "^0.21.5"
+    "stryker-api": "^0.21.5",
+    "stryker-html-reporter": "^0.16.7",


Can we remove these dependencies? We're planning to run Stryker on Stryker in the build in Februari

I'll remove then, yes. These were accidentally generated while testing stryker init.

nicojs · 2018-11-22T19:12:09Z

packages/stryker/src/initializer/StrykerInitializer.ts

-      await this.fetchAdditionalConfig(npmDependencies));
-    this.installNpmDependencies(npmDependencies, selectedPackageManager);
+    const selectedPreset = await this.selectPreset();
+    if (selectedPreset) {


Could we introduce a function here in the then branch?

nicojs · 2018-11-22T19:13:38Z

packages/stryker/src/initializer/StrykerInitializer.ts

+      this.installNpmDependencies(selectedPresetInstance.dependencies, selectedPackageManager);
+    }
+    else {
+      const selectedTestRunner = await this.selectTestRunner();


Idem for this else branch

nicojs · 2018-11-23T07:09:55Z

packages/stryker/src/initializer/StrykerPresets.ts

+import PresetOption from './PresetOption';
+
+const strykerPresets: PresetOption[] = [
+        {name: 'angular', create() { return new AngularPreset(); } },


Could we remove this second layer? Seems complicated. Couldn't we create an array of StrykerPreset classes here?

export default const strykerPresets = [ new AngularPreset(), new ReactPreset(), new VueJsPreset() ];

We could add the name property to the StrykerPreset class as an abstract property.

The idea was to preserve memory by only instantiating a preset once it's needed. Hence the arrow functions.

I understand, but you'll save a couple of bytes at most. I would rather have a piece of code that is better to understand and easier to maintain.

Ever heard of the phrase:

Premature optimization is the root of all evil

but you'll save a couple of bytes at most.

Fair point. 😅
I implemented your suggestion here.

nicojs · 2018-11-23T07:10:32Z

packages/stryker/src/initializer/presets/AngularPreset.ts

+        'stryker-typescript',
+        'stryker-html-reporter'
+    ];
+    public conf = `{


Can we add a reference to this conf object to keep it in sync with the handbook?

Same goes for the other presets

A better idea is to add a link to the specific handbook article at the top of the configuration file (in comments). That way, people know where to find more information.

nicojs · 2018-11-23T07:13:49Z

packages/stryker/src/initializer/presets/StrykerPreset.ts

@@ -0,0 +1,9 @@
+abstract class StrykerPreset {
+  public abstract dependencies: string[];
+  public abstract conf: string;


I don't like this api. There is an implicit dependency between the prompt() method and the conf and dependencies properties. As in, you need to call prompt before you can read the properties. Can we make the configuration and dependencies a return value of prompt. Maybe we shouldn't call it prompt anymore, because it does more. Maybe call it configure? I'm open to suggestions.

nicojs · 2018-11-23T07:16:48Z

packages/stryker/test/helpers/StrykerPresetMock.ts

@@ -0,0 +1,6 @@
+import StrykerPreset from '../../src/initializer/presets/StrykerPreset';
+
+export class StrykerPresetMock extends StrykerPreset {


Can this class reside in the StrykerInitializerSpec? Seems that it is only used there. I don't want helpers to get cluttered with mocks that are only used once.

nicojs · 2018-11-23T07:17:15Z

packages/stryker/test/unit/initializer/StrykerInitializerSpec.ts

@@ -103,6 +116,77 @@ describe('StrykerInitializer', () => {
      expect(promptPackageManagers.choices).to.deep.eq(['npm', 'yarn']);
    });

+    it('should immediately complete when a preset and package manager is chosen', async () => {


Great tests here! 👍

…e need for PresetOption interface

Wmaarts · 2018-11-23T14:19:32Z

Pushed some changes that were suggested, but I'm not sure why the build is failing now.

nicojs · 2018-11-27T09:06:21Z

@Wmaarts

I'm not sure why the build is failing now.

I don't know either. I've merged master and pushed to your branch. New build is running.

EDIT: found the issue. stryker-api was no longer a dev-dependency of stryker. As such, lerna decided to run the compilation of typescript code in parallel. This broke the build, as stryker needs the types of stryker-api.

* `StrykerPreset` -> `Preset` * `StrykerPresetConf` -> `PresetConfiguration`

nicojs · 2018-11-27T11:12:04Z

I've committed a few more changes. Namely renamed some stuff, changed the abstract classes to interfaces where appropriate and added the handbook url to the outputted stryker.conf.js file.

nicojs · 2018-11-27T11:30:39Z

Thanks again for your help @Wmaarts

Wmaarts added 3 commits November 19, 2018 14:48

Dependency changes

532b7de

Prompt for selecting preset config, with simple tests

0ec20a8

Refactoring and reworking prompting preset logic

0a9ac36

ghost added the 🔎 Needs review label Nov 22, 2018

Reincluded stryker.conf.js in stryker package after accidentally dele…

69abf2a

…ting it

nicojs requested changes Nov 23, 2018

View reviewed changes

Wmaarts added 4 commits November 23, 2018 09:27

Merge remote-tracking branch 'upstream/master'

491100f

Remove unnecessary dependencies

c4d57fd

Refactoring of StrykerPreset api.

e3e9518

Refactor StrykerPreset list to remove arrow function. Also removes th…

bdbb16a

…e need for PresetOption interface

Wmaarts and others added 2 commits November 23, 2018 16:36

Preset-specific test cases for Angular, React and VueJs

530bf49

Merge branch 'master' into feat/support-presets

dc4f84c

ghost assigned nicojs Nov 27, 2018

nicojs added 5 commits November 27, 2018 10:15

Reset stryker-api as a dev dependency of stryker

79cfe60

Rename some things

0715195

* `StrykerPreset` -> `Preset` * `StrykerPresetConf` -> `PresetConfiguration`

Add handbook url to the stryker configuration file

2c7e8ab

Fix typing of the prompPresets returned promise

520f39e

Format files using editorconfig

d8bff5c

nicojs approved these changes Nov 27, 2018

View reviewed changes

nicojs merged commit 5673e6b into stryker-mutator:master Nov 27, 2018

ghost removed the 🔎 Needs review label Nov 27, 2018

bharaninb mentioned this pull request Nov 27, 2018

Support "presets" during stryker init #1049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Stryker CLI 'init'): Support for preset configuration during 'stryker init' #1248

feat(Stryker CLI 'init'): Support for preset configuration during 'stryker init' #1248

Wmaarts commented Nov 22, 2018

nicojs left a comment

nicojs Nov 22, 2018

Wmaarts Nov 23, 2018

nicojs Nov 22, 2018

Wmaarts Nov 23, 2018

nicojs Nov 22, 2018

Wmaarts Nov 23, 2018

nicojs Nov 22, 2018

nicojs Nov 22, 2018

nicojs Nov 23, 2018

Wmaarts Nov 23, 2018

nicojs Nov 23, 2018

Wmaarts Nov 23, 2018 •

edited

Loading

nicojs Nov 23, 2018

nicojs Nov 23, 2018

nicojs Nov 23, 2018

nicojs Nov 23, 2018

nicojs Nov 23, 2018

nicojs Nov 23, 2018

Wmaarts commented Nov 23, 2018

nicojs commented Nov 27, 2018 •

edited

Loading

nicojs commented Nov 27, 2018

nicojs commented Nov 27, 2018

		@@ -0,0 +1,6 @@
		import StrykerPreset from '../../src/initializer/presets/StrykerPreset';

		export class StrykerPresetMock extends StrykerPreset {

feat(Stryker CLI 'init'): Support for preset configuration during 'stryker init' #1248

feat(Stryker CLI 'init'): Support for preset configuration during 'stryker init' #1248

Conversation

Wmaarts commented Nov 22, 2018

Implementation of #1049

Small disclaimer

Summary

Implementation

Where?

Adding a new preset

Shortcomings

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

Wmaarts Nov 23, 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

Choose a reason for hiding this comment

Wmaarts commented Nov 23, 2018

nicojs commented Nov 27, 2018 • edited Loading

nicojs commented Nov 27, 2018

nicojs commented Nov 27, 2018

Wmaarts Nov 23, 2018 •

edited

Loading

nicojs commented Nov 27, 2018 •

edited

Loading