-
Notifications
You must be signed in to change notification settings - Fork 248
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(grouping): checker accept groups of mutants #3450
Conversation
972e974
to
96d29d8
Compare
return group ?? mutants.map((m) => [m.mutant]); | ||
private executeChecker(checkerName: string, input$: Observable<MutantRunPlan>) { | ||
const group$ = this.checkerPool | ||
.schedule(input$.pipe(bufferTime(CHECK_BUFFER_MS)), async (checker, mutants) => { |
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'm using bufferTime
here, that makes it so the result of the previous checker can be used in the next one. Without having to wait for everything to be checked.
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 doubt if this is preferred over the previous solution. If the checkerpool is still full with tasks of the previous checker we don't need to add new tasks because all the "cpu cores" are still busy. This way we are creating new tasks with inefficient groups because there are probably not a lot of mutants to be grouped while this new group has to wait to be executed by the checkerpool. I think we want to create groups when the checkerpool is (almost) done with the previous checker, because there are more mutants ready for the next checker so we can create larger groups. We should maybe discuss this in a meeting so I can explain it better and we could consider the consequences of both implementations.
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.
As discussed in person. I doubt it would be a big performance gain. It is largely a theoretical discussion anyway until we get a second checker implementation.
mergeMap((mutantGroupResults) => mutantGroupResults), | ||
shareReplay() | ||
); | ||
const [passedCheckResult$, failedCheckResult$] = partition(checkTask$, (item): item is MutantRunPlan => item.plan === PlanKind.Run); |
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.
Instead of creating subjects and having to manage those instances, I use partition
as a way to split an observable
into 2. Using shareReplay
will use a ReplaySubject
under the hood to make sure the underlying observable is only executed once.
} | ||
|
||
previousPassedMutants$.subscribe({ |
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.
using subscribe
here is not good! Doing that will execute the observable. Subscribing again later will re-execute the observable again. Instead I'm using tap({ complete: () => {})
.
I had 30 mins to spare and did a small refactor. See my comments. Question: with my new code, do we still need the |
@@ -18,5 +18,5 @@ export interface Checker { | |||
* @param mutants the mutants to group | |||
* @returns the groups of mutant ids | |||
*/ | |||
group?(mutants: Mutant[]): Promise<string[][] | undefined>; | |||
group?(mutants: Mutant[]): Promise<string[][]>; |
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.
Rename createGroups
to group
. As in "to group" ("groeperen" in Dutch).
I also changed it to return a Promise<string[][]>
instead of Promise<Mutant[][] | undefined>
. I don't want checkers to be able to change mutants, so only the mutant id is allowed to be provided. I also don't see a reason for group
to return undefined
. It should always return the groups if implemented.
First of all thanks for your help @nicojs!
I don't think we should move the code of the |
It wouldn't be about the implementation, rather the process of checking mutants. Which is exactly what the mutation test executer is designed to do imo. 🤷 |
This pr concludes the grouping of mutants before they are sent to the checkers. It is still a draft, the tests need to be fixed.
Add support for checking groups of mutants, instead of one-by-one.
The new API looks like this:
Note: implement this new API in the TypeScript checker, but don't provide a implementation of
group
yet. Will come in the near future.BREAKING CHANGE: The
check
method of checker plugins now receives a group of mutants, and should provide aCheckResult
per mutant id.