test_runner: report covered lines, functions and branches to reporters #49320

philnash · 2023-08-25T05:48:39Z

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.

nodejs-github-bot · 2023-08-25T05:48:46Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/coverage.js

ljharb · 2023-08-25T05:51:12Z

lib/internal/test_runner/coverage.js

+        if (count > 0 && startOffset <= line.startOffset &&
+            endOffset >= line.endOffset) {


nit:

Suggested change

if (count > 0 && startOffset <= line.startOffset &&

endOffset >= line.endOffset) {

if (

count > 0

&& startOffset <= line.startOffset

&& endOffset >= line.endOffset

) {

This matches the existing style, in particular the conditional directly above it. I've tried to stay away from changing formatting too much.

lib/internal/test_runner/utils.js

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

rluvaton · 2023-08-25T12:08:43Z

Can you please add a test? It's weird that no tests broke...

lib/internal/test_runner/utils.js

lib/internal/test_runner/coverage.js

benjamingr

Mostly looks good, a test is missing

philnash · 2023-08-28T00:39:59Z

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.

atlowChemi

I think we might want to mark this as a notable-change PRs with changes that should be highlighted in changelogs. .
@cjihrig @MoLow WDYT?

nodejs-github-bot · 2023-08-28T04:36:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53587/

philnash · 2023-08-29T13:26:50Z

I think this needs the request-ci tag again. I think it was my fault that the Jenkins CI failed, and it should be fine this time.

nodejs-github-bot · 2023-08-29T13:32:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53615/

atlowChemi · 2023-08-29T15:47:52Z

I think it was my fault that the Jenkins CI failed, and it should be fine this time.

There was a downtime with one of the CI jobs, that why I didn't bother rerunning after your new commit 🙂

philnash · 2023-08-29T23:10:13Z

I think it was my fault that the Jenkins CI failed, and it should be fine this time.

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 needs-ci label and consider merging?

atlowChemi · 2023-08-30T04:11:41Z

Does this mean we can remove the needs-ci label and consider merging?

No need to remove the label 🙂
The rule about merging is a minimum of 48 hours with 2 approvers, or 7 days with 1, so unfortunately this can't be merged yet...

MoLow

LGTM, since coverage is experimental we are good in terms of semver-major

MoLow · 2023-08-30T05:28:01Z

I think we might want to mark this as a notable-change .

@atlowChemi why?

atlowChemi · 2023-08-30T05:29:47Z

I think we might want to mark this as a notable-change .

@atlowChemi why?

This is breaking the behavior of an existing feature, which is ok as it is experimental, but I find it worth noting

nodejs-github-bot · 2023-08-30T20:22:47Z

Landed in 3a6a80a

rluvaton · 2023-08-30T21:07:03Z

Thank you @philnash for your contribution! We hope to see you again 😀

philnash · 2023-08-30T22:47:46Z

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 #49303 PR-URL: #49320 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

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>

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Aug 25, 2023

philnash force-pushed the test-runner-coverage-covered-lines branch from ada1bd1 to 7a1b7f4 Compare August 25, 2023 05:52

ljharb reviewed Aug 25, 2023

View reviewed changes

test_runner: report covered lines, functions and branches to reporters

9dba88a

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

philnash force-pushed the test-runner-coverage-covered-lines branch from 7a1b7f4 to 9dba88a Compare August 25, 2023 05:56

philnash added 2 commits August 25, 2023 16:53

test_runner: fixing style issues

85e4ba5

test_runner: remove console.logs and fix lint errors

a382307

rluvaton reviewed Aug 25, 2023

View reviewed changes

lib/internal/test_runner/utils.js Outdated Show resolved Hide resolved

rluvaton reviewed Aug 25, 2023

View reviewed changes

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

rluvaton reviewed Aug 25, 2023

View reviewed changes

lib/internal/test_runner/coverage.js Outdated Show resolved Hide resolved

benjamingr reviewed Aug 27, 2023

View reviewed changes

test_runner: addressing PR comments

e0dd2d4

Removed additional loop from calculating max counts for functions. Simplified reporting of count for each line. Returned arrow function to implicit return.

philnash force-pushed the test-runner-coverage-covered-lines branch from b023ec4 to e0dd2d4 Compare August 28, 2023 00:53

test_runner: tests for coverage event

81448af

atlowChemi approved these changes Aug 28, 2023

View reviewed changes

atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 28, 2023

test_runner: skip new coverage tests if inspector is not available

741cdf9

github-actions bot mentioned this pull request Aug 29, 2023

CI Reliability 2023-08-29 nodejs/reliability#649

Open

21 tasks

rluvaton added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2023

github-actions bot mentioned this pull request Aug 30, 2023

CI Reliability 2023-08-30 nodejs/reliability#650

Open

16 tasks

MoLow approved these changes Aug 30, 2023

View reviewed changes

atlowChemi added experimental Issues and PRs related to experimental features. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 30, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 30, 2023

nodejs-github-bot merged commit 3a6a80a into nodejs:main Aug 30, 2023

This was referenced Aug 31, 2023

CI Reliability 2023-08-31 nodejs/reliability#651

Open

CI Reliability 2023-09-01 nodejs/reliability#652

Open

CI Reliability 2023-09-02 nodejs/reliability#653

Open

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

philnash mentioned this pull request Sep 12, 2023

test_runner: lcov reporter for test coverage #49626

Closed

atlowChemi mentioned this pull request Sep 19, 2023

Code coverage html output. nodejs/node-core-test#50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: report covered lines, functions and branches to reporters #49320

test_runner: report covered lines, functions and branches to reporters #49320

philnash commented Aug 25, 2023

nodejs-github-bot commented Aug 25, 2023

ljharb Aug 25, 2023

philnash Aug 25, 2023

rluvaton commented Aug 25, 2023 •

edited

Loading

benjamingr left a comment

philnash commented Aug 28, 2023

atlowChemi left a comment

nodejs-github-bot commented Aug 28, 2023

philnash commented Aug 29, 2023

nodejs-github-bot commented Aug 29, 2023

atlowChemi commented Aug 29, 2023

philnash commented Aug 29, 2023

atlowChemi commented Aug 30, 2023

MoLow left a comment

MoLow commented Aug 30, 2023

atlowChemi commented Aug 30, 2023

nodejs-github-bot commented Aug 30, 2023

rluvaton commented Aug 30, 2023

philnash commented Aug 30, 2023

		if (count > 0 && startOffset <= line.startOffset &&
		endOffset >= line.endOffset) {

test_runner: report covered lines, functions and branches to reporters #49320

test_runner: report covered lines, functions and branches to reporters #49320

Conversation

philnash commented Aug 25, 2023

nodejs-github-bot commented Aug 25, 2023

ljharb Aug 25, 2023

Choose a reason for hiding this comment

philnash Aug 25, 2023

Choose a reason for hiding this comment

rluvaton commented Aug 25, 2023 • edited Loading

benjamingr left a comment

Choose a reason for hiding this comment

philnash commented Aug 28, 2023

atlowChemi left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 28, 2023

philnash commented Aug 29, 2023

nodejs-github-bot commented Aug 29, 2023

atlowChemi commented Aug 29, 2023

philnash commented Aug 29, 2023

atlowChemi commented Aug 30, 2023

MoLow left a comment

Choose a reason for hiding this comment

MoLow commented Aug 30, 2023

atlowChemi commented Aug 30, 2023

nodejs-github-bot commented Aug 30, 2023

rluvaton commented Aug 30, 2023

philnash commented Aug 30, 2023

rluvaton commented Aug 25, 2023 •

edited

Loading