-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
test_runner: report covered lines, functions and branches to reporters #49320
test_runner: report covered lines, functions and branches to reporters #49320
Conversation
Review requested:
|
ada1bd1
to
7a1b7f4
Compare
if (count > 0 && startOffset <= line.startOffset && | ||
endOffset >= line.endOffset) { |
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.
nit:
if (count > 0 && startOffset <= line.startOffset && | |
endOffset >= line.endOffset) { | |
if ( | |
count > 0 | |
&& startOffset <= line.startOffset | |
&& endOffset >= line.endOffset | |
) { |
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.
This matches the existing style, in particular the conditional directly above it. I've tried to stay away from changing formatting too much.
This is a breaking change for the format of test:coverage events. But the test coverage is still experimental, so I don't believe it requires a semver-major bump. Fixes nodejs#49303
7a1b7f4
to
9dba88a
Compare
Can you please add a test? It's weird that no tests broke... |
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.
Mostly looks good, a test is missing
Thanks for the reviews so far. It shouldn't be weird that the tests didn't break, they currently only test the output of the test reporters and each reporter that handles coverage was also refactored to handle this change. I'll add a test now that checks the full coverage output. |
Removed additional loop from calculating max counts for functions. Simplified reporting of count for each line. Returned arrow function to implicit return.
b023ec4
to
e0dd2d4
Compare
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 think we might want to mark this as a
notable-change
@cjihrig @MoLow WDYT?
I think this needs the |
There was a downtime with one of the CI jobs, that why I didn't bother rerunning after your new commit 🙂 |
Ah, I see. Good to see that it's not down any more and all the builds ran successfully! Does this mean we can remove the |
No need to remove the label 🙂 |
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.
LGTM, since coverage is experimental we are good in terms of semver-major
@atlowChemi why? |
This is breaking the behavior of an existing feature, which is ok as it is experimental, but I find it worth noting |
Landed in 3a6a80a |
Thank you @philnash for your contribution! We hope to see you again 😀 |
Thanks @rluvaton, I'm working on one more thing at the moment, so you're going to see more of me 😄 |
This is a breaking change for the format of test:coverage events. But the test coverage is still experimental, so I don't believe it requires a semver-major bump. Fixes nodejs#49303 PR-URL: nodejs#49320 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fixes #49303.
This is a breaking change for the format of
test:coverage
events. But the test coverage is still experimental, so I don't believe it requires a semver-major bump.