Exclude mutation types about stryker-js HOT 13 CLOSED

This feature request seems to be a duplicate of #293.

from stryker-js.

I wonder if it makes sense to exclude/disable/enable mutation types by a system similar to the rules configuration of tslint (See https://palantir.github.io/tslint/usage/configuration/)

Example:

{
  mutations: {
    StringLiteral: false,
    Block: false
  }
}

Th advantage of this system

• Familiar to tslint users
• Can be easily extended in the future to configure rules in a more complex way. This can be useful in cases where people disagree about what should be how mutated (Example issue #465)

Example:

{
  mutations: {
    StringLiteral: {
      relaceExportedConst: false,
      replacements: ["", "<script>window.alert('Evil Hack!')</script>"]
    },
  }
}

from stryker-js.

@simondel @nicojs Is there any ongoing work in the direction of this ticket? Otherwise I would like to volunteer to implement it, at least for the TypescriptMutator. For me it seems that in the current design, the class TypescriptMutator is responsible for knowing all typescript "child" mutators and for executing them.
Therefore I think the best place to disable/enable/configure the "child" mutators is the constructor of https://github.com/stryker-mutator/stryker/blob/5dd8be70f0ec53152837abae6145214b9d06d364/packages/stryker-typescript/src/TypescriptMutator.ts#L24

In the long run, it might be better to do this work on a higher level, but I'm particular interested in TypeScript and it could be a good start.

from stryker-js.

There are plans to implement this, but it's not at the top of our todo-list. I would be a fan of working with a blacklist that you could set in your config. That way when we release a new mutation type, you will actually start using it.

Perhaps we could simply work with an array property that is added to the config:

excludedMutations: ['ArrayNewExpression', 'PrefixUnaryExpression'],

The names of the excluded mutations should be exactly the same (case and all) with the reported value:
https://github.com/stryker-mutator/stryker/blob/06d6bace999265ac6a8d0ae98ba6f6413cc3a1e0/packages/stryker-api/src/report/MutantResult.ts#L6
That way you could always look at your report and decide what you want to exclude.

The constructor you found is used for test purposes. A more solid option might be to filter the NodeMutator[] with the list in the config when the mutate function is called.

@nicojs What you think? PS why is the mutators property private in the TypeScriptMutator?

from stryker-js.

@simondel So you mean to filter in https://github.com/stryker-mutator/stryker/blob/5dd8be70f0ec53152837abae6145214b9d06d364/packages/stryker/src/Stryker.ts#L70 mutantName against the string array in excludedMutations?

Advantages:

• More generic (Works for all Mutators)
• Simpler
• Excluded Mutations could be reported if required
• Works better as commandline option (https://github.com/stryker-mutator/stryker/blob/master/packages/stryker/README.md#configuration)

Drawbacks:

• Mutants are created and than ignored. So it's not possible to disable the mutator but just ignore it's result. I guess performancewise the difference should be minimal, compared to the actual test ececution? What if a Mutator should be disable because it e.g. throws Exceptions?

Advantages/Drawback:

• Excluding mutations and a (potential future) configuration of mutations is handled separate/orthogonal

So your approach has many advantages, but I have to add that my approach could also work as a blacklist (If a mutation is not listed in mutations: {}, it's enabled by default.).

from stryker-js.

I actually meant filtering inside the mutate function in a mutator, but filtering in Stryker would not even be a bad idea!

I'm just not a big fan of doing a whole lot of magic in a constructor.

from stryker-js.

I actually meant filtering inside the mutate function in a mutator, but filtering in Stryker would not even be a bad idea!

Agreed. It has a big advantage as we would only have to implement it once for all current and future mutators. Generating mutants only takes a fraction of the total mutation testing time, so it's not a big performance hit to first generate all mutants. I also like the idea of logging how many mutants are ignored based on your configuration. Let's proceed with this approach.

Otherwise I would like to volunteer to implement it

@shybyte that's awesome! The floor is yours! If you need any help, don't hesitate to ask.

I wonder if it makes sense to exclude/disable/enable mutation types by a system similar to the rules configuration of tslint.

I like this approach. It looks clean and leaves room for more configuration, like @shybyte said. All mutators are enabled at the start, so it will effectively be a black list for now. Later on we could introduce new mutators with default enabled = false for performance reasons (PIT also takes this approach). @simondel would you also agree with this?

from stryker-js.

Let's do it!

from stryker-js.

@simondel @nicojs How should the new config property be named??

• mutations
• mutationTypes
• mutators
• mutants

???
In the report it seems be named Mutator. There is already a config option mutator for the "parent" mutator. This could mean that mutators is the correct name. It could also mean that mutators would be confusing (just a 's' more)...
Actually I'm a bit confused about the correct terminology.

from stryker-js.

Yeah it's quite confusing. I'd say we go with excludedMutations. @nicojs Agreed?

from stryker-js.

@simondel Your comment raises again the question if we implement the

• {excludedMutations: ['name', ...]}
• or the
• {mutations: {name: false}}

approach???
I personally have no particularly strong opinion in this question. The biggest advantage of the excludedMutations approach is probably, that it can be more easily used from the command line. Aside from this, I see more flexibility in the mutations approach.

@nicojs What do you think?

from stryker-js.

I've got an idea.

How about we allow this configuration:

{
   mutator: { 
      kind: 'typescript',
      mutations: {
         block: false,
         stringLiteral: { // example of a future config
            replaceExportedConst: false,
            replacements: ["", "<script>window.alert('Evil Hack!')</script>"]
         } 
    }

I think the intent here looks really clear. You would still be able to just put the kind as string as well: mutator: 'typescript'.

from stryker-js.

Fixed with #652

from stryker-js.