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

Added Typescript group checking 🚀 #3900

Merged
merged 101 commits into from
Feb 10, 2023
Merged

Conversation

danny12321
Copy link
Contributor

@danny12321 danny12321 commented Dec 9, 2022

Add grouping mutants to the @stryker-mutator/typescript-checker to improve performance.

The typescript checker is slow because mutants are checked for compile errors one by one. Each time a mutant is checked, TypeScript will do full type-checking. This is sped up by running multiple checkers in parallel, using typescript's --watch mode, and only compiling in memory. Yet still, this is very slow, as a rule of thumb: 10x slower than not enabling this checker.

This PR improves this by grouping mutants unrelated to each other in the same typescript compilation run. Any compilation errors that result from one of the mutants in a group can always be associated with one mutant.

One downside to this approach is that a TypeScript compilation sometimes forgets to report some of the errors (although it is a design goal of TypeScript to report them all). For example microsoft/TypeScript#46272

This is why you disable this grouping behavior by adding:

{
  "typeScriptChecker": {
    "prioritizePerformanceOverAccuracy": false
  }
}

This is just a draft PR. Todo:

ThierryRietveld and others added 30 commits October 28, 2022 11:46
@nicojs
Copy link
Member

nicojs commented Jan 20, 2023

@danny12321 CI pipeline is failing. It seems that you're still using structuredClone. See my comment here: #3900 (comment)

Can we move this PR out of "draft"?

@danny12321
Copy link
Contributor Author

@danny12321 CI pipeline is failing. It seems that you're still using structuredClone. See my comment here: #3900 (comment)

Can we move this PR out of "draft"?

Oops, thought we had removed all of them. I have fixed it now.

@danny12321 danny12321 marked this pull request as ready for review January 20, 2023 17:04
Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

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

Great job! Can't wait to get this merged 😍

Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

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

I've noticed that the coverage on some files is low. I've added unit tests to test the edge cases of the typescript-checker.ts file, but the typescript-compiler.ts tests are still missing some important code paths. See my remark there.

packages/typescript-checker/src/typescript-compiler.ts Outdated Show resolved Hide resolved
@odinvanderlinden
Copy link
Contributor

Great work guys 🎉

@nicojs nicojs merged commit 2f4adaa into stryker-mutator:master Feb 10, 2023
@nicojs
Copy link
Member

nicojs commented Feb 10, 2023

:shipit:

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.

4 participants