feat(ignore): support disable directives in source code #3072

Garethp · 2021-08-12T11:51:14Z

Add support // Stryker disable and // Stryker restore directives in source code.

Some examples:

// Stryker disable all
const foo = 2 + 40; // mutants here are ignored
// Stryker restore all

// Stryker disable BooleanLiteral
const foo = true || false; // => BooleanLiteral mutant is ignored, but other mutants are tested.
// Stryker restore all

// Stryker disable next-line all
const foo = true || false; // => ignored mutants
const bar = true || false; // => mutants 

// Stryker disable next-line all: I don't care about this piece of code
const foo = bar || baz; // ignored mutants with reason "I don't care about this piece of code"

Ignored mutants do end up in your mutation report, but are not placed or tested and they don't count towards your mutation score.

Closes #1472

…mment"

nicojs

Thanks @Garethp . I have a few remarks, mainly that I want ignore mutants to end up the HTML report with status "ignored", for details see remarks.

Note: we shouldn't forget to add an e2e test for this. Probably it would make sense to add tests to the e2e/test/ignore-project

nicojs · 2021-08-17T06:21:33Z

packages/instrumenter/src/transformers/babel-transformer.ts

@@ -139,6 +197,7 @@ export const transformBabel: AstTransformer<ScriptFormat> = (
   */
  function collectMutants(path: NodePath) {
    return [...mutate(path)]
+      .filter((mutant) => !isMutantInDisabledMap(path.node.loc, mutant))


I don't want the mutants to be filtered out, instead I want them to be collected, but with an ignoreReason set. That way, they will still end up in the report, but won't be placed, tested or influence the mutation score.

nicojs · 2021-08-17T06:24:08Z

packages/instrumenter/src/transformers/babel-transformer.ts

+    comments
+      .filter((comment) => comment.value.trim().startsWith('Stryker disable-next-line'))
+      .forEach((comment) => {
+        const [_, _disableType, mutations] = comment.value.trim().split(' ');


We should also keep track of the ignoreReason. For example:

// Stryker disable-next-line update The update mutator creates an infinite loop here.

The ignore reason we can then report is: "Disabled by user comment. The update mutator creates an infinite loop here."

For this to work, and keep parsing of the comment positional, we would need to force users to use // Stryker disable-next-line all if they want to ignore all, otherwise we cannot tell where the ignore reason starts.

What do you think about this?

It's not my preference, having to type all seems coutner to how I'd expect it to work (Personally I'd expect it to work the way eslint does since we're in a JS context and that's how I kind of expect these things to work in JS). I'd prefer to have have us take the positional argument there and check it against the list of enabled mutations to determine if it's a specific mutant or the start of a comment. I think we'd have to do this anwyay if we want to force the positional argument in order to check if the user accidently left it off.

Hmm good point. But still don't want to leave it up to "chance".

Maybe we can force the mutator names to be in square brackets? Some examples:

// Stryker disable-next-line [update] The update mutator creates an infinite loop here. // Stryker disable-next-line We don't care about log statements // Stryker disable-next-line [update] // Stryker disable-next-line

(the last 2 are without ignore reasons)

If there are no ignore reasons given, we should still ignore them with "Disabled by user comment", or something like that.

… placed

…e all mutants with, expand it into the actual array of all mutations

Garethp · 2021-08-22T00:05:31Z

@nicojs I've made it so that those mutants are marked with an ignore reason when disabled and added some tests to the ignore-project. I've also added the // Stryker disable [mutators] [reason] and // Stryker restore [mutators] [reason] commands. The HTML reporter doesn't show the user-defined reasons as far as I can see, but they do make it into the report and in the HTML viewer you see the comment right above the ignored mutation anyway

nicojs

Thanks for your continued work on this @Garethp. I see you've also managed to implement the disable and restore proposal from #1472 . Great stuff 🎉

I've refactored a bit. I've renamed the tests and moved all disable logic to a separate class: DirectiveBookkeeper. I've also managed to merge all directive matching into 1 regex to rule them all (based on your earlier work with the mutation range regex magic 😊).

One last question: I see we've strayed a bit away from the original design:

Stryker disable [foo] because reasons instead of Stryker disable foo: because reasons
Stryker disable-next-line because reasons instead of Stryker disable next-line all: because reaons
Stryker disable because reasons instead of Stryker disable all: because reasons.

It personally feel like we should keep it more in line with Stryker.NET. We can discus it tomorrow in our community meetup.

nicojs · 2021-08-25T20:41:32Z

e2e/test/ignore-project/verify/verify.ts

@@ -4,7 +4,7 @@ describe('After running stryker on jest-react project', () => {
  it('should report expected scores', async () => {
    await expectMetrics({
      killed: 8,
-      ignored: 13,
+      ignored: 23,


I think we should also verify the ignore reasons now. 🤷‍♀️

nicojs · 2021-08-26T18:18:45Z

packages/instrumenter/test/unit/transformers/babel-transformer.spec.ts

+        `,
+        });
+        act(ast);
+        expect(notIgnoredMutants()).lengthOf(0);


Test for the reason with a custom reason

nicojs · 2021-08-26T18:26:33Z

We've chosen to migrate it to the Stryker.NET syntax

nicojs · 2021-09-01T11:32:36Z

@Garethp I actually found some more bugs and decided to change the implementation because of it. Much better now! See fd5e978

Garethp added 2 commits August 12, 2021 12:44

feature(instrumenter): Allow adding a "disable-next-line [mutator] co…

3513370

…mment"

feature(instrumenter): Adding more tests to cover more use-cases

4a4fbca

nicojs requested changes Aug 17, 2021

View reviewed changes

Garethp added 3 commits August 22, 2021 00:11

feature(instrumenter): Mark mutations as ignored rather than just not…

f54f730

… placed

feature(instrumenter): Adding // Stryker disable and //Stryker restore

79c6d99

feature(instrumenter): Instead of using 'all' as magic value to ignor…

19bbef9

…e all mutants with, expand it into the actual array of all mutations

nicojs added 4 commits August 25, 2021 22:23

test: refactor test names and assertions

a81cae2

refactor(directives): move directive logic to separate class

a68ce36

Remove .only

a0273f1

Merge branch 'master' into disable-mutation-comment

f2427f5

nicojs self-requested a review August 25, 2021 20:29

nicojs requested changes Aug 25, 2021

View reviewed changes

nicojs reviewed Aug 25, 2021

View reviewed changes

nicojs added 2 commits August 26, 2021 13:18

Add 2 more unit tests

ca05ef8

Small code improvement

28e8bb4

nicojs mentioned this pull request Aug 26, 2021

Exclude code with regex #2966

Closed

nicojs reviewed Aug 26, 2021

View reviewed changes

nicojs added 4 commits August 27, 2021 12:57

Use stryker.net syntax

509477f

Merge branch 'master' into disable-mutation-comment

3fa5cdf

Case-insenstivie mutator name comparison

488ff93

Add ignore reason to tests of ignore project

a917435

nicojs changed the title ~~feature(instrumenter): Allow adding a "disable-next-line [mutator] comment"~~ feat(ignore): support ignore directives in source code Aug 27, 2021

nicojs added 2 commits August 27, 2021 17:56

Add support for restore next-line

137cb94

Update implementation with Rule interface

fd5e978

nicojs changed the title ~~feat(ignore): support ignore directives in source code~~ feat(ignore): support disable directives in source code Sep 1, 2021

nicojs added 2 commits September 1, 2021 13:26

document disable mutants

ab935dd

Fix compile error

7ba09c0

nicojs enabled auto-merge (squash) September 1, 2021 11:28

nicojs self-requested a review September 1, 2021 11:31

nicojs approved these changes Sep 1, 2021

View reviewed changes

nicojs merged commit 701d8b3 into stryker-mutator:master Sep 1, 2021

Garethp deleted the disable-mutation-comment branch September 1, 2021 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ignore): support disable directives in source code #3072

feat(ignore): support disable directives in source code #3072

Garethp commented Aug 12, 2021 •

edited by nicojs

nicojs left a comment •

edited

nicojs Aug 17, 2021

nicojs Aug 17, 2021

Garethp Aug 19, 2021

nicojs Aug 20, 2021 •

edited

Garethp commented Aug 22, 2021

nicojs left a comment •

edited

nicojs Aug 25, 2021

nicojs Aug 26, 2021

nicojs commented Aug 26, 2021

nicojs commented Sep 1, 2021

feat(ignore): support disable directives in source code #3072

feat(ignore): support disable directives in source code #3072

Conversation

Garethp commented Aug 12, 2021 • edited by nicojs

nicojs left a comment • edited

Choose a reason for hiding this comment

nicojs Aug 17, 2021

Choose a reason for hiding this comment

nicojs Aug 17, 2021

Choose a reason for hiding this comment

Garethp Aug 19, 2021

Choose a reason for hiding this comment

nicojs Aug 20, 2021 • edited

Choose a reason for hiding this comment

Garethp commented Aug 22, 2021

nicojs left a comment • edited

Choose a reason for hiding this comment

nicojs Aug 25, 2021

Choose a reason for hiding this comment

nicojs Aug 26, 2021

Choose a reason for hiding this comment

nicojs commented Aug 26, 2021

nicojs commented Sep 1, 2021

Garethp commented Aug 12, 2021 •

edited by nicojs

nicojs left a comment •

edited

nicojs Aug 20, 2021 •

edited

nicojs left a comment •

edited