Stryker: Exclude specific mutations

[simondel]

In some projects, there are mutations that users want to exclude. An example of this is logging. We've had multiple questions of users that do not want to test (some) logging.

Why?

1. Not all code is worth testing (using unit tests)
2. Wanting to maintain a 100% mutation score and be alerted of any new surviving mutants

Why not?

1. Excluding mutations gives a false sense of quality: "I've tested 100% of my code, except for the code I haven't tested"
2. If it's not important to test a piece code, you may not find that piece of code important at all, so removing it would be preferred.
3. Adds complexity to the usage and development of Stryker

Proposed implementation

We can use inline comments to disable certain mutators or disable Stryker entirely. For example:

• // stryker-mutator disable next or /* stryker-mutator disable next */: this would disable all mutators Stryker on the next AST node (statement)
• // stryker-mutator disable next ['StringLiteral'] or /*stryker-mutator disable next ['StringLiteral']*/: this would only disable the StringLiteral mutator for the next AST node (statement).
• We might also want to allow people to exclude some patterns via stryker configuration. First, we would need to implement basic ignoring.

During the mutating process, we could keep track of the comments. We should _still_ generate the mutants, but output them as "MutantState.Ignored" (new state) so it can be reported accordingly by different reporters.

Timeline

We want to introduce a parser API to support an easy and safe way to make the exclusions. This issue will lay the groundwork for it: #1893. Before this is done, we will not support this option.

_This main post will be kept up to date with the latest info_

Related issues:

1174, #1470, #1464, #1653

Related pull requests:

1810, #1912, #2084

[Djaler]

This is very important to me. I have so many props in my vue components library. Example

export default Component extends Vue {
...
    @Prop({type:String})
    value: string
...
}

And Stryker applies ObjectLiteral mutator to prop definition. I do not need to test this case, because this is just a some kind of type definition.

[nicojs]

Note: mutation switching is tracked in #1514. Once we have that (and one parser and AST), this feature should be easier to implement. Let's keep this in mind when we implement #1514

ObjectLiteral mutator to prop definition

How would you prefer to exclude these? I would guess that you don't want to add // stryker-mutator ignore next everywhere in your code.

[Djaler]

How would you prefer to exclude these? I would guess that you don't want to add // stryker-mutator ignore next everywhere in your code.

Yeah, it would be cool to exclude such cases by regexp. Or by custom filter function that will take AST node as argument with possibility to check parent nodes

[kmdrGroch]

@nicojs, @simondel I am working on this and it looks quite nice for now :)

[simondel]

Nice one @kmdrGroch! That looks promising! What is your end goal regarding syntax and features?

[kmdrGroch]

@simondel I am going to make features like these:
stryker-enable (or stryker: enable) [I don't know yet since I want the easiest to debug/write]
stryker-disable (same pattern everywhere)

these 2 are to enable/disable globally (In a current file of course)

these 2 are fully implemented right now + stryker: disable-next (only for 1 mutation)

I have also been working on disabling the first occurrence of given mutator like stryker: arrayMutator, stringMutator etc. (of course it works for more than 1 like I showed)

Any feedback is nice

[leobenkel]

Maybe you could use the same syntax as ScalaStyle : http://www.scalastyle.org/configuration.html

// scalastyle:off magic.number
var foobar = 134
// scalastyle:on magic.number

[kmdrGroch]

looks nice !

Maybe you could use the same syntax as ScalaStyle : http://www.scalastyle.org/configuration.html

// scalastyle:off magic.number
var foobar = 134
// scalastyle:on magic.number

[kmdrGroch]

So after rethinking it I think I will make options like:

// stryker:on - globally switches mutations on
// stryker:off - globally switches mutations off

// stryker:on ArrayLiteral
// stryker:off ArrayLiteral, ArrayNewExpression, ...
(switches on / off specific type of mutations [using stryker:on/off will reset these too. //stryker:on used after //stryker:off ArrayLiteral, ArrayNewExpression will switch on all mutations again.
]).

// stryker:next-on
// stryker:next-off
these two will enable / disable next mutation (note: even after using stryker:off //stryker:next-on will cause mutant creation!)

[kmdrGroch]

@nicojs, @simondel, any thoughts?

[Mobrockers]

@nicojs We have had 'ignoring' mutators for a while now. We mark the mutations as 'skipped' status instead of ignore. We should probably agree on using the same status name to indicate excluded mutations.

[tsachis]

@simondel @nicojs @kmdrGroch @Djaler
I've added an option to exclude code patterns via configuration (#1810)
Would appreciate any feedback.

Usage:

mutator: {
  name: 'typescript',
  excludedExpressions: [
    'logger.debug',
    'LoggerFactory.',
    '.propTypes = '
  ]
}

[simondel]

Thanks for taking the effort to work on this highly requested feature @tsachis ♥️

Keep in mind that a user-facing API has not yet been finalized. This will most likely mean rework for your PR, or it could mean that your PR will not be merged at all because the way your solution works may be radically different from the defined requirements.

[kmdrGroch]

considering exact post made by @simondel (mean this issue), I think this branch is the closest one to solve it: https://github.com/kmdrGroch/stryker/tree/fix-1472. Tho I had some problems with typescript part (Javascript part works just fine)

[tsachis]

@kmdrGroch Using inline comments to exclude mutations is a very useful feature.
However, IMO it does not cover the use case of excluding patterns in your entire codebase (e.g. logger.info, propTypes).

Adding a way to globally exclude patterns is another important feature that can cover that use case, and help users improve process performance and filter out noise from the output.

@simondel the above PR is just a POC. I understand the final API can be completely different, and I would be happy to contribute and push this feature forward.

[Ranguna]

Thank you for the awesome work on this but there's no activity in the PR for the last 3 months, is there any updates on this ?

[kmdrGroch]

Sorry, i hadn't got that much time and I won't have in next few months probably. You can freely use resources from my PR, unless you can wait longer...

[Ranguna]

No worries. We are waiting for this so we can start using mutation testing in our projects.
I'll wait a few more months :+1:

[matthias-pichler-warrify]

For me the real problem is that stryker just screws with my log statements and 50% of all mutants that survive are just StringLiteral or ObjectLiteral replacements for my logger which drastically reduces the value stryker provides when you have to sift through 100s of meaningless test output.

[kmdrGroch]

@warrify Mutation score opposing test coverage, doesn't have to be 100%. Sometimes you need more code (which seems to be unnecessary) just to make something faster for some cases (and code works without it but slower). Mutation tests are rather for development so you can see if there is something to correct / add new test / remove. Of course we want this feature, don't get me wrong, but just to remind you - 100% isn't a must ;)

But from my experience, unless your code is untestable or is optimisations, you should be able to get 100%

[matthias-pichler-warrify]

@kmdrGroch Thanks for your reply. My point was not really about the score. We don't strive for 100% score nor coverage. But to clarify what I meant by reducing value:
(I will keep the examples specific to the case of the logger though to keep them realistic)

This logger statement results in 2 additional mutans (StringLiteral & ObjectLiteral)

logger.warn("Something happened", { foo: reason })

When you have a handful of tests which are run for every mutant and half your mutants are basically "meaningless" you just waste time and, in case of a CI System, money.

Also when something takes minutes longer to run it will also be run much more infrequent by developers. Same as with regular test suites.

And when the run is finally over, you want to go through the list and see if anything for your tests could be improved etc. But the clear-text reporter would overflow the terminal so half the output is lost and all you see are those damn logger statements. So you have to write the outputs to disk and then have to go through a file with a couple of thousand lines hoping that you see anything meaningful which is buried between meaningless survival reports.

So in reality this just makes it significantly harder to gain anything from mutation testing yet to improve upon the insights.

[kmdrGroch]

we could think of making so we ignore phrases starting from sth, like console or logger... To be honest, it is pretty easy for javascript-mutator, but I had big problems with typescript :c
Unfortunately for you, I think we will try solving this problem after adding mutation-switching introduced in https://github.com/stryker-mutator/stryker/issues/1514 since it will be a bit breaking.

Cheers

[Ranguna]

Looking at the checklist of #1514, it's going to take quite a bit to get this into a release.

Hopefully I won't create many mutants by the time I can use stryker.

Keep it up :muscle:

[kmdrGroch]

@Ranguna if it is super necessary for your project, I could try implementing it specifically for your use (but I need to know which mutator you are using and what features you want), but it wouldn't be supported by Stryker officially.

[Ranguna]

Thank you for the incredible support @kmdrGroch!

It's not really super necessary. Stryker would just be a nice thing to have but we can live without it for the time being.

Unfortunately I'll be using the mutator that you've been having problems with, the typescript mutator. My use case would be something like this:

function a(realArgument: string, _probablyUnusedArgumentForDebuggingAndSuch: string) {
  // realArgument is used
  // but, _probablyUnusedArgumentForDebuggingAndSuch might never be used specifically or it's just used to pass a debug string around. So, there's no real impact on the overall implementation of `a`.
}

It would be nice if I could ignore mutations on _probablyUnusedArgumentForDebuggingAndSuch, maybe with a regex on idents like /^_.*$/.

As much as I'm thankful, I don't know if having a specific build for my project would be a good idea since I'm not sure how I'd handle future Stryker (break/conflicting) updates later on.

Again, thank you very much for the support!