-
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
fix(reporters): add offset when reporting a single mutant #4158
Conversation
This was a bug, we were reporting the mutant without the offset included in the mutant. Came across this while implementing realtime reporting :).
2e5fc4f
to
5ac99a1
Compare
const input = factory.mutantTestCoverage({ | ||
fileName: 'add.js', | ||
id: '3', | ||
location: factory.location(), |
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.
You probably don't want to rely on the defaults for this test, since you assert them below. I always try to be explicit in the tests.
@@ -97,6 +97,7 @@ export class MutationTestReportHelper { | |||
} | |||
|
|||
private reportOne(result: MutantResult): MutantResult { | |||
result.location = this.toLocation(result.location); |
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 understand that this is more DRY than doing it in reportMutantRunResult
, but the result is that the construction of result
is now spread across 2 places. Also you're mutating an object on the heap, which can lead to undesired consequences (it's convoluted).
Can you move this to reportMutantRunResult
? You can still do the location calculation once atop that function.
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.
Then it would also have to be moved to reportMutantStatus
and reportCheckFailed
, but I think that's okay.
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.
Yes, that's probably better than the 2-staged way of doing now, do you agree?
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.
Seems good, since we keep the creation/modification outside of the reportOne
method (that should only report and not do anything else).
No description provided.