Clarification and rectification for the 'IgnoreMethod' filtering logic

[dupdob]

Context

This is a followup of issue #2653 where I realized that existing unit tests (for the IgnoreMethodFilter) were non representative.
I have been working on improving on the test, but I often face results I deem illogical.
The purpose of this discussion is to improve/narrow the expected behavior of this filter and improve doc and code accordingly.

Why

Because the question turns to be subtler than one could think at first: what means 'ignoring a method' ?

Analysis

The purpose of this option is to help users (of Stryker) ignore 'en masse' mutations they can not (or do not intend to) test for. An often given example is application logging, which is hard to test and rarely, if ever, tested. This happens also with console output (Console.Write) or concurrency related test (ConfigureAwait).

These methods share all an important characteristic: calling them result in some side effect(s) that is difficult to test for.

Hence I propose a refined description of the Ignore filter: it ignores mutations impacting only (side) effects from ignored methods.

Concrete examples

For a given ignored method M.

For expression mutations:

1. Mutations on parameters on an invocation of M must be ignored : such as M(true) to M(false). This is already the case in V 3.10.
2. Mutations happening before (i.e on the left of) an invocation., such as cond ? M(true) : M(false) => cond ? M(false) : M(true). This is not the case in V 3.10
3. Mutations happening after (i.e on the right of) an invocation., such as M() ? 1 : 2 => M() ? 2 : 1. This is not the case in V 3.10

For statement mutations:

1. Statement expressions (e.g x = M()) should follow Expression rules. This is the case in V 3.10.
2. Declaration statement (e.g var x = M()) should follow Expression rules. This is not he case in V3.10
3. Generalization: a statement mutation should be ignored if its expression(s) mutations should be ignored

For block mutations

1. Block statements should be ignored if every block's statement mutations should be ignored

[richardwerkman]

Hi, I get why this is a point of discussion. Here is my view on this: When ignoring a method I expect the arguments passed to the method to be skipped from mutation. However, the return value of the invocation can be part of a mutation.

I think your examples are a bit vague, as some of the mutations aren't even valid mutations (stryker doesn't swap expressions in conditional ternary expressions). I'll give some examples of what I expect to happen:

Original	Mutated	Ignored
IgnoredMethod(true)	IgnoredMethod(false)	yes
x = IgnoredMethod();	removed	yes
var x = IgnoredMethod();	removed	yes
x == IgnoredMethod()	x != IgnoredMethod()	no
IgnoredMethod()++	IgnoredMethod()--	no
x == 1 ? IgnoredMethod(true) : IgnoredMethod(false)	x != 1 ? IgnoredMethod(true) : IgnoredMethod(false)	no

So in short: The invocation of the ignored method should stay exactly like it is, it shouldn't be removed and the arguments shouldn't be mutated. But side effects from the result of the ignored method should be mutated.

[dupdob]

Thanks for your feedback

I think your examples are a bit vague, as some of the mutations aren't even valid mutations (stryker doesn't swap expressions in conditional ternary expressions).

Sorry if they are vague, but all those are valid mutations in the general sens. How the filter behave should not be dictated by existing or upcoming mutators 😄 .
Thanks the table, definitely a more apt format:

Original	Mutated	Ignored (for me)
IgnoredMethod(true)	IgnoredMethod(false)	yes
x = IgnoredMethod();	removed	yes
var x = IgnoredMethod();	removed	yes
x == IgnoredMethod()	x != IgnoredMethod()	no
IgnoredMethod()++	IgnoredMethod()--	no
x == 1 ? IgnoredMethod(true) : IgnoredMethod(false)	x != 1 ? IgnoredMethod(true) : IgnoredMethod(false)	yes (1)
SomeMethod(true).IgnoredMethod(true)	SomeMethod(false).IgnoredMethod(true)	yes (2)

Here is my rationale

1. for ternary operator, or switch expressions, the rule should be: ignore if every outcome expression is to be ignored.
2. mutation happening in the member access expression should be ignored as they are equivalent to mutating this, which should be considered as a parameter

[dupdob]

The problem with this definition is that statement and block mutator are not ignored then, as they have larger scope

[rouke-broersma]

True, I think that's a separate issue tbh. The block and statement mutator should not be active on scopes that contain any mutation that is ignored except if this mutations are ignored due to either mutation level or ignore mutation. There's no good reason for them to be active when there are mutations but they have been explicitly ignored.

[dupdob]

I do not think so: what if the only active mutator is the statement or block mutator?

[rouke-broersma]

The more explicit filter (will have in theory) precedence so the mutations will be marked as ignored by ignore-method and the block and statement mutator will respect that (in the future).

[dupdob]

coming back to this: I share this view. But the net result is that we have to ignore 'larger scope' mutation when they only impact ignored methods. The canonical example is logging: automated testing of logging is useless 99% of the time. So any mutation that only impact logging should be ignored.
May be the most famous example is :

If (LoggingEnabled)
    _logger.Log(...);

As of today, Stryker will mutate the if, resulting in unkillable mutants. The user can use Stryker comments to skip those, but this may be a lot of work.
This may be hard work to achieve (I do not think so), but this makes sense.

On the other hand, I fail to find any example where a similar mutation (with some other to be ignored methods) would have any value to be kept.
Is there any counter example of such a construct where one would want it to be mutated nonetheless?

[dupdob]

Just added a unit test for those examples.

Original	Mutated	Ignored (expected)	Actual
IgnoredMethod(true)	IgnoredMethod(false)	yes	yes
x = IgnoredMethod();	removed	yes	no
var x = IgnoredMethod();	removed	yes	no
x == IgnoredMethod()	x != IgnoredMethod()	no	no
IgnoredMethod()++	IgnoredMethod()--	no	no
x == 1 ? IgnoredMethod(true) : IgnoredMethod(false)	x != 1 ? IgnoredMethod(true) : IgnoredMethod(false)	yes	no
SomeMethod(true).IgnoredMethod(true)	SomeMethod(false).IgnoredMethod(true)	yes	no

While I was expecting the last two to fail, I did not expect the simple 'x = IgnoredMethod();' to fail.
This may be a bug in the test logic. Looking into it. Confirmed. This is a Stryker bug.