feat(ignore mutation): Add support for ignoring mutations in code using mutiline string comments

[dupdob]

Add support for the so called multiline comments format (/* comment */) for Stryker comments. Updated the doc and provided some detailed explanation for once option. Note that the Stryker comment must still be on a single line.

Multiline comments should work exactly line single line comments when put on a dedicated line.

Stryker attempts to associate comments within the line with the previous syntax node, that is:

if (condition) {/* Stryker comment*/;....}: this comment will apply to the whole syntax block if (/* Stryker comment*/ condition) {....}: this comment will apply to the whole condition expression

Stryker comments may not be detected if they are within a syntax node if (cond/* Stryker comment*/ition) {....}: will have no effect if (condition /* Stryker comment*/) {....}: will have no effect

Fixes #3008

[nwalker-et]

I'm also having a hard time getting the new tests to pass when I run them locally, though it seems like they're passing in the CI/CD pipeline. Is there something I'm missing in my local setup? In case it helps, I'm getting errors like the following:

  Failed ShouldNotMutateIfDisabledByMultilineComment [14 ms]
  Error Message:
   Test method Stryker.Core.UnitTest.Mutants.CsharpMutantOrchestratorTests.ShouldNotMutateIfDisabledByMultilineComment threw exception:
Shouldly.ShouldAssertException: isSame
    should be
True
    but was
False

Additional Info:
    AST's are not equivalent. Line[9]
actual:    if ((StrykerNamespace.MutantControl.IsActive(2)?!(condition && other):(StrykerNamespace.MutantControl.IsActive(1)?condition || other:condition && other))) /* Stryker disable once all */ {
expect:    if ((StrykerNamespace.MutantControl.IsActive(0)?!(condition && other):(StrykerNamespace.MutantControl.IsActive(0)?condition || other:condition && other))) /* Stryker disable once all */ {
Actual(full):
using System;
using System.Collections.Generic;
using System.Text;
...

[dupdob]

I'm also having a hard time getting the new tests to pass when I run them locally, though it seems like they're passing in the CI/CD pipeline. Is there something I'm missing in my local setup? In case it helps, I'm getting errors like the following:

  Failed ShouldNotMutateIfDisabledByMultilineComment [14 ms]
  Error Message:
   Test method Stryker.Core.UnitTest.Mutants.CsharpMutantOrchestratorTests.ShouldNotMutateIfDisabledByMultilineComment threw exception:
Shouldly.ShouldAssertException: isSame
    should be
True
    but was
False

Additional Info:
    AST's are not equivalent. Line[9]
actual:    if ((StrykerNamespace.MutantControl.IsActive(2)?!(condition && other):(StrykerNamespace.MutantControl.IsActive(1)?condition || other:condition && other))) /* Stryker disable once all */ {
expect:    if ((StrykerNamespace.MutantControl.IsActive(0)?!(condition && other):(StrykerNamespace.MutantControl.IsActive(0)?condition || other:condition && other))) /* Stryker disable once all */ {
Actual(full):
using System;
using System.Collections.Generic;
using System.Text;
...

You should pull again the code. I usually amend commits to fix test errors and force push the PR. I do not expect people to pull the code until the factory is green and the PR is ready (and not draft).

[nwalker-et]

You should pull again the code. I usually amend commits to fix test errors and force push the PR. I do not expect people to pull the code until the factory is green and the PR is ready (and not draft).

Thanks, that makes sense! I was just wanting to test out some of my own suggestions, so I'll keep that in mind if I run into the issue again.

Stryker comments may not be detected if they are within a syntax node if (cond/* Stryker comment*/ition) {....}: will have no effect if (condition) /* Stryker comment*/ {....}: will have no effect

The wording here might be worth clarifying. In most cases, if a Stryker comment is within a syntax node, it may not apply to that syntax node, but it will often apply to at least one of that syntax node's descendants. In your first example, it might only apply to part of the condition expression, and perhaps not the part one would expect (unless it came before a parenthesized expression). In the second example, it wouldn't apply to the if-statement, but it would apply to the if-statement's block, since that is its own syntax node.

More nuanced thoughts on when Stryker comments might not be detected at all...

With the new logic in this PR, I believe a Stryker comment would only go *undetected* if either a) it is leading trivia for a token that does not appear first within its parent syntax node, or b) it is trailing trivia for a token that is not immediately followed by the start of a new syntax node. So, for example, the Stryker comments in the following code would not be detected at all: ```csharp // Condition a) SomeMethod( true /* Stryker disable all */, 10); // The Stryker comment is leading trivia for the `,` token, which is a middle child of the ArgumentList syntax node. // Condition b) SomeMethod(true, 10 /* Stryker disable all */); // The Stryker comment is trailing trivia for the `10` token, and the token immediately after it, `)`, is not the start of a syntax node. ``` In either case, the comment would have no effect, and in my opinion, this would be the expected behavior given how Stryker currently handles scope. I haven't thought of any scenarios where one would expect different behavior when at least one of the two conditions I mentioned holds true, so I think the logic in this PR should work just fine.

[dupdob]

Stryker comments may not be detected if they are within a syntax node if (cond/* Stryker comment*/ition) {....}: will have no effect if (condition) /* Stryker comment*/ {....}: will have no effect

This doc is wrong here (I fixed it in a recent commit). I wrote it before doing any discovery testing: if (condition) /* Stryker comment*/ {....} actually has an effect (on the if true block or statement)

The wording here might be worth clarifying....

Yes but no.

SomeMethod(true
  /* Stryker disable all */, 
  "");

No effect here, the empty string will be mutated

SomeMethod(true,
  /* Stryker disable all */
  "");

Effect here, the empty string mutation will be ignored. Simply by moving the comma.

You and I and people familiar with AST in general and more specifically Roslyn will find this logical, but most people may just see a code style difference. My view:

1. Documenting this precisely is tricky. As in, the only people who will get what it means are the less likely to need it
2. In practice, this will be indeed a rare occurrence
3. People rarely read the doc
4. Documenting it makes it a feature, while keeping it non documented allows behavior to be adjusted and improved in the future, without being a breaking change

[sonarcloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud