test_runner: add code coverage support to spec reporter #46674

pulkit-30 · 2023-02-15T18:48:01Z

test.mjs

import test from 'node:test';
import assert from 'node:assert'
test('test', () => {
  assert.strictEqual(true, true)
});

output:
running with ./node --test-reporter=spec --experimental-test-coverage test.mjs

nodejs-github-bot · 2023-02-15T18:48:06Z

Review requested:

@nodejs/test_runner

pulkit-30 · 2023-02-15T19:15:32Z

Hey @cjihrig, I need some help

what colour and symbol we should use for 'test:coverage' (I have used the same colour and symbol as 'test:diagnostic').
I have added tests test/message/test_runner_output_spec_reporter_with_coverage.js , but I am not sure this is the correct way to test this.
can you please help me out with these two 🚀

MoLow · 2023-02-16T06:38:24Z

lib/internal/test_runner/reporter/spec.js

@@ -59,6 +63,36 @@ class SpecReporter extends Transform {
      ), `\n${indent}  `);
    return `\n${indent}  ${message}\n`;
  }
+  #reportCoverage(color, nesting, summary, symbol) {
+    let report = `${color}${this.#indent(nesting)}${symbol}start of coverage report${white}\n`;


seems like this is duplicated from the tap reporter, can we extract the code to a common location?

Yes 👍
where we should add this code?
I think internal/util/ or internal/test_runner/utils.js one may be the right location...?

Also, can you please tell me what color and symbol we can use with the spec coverage-reporter?

cjihrig · 2023-02-16T14:33:54Z

lib/internal/test_runner/reporter/spec.js

@@ -59,6 +63,36 @@ class SpecReporter extends Transform {
      ), `\n${indent}  `);
    return `\n${indent}  ${message}\n`;
  }
+  #reportCoverage(color, nesting, summary, symbol) {
+    let report = `${color}${this.#indent(nesting)}${symbol}start of coverage report${white}\n`;
+    report += `${color}${this.#indent(nesting)}${symbol}file | line % | branch % | funcs % | uncovered lines${white}\n`;


When I wrote this table for the TAP reporter, I kind of assumed that there is a good chance the TAP will be processed by something else to make it display more nicely. For the spec reporter, I would expect something a little nicer looking out of the box.

maybe coverage should have a separate full-blown reporter?

No strong feeling there.

I don't know how much Node's spec coverage reporter should provide out of the box. But I guess the two common elements for these types of coverage reports would be:

output statistics formatted into a table

percentage thresholds indicated by color (green, yellow & red).

At #46665 (comment) and #47032, I suggested since spec reporter is intended to provide an user readable output, I propose to use Istambul text reporter, that's the same being used by Jest, NYC, and by extension also c8.

@piranna how about this:

Looks nice, I like it :-) Just remove the extra whitespace in line % and funcs % columns to save horizontal space.

Are going filenames and number to have some extra colors too, like yellow and red?

Here are some screenshot at different available terminal width:

Full width

Medium:

Small:

With uncovered lines as ranges and filename with colors.
I will prepare a PR if that's okay.

Here are some screenshot at different available terminal width:

I love it!!! 😍

With uncovered lines as ranges and filename with colors.

I would also set yellow for percentages between red and green, and I would remove the comma between the ranges, the whitespace is enough separator. Also in ranges, I would set red for uncovered lines and yellow for uncovered branches, if possible at the same time, then this would be a full replacement for IstanbulJs text reporter :-)

@dmnsgn I think it looks fantastic.

I will prepare a PR if that's okay.

Please do.

PR: #47791 (maintainers edit is set so feel free to take over and do any change you deem necessary)

cjihrig · 2023-02-16T14:36:23Z

test/message/test_runner_output_spec_reporter_with_coverage.out

+ start of coverage report
+ *
+ *
+ *
+ *
+ *
+ end of coverage report


It seems like we're not validating the important parts here. You can see how I tested this here.

cjihrig · 2023-02-16T14:38:31Z

test/message/test_runner_output_spec_reporter_with_coverage.js

+const spawn = require('node:child_process').spawn;
+const child = spawn(process.execPath,
+                    ['--no-warnings', '--test-reporter', 'spec',
+                     '--experimental-test-coverage', 'test/message/test_runner_output.js'],


I think we should use a simpler test file than test_runner_output.js for this.

MoLow · 2023-03-14T16:37:21Z

lib/internal/test_runner/reporter/spec.js

+    if (coverage < 50) return `${red}${coverage}${color}`;
+    return coverage;
+  }
+  #reportCoverage(nesting, symbol, color, summary) {


there seems this is still a code duplication

MoLow

Nice.

MoLow · 2023-03-15T19:12:33Z

@cjihrig any chance you give an extra look?

cjihrig

Left a few comments. I'm personally unsure how far the shared coverageThreshold() will get us, but I won't block this on that since it can be revisited in the future.

It's also worth noting that this is definitely not the Istanbul text reporter requested in #46665 (comment). I personally don't care about that aspect, but others might.

cjihrig · 2023-03-15T19:44:12Z

test/parallel/test-runner-spec-coverage.js

+  });
+}
+
+function getCoverageFixtureReport() {


There is a lot of overlap between this file and test-runner-coverage.js. I would add the two tests from this file to the existing file. You could even leverage subtests:

test('spec reporter', async (t) => { await t.test('coverage is reported and dumped to NODE_V8_COVERAGE if present', (t) => {}); // etc. });

If you do that, please create a similar top level test for the TAP reporter too.

cjihrig · 2023-03-15T20:03:51Z

lib/internal/test_runner/utils.js

+  return coverage;
+}
+
+function getCoverageReport(pad, summary, symbol = '# ', color = white) {


Please remove the default values for symbol and color. The TAP reporter can pass them explicitly. Also, instead of using white for the TAP reporter, please use the empty string (the value of white if colors are not supported).

MoLow · 2023-03-15T20:30:48Z

It's also worth noting that this is definitely not the Istanbul text reporter requested in #46665 (comment). I personally don't care about that aspect, but others might.

I think such a reporter fits better on user land since it will probably have many dependencies

cjihrig · 2023-03-15T20:32:58Z

it will probably have many dependencies

Possibly. I thought the ask was just to output a table that looks like https://istanbul.js.org/docs/advanced/alternative-reporters/#text. But again, I'm not too worried about it myself. I'll let others chime in if it's a big deal to them.

MoLow · 2023-03-15T20:42:19Z

I thought the ask was just to output a table that looks like istanbul.js.org/docs/advanced/alternative-reporters/#text.

that is something that can be done with https://www.npmjs.com/package/v8-to-istanbul and https://www.npmjs.com/package/istanbul-lib-report (wich is mostly what c8 does). those are the dependencies I am referring to

cjihrig · 2023-03-18T15:31:57Z

test/parallel/test-runner-coverage.js

-  assert.strictEqual(result.status, 0);
-  assert(!findCoverageFileForPid(result.pid));
-});
+test('test tap coverage reporter', async (t) => {


The first two subtests (--experimental-test-coverage and --test cannot be combined and handles the inspector not being available) are not specific to the TAP reporter and can stay as top level tests.

cjihrig · 2023-03-18T15:33:10Z

lib/internal/test_runner/utils.js

+            `${coverageThreshold(totals.coveredBranchPercent, color)} | ` +
+            `${coverageThreshold(totals.coveredFunctionPercent, color)} |\n`;
+
+  report += `${pad}${symbol}end of coverage report${white}\n`;


Should white be used here? I'm trying to avoid generating colors for the TAP reporter. If this is to reset the output color, then maybe do something like:

if (color) { report += white }

cjihrig · 2023-03-28T13:38:36Z

test/parallel/test-runner-coverage.js

-  if (!process.features.inspector) {
-    return;
-  }
+test('test tap coverage reporter', async (t) => {


Not something that needs to be done in this PR, but we should probably specify --test-reporter tap explicitly for these tests now that the default is sometimes TAP and sometimes spec.

we can do in this pr 👍🏼

Not something that needs to be done in this PR, but we should probably specify --test-reporter tap explicitly for these tests now that the default is sometimes TAP and sometimes spec.

Since tap is more machine friendly and spec more user friendly, I think the latest one should be used by default always, and being tap and opt-in, similar to json output.

nodejs-github-bot · 2023-04-02T17:09:03Z

Commit Queue failed

- Loading data for nodejs/node/pull/46674 ✔ Done loading data for nodejs/node/pull/46674 ----------------------------------- PR info ------------------------------------ Title test_runner: add code coverage support to spec reporter (#46674) Author Pulkit Gupta (@pulkit-30) Branch pulkit-30:fix-46665 -> nodejs:main Labels needs-ci, dont-land-on-v14.x, test_runner Commits 2 - test_runner: add code coverage support to spec reporter - added test-reporter explicitly Committers 1 - Pulkit Gupta <76155456+pulkit-30@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/46674 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/46674 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 15 Feb 2023 18:48:01 GMT ✔ Approvals: 2 ✔ - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46674#pullrequestreview-1336840123 ✔ - Moshe Atlow (@MoLow): https://github.com/nodejs/node/pull/46674#pullrequestreview-1342206151 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-04-02T09:57:41Z: https://ci.nodejs.org/job/node-test-pull-request/50812/ - Querying data for job/node-test-pull-request/50812/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ✔ origin/main is now up-to-date - Downloading patch for 46674 From https://github.com/nodejs/node * branch refs/pull/46674/merge -> FETCH_HEAD ✔ Fetched commits as 85705a47958c..1f1de83ed66e -------------------------------------------------------------------------------- [main e6e11385ba] test_runner: add code coverage support to spec reporter Author: Pulkit Gupta <76155456+pulkit-30@users.noreply.github.com> Date: Mon Mar 27 02:32:05 2023 +0530 4 files changed, 163 insertions(+), 89 deletions(-) [main 934b5030b7] added test-reporter explicitly Author: Pulkit Gupta <76155456+pulkit-30@users.noreply.github.com> Date: Tue Mar 28 22:49:32 2023 +0530 1 file changed, 2 insertions(+), 2 deletions(-) ✔ Patches applied There are 2 commits in the PR. Attempting autorebase. Rebasing (2/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test_runner: add code coverage support to spec reporter

PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Moshe Atlow moshe@atlow.co.il

[detached HEAD 5df08caa37] test_runner: add code coverage support to spec reporter
Author: Pulkit Gupta 76155456+pulkit-30@users.noreply.github.com
Date: Mon Mar 27 02:32:05 2023 +0530
4 files changed, 163 insertions(+), 89 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
added test-reporter explicitly

PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Moshe Atlow moshe@atlow.co.il

[detached HEAD 09f573e26c] added test-reporter explicitly
Author: Pulkit Gupta 76155456+pulkit-30@users.noreply.github.com
Date: Tue Mar 28 22:49:32 2023 +0530
1 file changed, 2 insertions(+), 2 deletions(-)

Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/4589831840

nodejs-github-bot · 2023-04-02T17:18:50Z

Landed in 6d06586

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Add a "table" parameter to getCoverageReport. Keep the tap coverage output intact. Change the output by adding padding and truncating the tables' cells. Add separation lines for table head/body/foot. Group uncovered lines as ranges. Add yellow color for coverage between 50 and 90. Refs: nodejs#46674

Add a "table" parameter to getCoverageReport. Keep the tap coverage output intact. Change the output by adding padding and truncating the tables' cells. Add separation lines for table head/body/foot. Group uncovered lines as ranges. Add yellow color for coverage between 50 and 90. Refs: #46674 PR-URL: #47791 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

PR-URL: nodejs#46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Add a "table" parameter to getCoverageReport. Keep the tap coverage output intact. Change the output by adding padding and truncating the tables' cells. Add separation lines for table head/body/foot. Group uncovered lines as ranges. Add yellow color for coverage between 50 and 90. Refs: nodejs#46674 PR-URL: nodejs#47791 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Add a "table" parameter to getCoverageReport. Keep the tap coverage output intact. Change the output by adding padding and truncating the tables' cells. Add separation lines for table head/body/foot. Group uncovered lines as ranges. Add yellow color for coverage between 50 and 90. Refs: #46674 PR-URL: #47791 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner labels Feb 15, 2023

pulkit-30 force-pushed the fix-46665 branch from 074f2fe to 1512e10 Compare February 15, 2023 19:11

MoLow reviewed Feb 16, 2023

View reviewed changes

cjihrig reviewed Feb 16, 2023

View reviewed changes

aduh95 mentioned this pull request Feb 27, 2023

chore(test): migrate to node:test nodejs/node-core-utils#670

Merged

pulkit-30 force-pushed the fix-46665 branch from 96e321a to 17cf7f6 Compare March 12, 2023 18:37

pulkit-30 marked this pull request as ready for review March 12, 2023 18:42

pulkit-30 requested a review from cjihrig March 12, 2023 18:43

benjamingr approved these changes Mar 13, 2023

View reviewed changes

pulkit-30 requested review from MoLow and removed request for cjihrig March 14, 2023 13:57

MoLow reviewed Mar 14, 2023

View reviewed changes

pulkit-30 force-pushed the fix-46665 branch from 0023874 to c57d48e Compare March 15, 2023 14:05

pulkit-30 requested a review from MoLow March 15, 2023 14:06

MoLow approved these changes Mar 15, 2023

View reviewed changes

pulkit-30 requested a review from cjihrig March 15, 2023 20:07

cjihrig reviewed Mar 15, 2023

View reviewed changes

cjihrig reviewed Mar 18, 2023

View reviewed changes

pulkit-30 requested a review from cjihrig March 20, 2023 07:19

pulkit-30 force-pushed the fix-46665 branch from c8aa32a to 5a3f4d8 Compare March 26, 2023 21:02

cjihrig reviewed Mar 28, 2023

View reviewed changes

test_runner: add code coverage support to spec reporter

ca35e89

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 2, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 2, 2023

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

nodejs-github-bot merged commit 6d06586 into nodejs:main Apr 2, 2023
61 checks passed

pulkit-30 deleted the fix-46665 branch April 2, 2023 17:42

This was referenced Apr 3, 2023

CI Reliability 2023-04-03 nodejs/reliability#538

Open

CI Reliability 2023-04-04 nodejs/reliability#539

Open

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023

test_runner: add code coverage support to spec reporter

6a70f1e

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023

test_runner: add code coverage support to spec reporter

217fc56

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023

test_runner: add code coverage support to spec reporter

d4c945d

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023

test_runner: add code coverage support to spec reporter

48206cb

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

RafaelGSS pushed a commit that referenced this pull request Apr 8, 2023

test_runner: add code coverage support to spec reporter

6b87f29

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

dmnsgn mentioned this pull request Apr 30, 2023

test_runner: refactor coverage report output for readability #47791

Merged

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

test_runner: add code coverage support to spec reporter

12fef4b

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023

test_runner: add code coverage support to spec reporter

99312a5

PR-URL: nodejs#46674 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

test_runner: add code coverage support to spec reporter #46674

test_runner: add code coverage support to spec reporter #46674

Conversation

pulkit-30 commented Feb 15, 2023 • edited

nodejs-github-bot commented Feb 15, 2023

pulkit-30 commented Feb 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richiemccoll Feb 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow left a comment

Choose a reason for hiding this comment

MoLow commented Mar 15, 2023

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow commented Mar 15, 2023

cjihrig commented Mar 15, 2023

MoLow commented Mar 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 2, 2023

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Moshe Atlow moshe@atlow.co.il

PR-URL: #46674 Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com Reviewed-By: Moshe Atlow moshe@atlow.co.il

Successfully rebased and updated refs/heads/main.

nodejs-github-bot commented Apr 2, 2023

pulkit-30 commented Feb 15, 2023 •

edited

pulkit-30 commented Feb 15, 2023 •

edited

richiemccoll Feb 17, 2023 •

edited

PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Moshe Atlow moshe@atlow.co.il

PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Moshe Atlow moshe@atlow.co.il