Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(typescript): Disable type checking #2446

Merged
merged 13 commits into from
Sep 2, 2020

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Aug 30, 2020

Disable type checks by inserting // @ts-nocheck and removing all other // @ts-xxx directives.

It can be configured with:

{
  "disableTypeChecks": "{test,src,lib}/**/*.{js,ts,jsx,tsx,html,vue}" // configure a pattern or `false` here
  "warnings": {
    "preprocessorErrors": true // enable/disable error logging here (default: true)
  }
}

You can disable it completely by setting disableTypeChecks to false. The default is "{test,src,lib}/**/*.{js,ts,jsx,tsx,html,vue}".

The disabling of type checks is done in the DisableTypeChecksPreprocessor class. It calls the function in the instrumenter package to do the actual disabling. The reason for this is that files need to be parsed to distinguish between code and comments (@ts-xxx directives are only removed when they appear inside comments). It also works with HTML and vue files, so it also supports use cases where type checking is done inside vue files (with the vue-cli).

Whenever parsing a file results in an error, a warning is logged and mutation testing will still proceed. The warning can be disabled by setting warnings.preprocessorErrors to false.

This setting replaces both sandbox.fileHeaders and sandbox.stripComments, since those resulted in a lot of problems (see #2438 ).

Fixes #2438

DIsable type checking by inserting `// @ts-nocheck` and removing all other `// @ts-xxx` directives. This is done in the instrumenter, by parsing the input files and scanning the comments.

It can be configured with:

```json
{
  "sandbox": {
    "disableTypeChecking": "**/*.ts"
  }
}
```

You can disable it completely by setting `disableTypeChecking` to `false`.

This setting replaces both `sandbox.fileHeaders` and `sandbox.stripComments`.
Copy link

@brodycj brodycj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Can you add a link to issue Discussion: TypeScript compile errors with mutation switching #2438 to the description of this PR?
  • I assume you will remove the other sandbox options from the schema and update the documentation?
  • This looks like an empty file on GitHub: packages/core/test/unit/sandbox/disable-type-checking-preprocessor.spec.ts
  • I left a couple more comments on the changes

packages/api/schema/stryker-core.json Outdated Show resolved Hide resolved
packages/instrumenter/src/disable-type-checking.ts Outdated Show resolved Hide resolved
@nicojs nicojs marked this pull request as ready for review August 31, 2020 09:19
@nicojs
Copy link
Member Author

nicojs commented Aug 31, 2020

@brodybits Everything is now ready for review, please have a look 👍

@brodycj
Copy link

brodycj commented Aug 31, 2020

@nicojs I did take a quick look earlier today, may need some more time to take a more careful look. Thanks for your work so far on this!

@@ -191,7 +180,7 @@
"type": "string"
}
],
"default": "**/*+(.js|.ts|.cjs|.mjs)?(x)"
"default": "**/*.{js,ts,jsx,tsx,html,vue}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have second doubts about this default. It might be too broad. For example, when running Stryker on express you get this warning:

18:36:16 (20065) WARN DisableTypeCheckingPreprocessor Unable to disable type checking for file "/home/nicojs/stryker/perf/test/express/examples/ejs/views/footer.html". Shouldn't type checking be disabled for this file? Consider configuring a more restrictive "sandbox.disableTypeChecking" settings (or turn it completely off with `false`) ParseError: Parse error in /home/nicojs/stryker/perf/test/express/examples/ejs/views/footer.html (1:0) Unexpected closing tag "body". It may happen when the tag has already been closed by another tag. For more info see https://www.w3.org/TR/html5/syntax.html#closing-elements-that-have-implied-end-tags
    at ngHtmlParser (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/parsers/html-parser.js:51:15)
    at async Object.parse (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/parsers/html-parser.js:33:18)
    at async DisableTypeCheckingPreprocessor.disableTypeChecking [as impl] (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/instrumenter/dist/src/disable-type-checking.js:16:17)
    at async /home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/disable-type-checking-preprocessor.js:30:32
    at async Promise.all (index 36)
    at async DisableTypeCheckingPreprocessor.preprocess (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/disable-type-checking-preprocessor.js:27:30)
    at async MultiPreprocessor.preprocess (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/sandbox/multi-preprocessor.js:14:25)
    at async MutantInstrumenterExecutor.execute (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/process/2-MutantInstrumenterExecutor.js:24:23)
    at async Stryker.runMutationTest (/home/nicojs/stryker/perf/test/express/node_modules/@stryker-mutator/core/src/Stryker.js:32:44)
18:36:16 (20065) WARN DisableTypeCheckingPreprocessor (disable "warnings.preprocessorErrors" to ignore this warning

Maybe a better default would be {test,src,lib}/**/*.{js,ts,jsx,tsx,html,vue}?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've implemented this.

@nicojs
Copy link
Member Author

nicojs commented Sep 2, 2020

@brodybits I'll be merging this as soon as the tests pass, since it is blocking some progress on other stuff. Feel free to still finish you're review and comment (or again a PR), we can always improve later 🙏

@brodycj
Copy link

brodycj commented Sep 2, 2020

I would probably rename the option (and ideally the modules) to disableTypeCheck or disableTypeChecks.

My apologies for the delays.

@brodycj
Copy link

brodycj commented Sep 2, 2020

Thanks! Will you also rename the modules?

@nicojs
Copy link
Member Author

nicojs commented Sep 2, 2020

Excellent point! Much better. Thanks a lot! And just in time for this PR 👍

Thanks! Will you also rename the modules?

Yeah, forgot about that at first, did that right after. We're doing squash merges, so it will look nice and tidy afterward ;)

My apologies for the delays.

No worries whatsoever. I'm not entitled to your time, I know that there are more important things than this in the world 🤗

Thank you for the review! I'm glad you find naming things just as important as we do 👍

@nicojs nicojs merged commit 3ff996b into epic/mutation-switching Sep 2, 2020
@nicojs nicojs deleted the feat/disable-type-checking branch September 2, 2020 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: TypeScript compile errors with mutation switching
2 participants