Ignore 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

[bartekleon]

@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?

[bartekleon]

@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

[bartekleon]

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

[bartekleon]

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!)

[bartekleon]

@nicojs, @simondel, any thoughts?

[rouke-broersma]

@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.

[bartekleon]

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.

[bartekleon]

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 #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 💪

[bartekleon]

@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!

[kholvoet]

Another reason is the sometimes there is more than one way to get the same value. Consider:,

   static mapColor(n: number): number {
        if (n > 0 && n < 1) {
            return Math.round(255 * n);
        }
        return n > 0 ? 255 : 0;
   }

Here. the boundary values 0 an 1 both 'work' through 2 paths, so one mutation (> to >= or < to <=) can't actually force a failure. With some herculean effort (due to typescript and legacy testing code), I could maybe spy on the standard lib, to determine which path and 'error' in the test if the 'wrong' path is taken, but really, the point is that 0 -> 0, and 1 -> 255. It doesn't matter to me which rule is uses. So being able to say 'ignore this' isn't putting me at risk I am unwilling to take.

[bartekleon]

I guess that could count as equivalent mutant. We covered it in documentation :)

[nicojs]

It doesn't matter to me which rule is uses. So being able to say 'ignore this' isn't putting me at risk I am unwilling to take.

Yes, it's covered here: https://stryker-mutator.io/docs/mutation-testing-elements/equivalent-mutants/

It is a valid case for exclusion of the mutant, we should be able to cover this feature after the v5 release (which will be ~May)

[quoininc-huudatran]

If not too late, I see the project already uses eslint, so maybe use the same syntax?

// eslint-disable-next-line rule1,rule2

I basically have the same issue as logging, but with the debug package.

[nicojs]

@rouke-broersma what syntax are you guys using to ignore mutants?

[rouke-broersma]

@rouke-broersma what syntax are you guys using to ignore mutants?

stryker-mutator/stryker-net#1583

[nicojs]

Thanks @rouke-broersma

In short, syntax for Stryker.NET:

// Stryker [disable|restore][once][all| mutator list][: reason for disabling] where

* **disable**: disable some or all mutators. This action is valid until:
  
  * end of scope (i.e. closing brackets
  * explicit _enable_comment

* **restore**: re enable some or all previously disabled mutators. Note:
  
  * this does not override overall Stryker settings, one cannot enable a mutator disabled through general settings.
  * no error is generated if one enables a mutator not disabled by an earlier comment

* **once**: the action is limited to the following statement or expression. Valid both disable or enable

* **all**: action is for every mutator

* **mutator list**: comma separated list of mutators. you must use the mutator name, not is description. E.g.: `arithmetic, statement`

* **reason for disabling**: comment that will used as status detail for why the mutation has been ignored.

This is pretty extensive, but we can choose to start by implementing parts of it.

NB: I think once is a bit odd. I would assume that // Stryker disable once StringMutator would ignore the following string, but not the string after that. Instead, it is specified here as ignoring string mutants on the next statement or expression. I personally feel like "next-line" (like eslint does) is more clear here.

[rouke-broersma]

The feature has not yet been released. You can discuss the specific semantics with @dupdob as I personally don't mind either once or next.

[dupdob]

I picked once because it is used by Resharper for a similar feature (see this section). Since Resharper is a popular tool, it made sense to reach for familiarity.
Indeed, once does not capture the behavior, as it may ignore between 0 (not needed) and n mutants (complex expression). My guesstimate is that it will ignore one mutant 90% of the time, so that may be an acceptable inaccuracy.

I though a lot about this, and next is not perfect either:

• next is kinda like once, as such it does not convey that it could be for multiple mutations
• next-line may look better, but the truth is that disable once works for the whole syntax element that this comment is linked to. It can be a whole method if it is on the method signature, or a single argument, if it is placed just before it.

In any case, changing this is not difficult. We could even accept both forms, assuming the meaning is the same.

One last thing: trying to implement a 'next-line-only' feature would require a lot of work, as in C#, like in other C derived languages, eol is roughly equivalent to a whitespace, syntax-wise, meaning we would have to look for it especially and not rely on syntax parsing.

[nicojs]

This feature got released in StrykerJS 5.4. See https://stryker-mutator.io/docs/stryker-js/disable-mutants#using-a--stryker-disable-comment for information.

Examples:

function max(a, b) {
    // Stryker disable next-line EqualityOperator
  return a < b ? b : a;
}

// Stryker disable all
function max(a, b) {
    return a < b ? b : a;
}
// Stryker restore all
function min(a, b) {
  return a < b ? b : a;
}

// Stryker disable EqualityOperator,ObjectLiteral: We'll implement tests for these next sprint
function max(a, b) {
  return a < b ? b : a;
}

// Stryker disable all
function max(a, b) {
  // Stryker restore EqualityOperator
  return a < b ? b : a;
}

[vitalets]

Nice to see this feature!
Although I think it would be better to make this comment look more "technical": stryker-disable-next-line.
It allows to distinguish it from usual comments and aligns with disabling comments of other tools, e.g. eslint.

function max(a, b) {
  // stryker-disable-next-line EqualityOperator
  return a < b ? b : a;
}

vs

function max(a, b) {
  // Stryker disable next-line EqualityOperator
  return a < b ? b : a;
}

[nicojs]

I personally think the comment is technical enough. You would have to write pretty strange comments if you accidentally write an ignore comment...

It's also something we need to align on with Stryker.NET, Stryker4s and other future Stryker versions and they're already using this syntax 🤷‍♂️

[adarshnd14]

Nice to see this feature!

But commenting whenever we use certain pattern would be a hard task to do particularly in case of large repo with recurring pattern. Do we have any option in configuration that will enable us to ignore certain patterns. Like disabling objectLiteral mutant where .propType is defined.

We can consider the same example in this issue #1653