improve & focus sandbox preprocessing options

[brodybits]

• add sandbox.ignorePatterns functionality to sandbox preprocessing, ignores test files & __tests__ directory by default
• rename sandbox.fileHeaders -> sandbox.addHeaders, for consistency (all verbs)
• rename sandbox.stripComments -> sandbox.removeComments (bikeshedding)
• simplify default sandbox file patterns
• limit default sandbox preprocessing to ts & tsx files
• remove eslint-disable line from default sandbox.addHeaders - I think that smart Stryker configuration would not run eslint during mutation testing

This proposal was triggered by my investigation of #2403. I tried a couple other ideas but they are probably not worth considering.

It is very opinionated to the point of bikeshedding, but I broke the changes into individual commits. We should be able to revert or remove any changes not wanted in this proposal.

We may want to rename a few modules if we rename the options like I proposed, which should be pretty trivial.

[nicojs]

Thanks for taking a look at the preprocessors and trying to improve them. I certainly don't like them so any improvement is awesome.

I'm not a fan of ignorePatterns. It is confusing, as you can read in one of my comments.

I also see you're trying to exclude test files by default. Unfortunately, I think they shouldn't be excluded because of problems with typescript compilation. I wish there was an easier way to run typescript without typechecking, but there isn't AFAIK.

Did you ever find out why exactly the initial test run in your use case fails without the override of sandbox options? Is it because of the // @jest-environment directives? If so, maybe we can do something smart where always leave those in? I would prefer that over changing the defaults of removeComments and addHeaders.

Please let me know what you think.

[nicojs]

So remove is better than strip? I guess I've used the naming of the strip-comments dependency. Remove sounds better indeed.

[nicojs]

Actually, @ts-nocheck is also needed for js files if typescript is compiling with the --checkJS option

[nicojs]

I see you've remvoed the eslint-disable check. You think build commands won't fail on eslint checks?

[nicojs]

So this is like a "master switch"? Wouldn't that be confusing? What does this mean:

{
  sandbox: {
    ignorePatterns: "**/*.js",
    removeComments: "**/*.js"
  }

[brodybits]

So this is like a "master switch"?

yes

My idea was that it should be possible to ignore all files in __tests__ for example, even though some files in __tests__ would match a pattern such as **/*.{js,ts} or **/!(*.spec,*.test,*Spec).{js,ts}.

But yeah, this idea would probably be too confusing. Back to the drawing board.

[nicojs]

Ignoring the test files will not help in scenario's where typescript is used.

For example:

// concat.ts
function concat(a: string, b: string) {
   return a + b; // return type of this function will change because of mutation switching
}

// __tests__/concat.spec.ts
it('should concat', () => {
  const actual: string = concat('a', 'b');
  expect(actual).eq('ab');
});

In this case, __tests__/concat.spec.ts will give a compile error. You would notice it if your using buildCommand: "tsc -b" or you use jest-ts for example.

[nicojs]

Note: should be removed still to removeComments

[nicojs]

This file should also be renamed to remove-comments-preprocessor

[nicojs]

This class should also be renamed to RemoveCommentsPreprocessor.

[brodybits]

Thanks Nico for the quick review. I may need a few days to process and respond to the feedback. In general, I think the React library (https://github.com/facebook/react) may be a special case where we would need the config workarounds for quite a while. Back to draft status for now.

[nicojs]

I think React may be a special case

So is Angular or Vue. They all need special configuration to make work.

I was thinking, instead of improving the defaults for these use cases (always a trade-off, improving one worsens another) we could also choose to add "preset" functionality.

For example, --preset react would set the sandbox config needed to run "react". That would lessen the need for more custom overrides.

However, we might still need a way to leave the @jest-environment in the code, while adding @ts-nocheck and removing all other @ts-xxx directives. (for the jest-ts use case, the jest transpiler that does type checking).

[brodybits]

By "React may be a special case" I had meant "React library (https://github.com/facebook/react) may be a special case". I updated my comment to make this more clear.

That said, the idea of adding and using the "preset" functionality sounds nice and may prove quite helpful down the road. I imagine it would be nice for a user to start with a "preset" and customize a limited number of items, like I see in the Renovate bot. Maybe this is what you meant from the beginning.

Unfortunately I am a bit lost in terms of the Jest functionality here. But I was just thinking it may (or may not) be useful if we would only do the sandbox preprocessing updates on files to be mutated and not test files. That's what I wanted to do with the ignorePatterns option, since I could not see a way to have the existing fileHeaders & stripComments ignore all files in __tests__ directory.

But as I said in the comment above, I think you are right that this would be too confusing. I did raise another idea in #2407 to only update sources to be mutated in the sandbox, but think it would not solve with problem with the React library.

As I said before, my proposal to rename "stripComments" to "removeComments" may be a form of bikeshedding. Some could find "strip" to be a little offensive, that is why I proposed renaming it. But not a really big deal on my part.

I would be happy to move the changes to rename the 2 sandbox options into a separate PR, and rename the source modules accordingly in the separate PR. Maybe sometime later this week.

[nicojs]

Closed in favor of #2446