feature(instrumentor): Allow only running specific mutants

[Garethp]

This is a PR to accompany issue #2751. It allows you to define specific mutants through the --mutate flag by adding a non-inclusive range for the mutant you want to run. For example, --mutate src/index.ts:1:3:1:5 would only instrument mutants that start on line 1 column 3 and end on line 1 column 5 (non-inclusive)

[nicojs]

I've started a review at my own risk (since you're still busy with it) and got some remarks. Mainly I think file.js:5:0:20:0 should mutate line 5 up and including line 20. So you declare a range of mutants. I think the way you see it is that it will only mutate that specific mutant itself.

[Garethp]

I've started a review at my own risk (since you're still busy with it) and got some remarks.

It's fine, I was thinking of doing the --specificMutantsJSON flag as part of a different PR anyway and move this from WIP to ready for review.

Mainly I think file.js:5:0:20:0 should mutate line 5 up and including line 20. So you declare a range of mutants. I think the way you see it is that it will only mutate that specific mutant itself.

Only mutating the specific mutant defined and not any inclusive mutants is what I laid out in the issue and was what I thought would be good here. What's the advantage of having an inclusive range rather than a specific match? As long as I can have some way to define specific non-inclusive mutant ranges then I'm not bothered. I can move that functionality to --specificMutantJSON, but I get the feeling that you're not super keen on that solution either

[nicojs]

Thanks, @Garethp I've taken a more in-depth look.

Sorry for the delay 😔, There is a lot of stuff on my plate and I sometimes need a trigger, so feel free to do that any time on Slack (as you now did).

I could also pick up the remarks in this PR. I have some time this week, I planned an open-source day this Friday and will have some time in this weekend as well.

[Garethp]

I've gone ahead and moved the parsing to happen only in the input-file-resolver, and the validation to only occur in the options-validator. I haven't implemented skipping any code that lies outside of the mutation range, but I'm not even sure how I'd test that.

We'd have to not skip over nodes that surround the ranges, so we'd need to keep the current condition and just have another one that does skip for anything where none of the node falls inside that range, which is what I think you were thinking about? But in that case, the observable output of the new condition wouldn't change, and it would be a purely performance improvement with no way to write a test against it, right?

I think it's a great idea, I'm just not sure how you want to approach it

[nicojs]

We'd have to not skip over nodes that surround the ranges, so we'd need to keep the current condition and just have another one that does skip for anything where none of the node falls inside that range, which is what I think you were thinking about? But in that case, the observable output of the new condition wouldn't change, and it would be a purely performance improvement with no way to write a test against it, right?

Exactly right. We should have tests for all those cases you're describing, make sure our code works, then we can introduce the performance enhancement and see if the tests still pass. The performance enhancement doesn't have an observable output, but that's fine, as long as the tests still pass, nothing broke 🤷‍♂️. That's enough for me. It would result in equivalent mutants, which is why I wouldn't go for 100% mutation score.

I actually found a bug... I think:

              path.node.loc!.start.line >= mutant.start.line &&
              path.node.loc!.start.column >= mutant.start.column &&
              path.node.loc!.end.line <= mutant.end.line &&
              path.node.loc!.end.column <= mutant.end.column

This would mean that a mutant from line 2, column 1, to line 2 column 2 would not fall into the range of line 1, column 0, line 100, column 0 (because of the column matches)

I'll go ahead and add tests for this and fix it. As well as review your work. Thanks for the hard work and sorry again for my late responses

[nicojs]

Ok I think this should be it 😅. This is what I changed:

• Rename filename to fileName. This is a hot topic within our code base, but since we're using fileName currently everywhere I went ahead and changed it.
• Rename mutationRange to mutationRanges, since it's an array.
• Implement the new mutation range matching algorithm using locationIncluded and locationOverlaps. I think it looks quite nice 🎨
• Add a bunch of unit tests for the mutation ranging algorithm inside test/unit/transformers/babel-transformer.spec.ts
• Change the mutation range functionality a bit. Stryker will now also mutate files that don't have mutation ranges. I thought this would make the most sense. If you run stryker run -m "foo.js:0:0:1:0,bar.js" I would personally expect bar.js to also be mutated. Thoughts?
• Refactor the regexes to reduce them to 1 uber regex. It uses regex groups to capture the file name, lines and columns: /(.*):(\d+):(\d+):(\d+):(\d+)$/. It's exported and reused by the options validator.
• Changed the e2e test a bit so it also verifies that files without ranges are mutated and files that are not listed at all are not mutated.
• I've documented mutation ranges in configuration.md

Some final thoughts:

• 🤔 should we change the syntax to foo.js:1:0-10:0? That might feel more natural for some users
• 🤔 should we allow for a short range syntax: foo.js:1-10? This would be pretty simple to implement:
  • 1-10 is equivalent to 1:0-11:0 (it includes everything though line 10 excluding line 11)
  • 1:4-10 is equivalent to 1:4-11:0

[Garethp]

Thanks for your help, the changes you made are great!

should we change the syntax to foo.js:1:0-10:0? That might feel more natural for some users
should we allow for a short range syntax: foo.js:1-10? This would be pretty simple to implement

Both of these ideas are fantastic. I'll try to find some time to do that

[Garethp]

@nicojs I've updated the regex to accept the new format as well as optional columns. The documentation should also be updated to reflect it

[nicojs]

I've changed the implementation a bit. Now the to-line is inclusive when using a-b syntax. I'm using Number.MAX_SAFE_INTEGER as a default for the column number, sounds like a big enough number to me.