-
Notifications
You must be signed in to change notification settings - Fork 250
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
Conversation
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`.
There was a problem hiding this 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
@brodybits Everything is now ready for review, please have a look 👍 |
@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}" |
There was a problem hiding this comment.
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}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this.
@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 🙏 |
I would probably rename the option (and ideally the modules) to disableTypeCheck or disableTypeChecks. My apologies for the delays. |
Thanks! Will you also rename the modules? |
Excellent point! Much better. Thanks a lot! And just in time for this PR 👍
Yeah, forgot about that at first, did that right after. We're doing squash merges, so it will look nice and tidy afterward ;)
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 👍 |
Disable type checks by inserting
// @ts-nocheck
and removing all other// @ts-xxx
directives.It can be configured with:
You can disable it completely by setting
disableTypeChecks
tofalse
. 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 theinstrumenter
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
tofalse
.This setting replaces both
sandbox.fileHeaders
andsandbox.stripComments
, since those resulted in a lot of problems (see #2438 ).Fixes #2438