feat(group-mutants): checkers check groups of mutants #3366
Conversation
…merge Group mutants packagelock merge
danny12321
changed the title
There was a problem hiding this comment.
Wow, that seems to be about a half years work condensed into 1 PR 😅. Well done!
This is my first round of review comments. I didn't open in VSCode yet, so it was a colorful experience. I will have a closer look in the second round (after fixes are implemented). No time pressure, let's make sure we get it right.
Also: you probably want to merge epic/esm into this branch. Some of the changes will go away as well as the merge conflict.
| await expectMetricsJson({ | ||
| killed: 27, | ||
| mutationScore: 57.45, | ||
| killed: 25, |
There was a problem hiding this comment.
Why the changes in score of e2e test?
There was a problem hiding this comment.
This change came from an upstream fetch.
package.json
Outdated
| "@typescript-eslint/eslint-plugin": "~5.9.1", | ||
| "@typescript-eslint/parser": "~5.9.1", | ||
| "@typescript-eslint/eslint-plugin": "~5.10.0", | ||
| "@typescript-eslint/parser": "~5.10.0", |
There was a problem hiding this comment.
This change also came from an upstream fetch.
packages/api/src/check/checker.ts
Outdated
|
|
||
| import { CheckResult } from './check-result'; | ||
|
|
||
| export interface Checker { | ||
| init(): Promise<void>; | ||
|
|
||
| check(mutant: Mutant): Promise<CheckResult>; | ||
| check(mutant: MutantTestCoverage[]): Promise<Array<{ mutant: MutantTestCoverage; checkResult: CheckResult }>>; |
There was a problem hiding this comment.
Now that I think about it, maybe a Promise<Record<string, CheckResult>> is better (keys are mutant ids). It would at least save a lot of duplicate data being serialized back to the core process.
| if (!this.options.checkers.length) { | ||
| const checkResult$ = new Subject<MutantResult>(); | ||
| checkResult$.complete(); | ||
| return { |
There was a problem hiding this comment.
I like this early return, but could we maybe implement this in the CheckerFascade class? I want this Executor class to not have to worry about these kinds of details.
| mutant, | ||
| }; | ||
| if (!this.options.checkers.length) { | ||
| const checkResult$ = new Subject<MutantResult>(); |
There was a problem hiding this comment.
Instead of creating a subject and completing it instantly, we can use empty()
|
|
||
| import { toPosixFileName } from './fs/tsconfig-helpers'; | ||
|
|
||
| export function createGroups(sourceFiles: SourceFiles, mutants: MutantTestCoverage[]): MutantTestCoverage[][] { |
There was a problem hiding this comment.
This probably deserves a JS doc explaining what we're doing and why.
| continue; | ||
| } | ||
|
|
||
| let ignoreList = [firstSourceFile.fileName, ...firstSourceFile.importedBy]; |
There was a problem hiding this comment.
Can be a const. Instead of using ignoreList = [...ignoreList, val, val2] you can use ignoreList.push(val, val2). Also: you probably want to use a Set, because it is much more performant when checking for existence.
| let groups: MutantTestCoverage[][] = []; | ||
|
|
||
| while (mutantsWithoutGroup.length) { | ||
| const firstMutant = mutantsWithoutGroup[0]; |
There was a problem hiding this comment.
Suggestion: use shift or pop to get a mutant from the list. That way, you also removed it (and don't need to start from 1 later).
|
|
||
| function addGroupToList() { | ||
| // Can be improved, we know the index in the for loop but can't remove it because we loop over the same array we want te remove from | ||
| // A solution can be to store a array with indexes to remove |
| if (possibleMutants.length === 1) { | ||
| mutantResults[mutantResults.findIndex((mutantResult) => mutantResult.mutant.id === possibleMutants[0].id)].errors.push(error); | ||
| } else if (possibleMutants.length > 1) { | ||
| possibleMutants.forEach((mutant) => mutantsToTestIndividual.add(mutant)); |
There was a problem hiding this comment.
You probably want to log on debug level here. Useful information for debugging performance issues later.
|
I'm thinking of the best way to merge this in. Since this is a very big change and it already diverged from the base branch a lot. I mean, we've migrated to ESM in the meantime, literally changing very code file in our repository 😇. We might be better off abandoning this branch, and instead, create fresh ones from
@danny12321 @ThierryRietveld do you agree with this approach? We've planned some more Stryker days next week Friday and the following Monday, maybe we can discuss it then? I've also sent you guys invites to become contributors on StrykerJS. That way you can work on the |
|
Hi Nico, thanks for the invitation to join the Stryker team! @danny12321 and I agree that splitting the PR into two separate PR's is the best option to make it smaller. Next week Friday and Monday we got 'Open Leercentrum', so I don't know if we can get some time to participate in the Stryker days. This week I got some time to start the separation of this PR. |
|
Ok, great. We can at least have a talk via teams. I'll plan a meeting. See you then, have fun in the mean time! 🤗 |
|
@ThierryRietveld @danny12321 is this PR still relevant? Or will you create a new one? |
The checkers (also known as the TypeScript-checker) now receive an array of mutants. The group of mutants can be checked at once which will increase performance in larger projects.