Skip to content
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: add code coverage support to spec reporter #46674

Merged
merged 2 commits into from
Apr 2, 2023

Conversation

pulkit-30
Copy link
Contributor

@pulkit-30 pulkit-30 commented Feb 15, 2023

fixes #46665

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

Screenshot 2023-03-13 at 12 13 11 AM

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Feb 15, 2023
@pulkit-30
Copy link
Contributor Author

pulkit-30 commented Feb 15, 2023

Hey @cjihrig, I need some help

  1. what colour and symbol we should use for 'test:coverage' (I have used the same colour and symbol as 'test:diagnostic').
  2. 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 🚀

@@ -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`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -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`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe coverage should have a separate full-blown reporter?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feeling there.

Copy link
Contributor

@richiemccoll richiemccoll Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some screenshot at different available terminal width:

  • Full width

Screenshot 2023-04-29 at 13 44 25

  • Medium:

Screenshot 2023-04-29 at 13 44 39

  • Small:

Screenshot 2023-04-29 at 13 44 49

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmnsgn I think it looks fantastic.

I will prepare a PR if that's okay.

Please do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 281 to 287
start of coverage report
*
*
*
*
*
end of coverage report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@pulkit-30 pulkit-30 requested review from MoLow and removed request for cjihrig March 14, 2023 13:57
if (coverage < 50) return `${red}${coverage}${color}`;
return coverage;
}
#reportCoverage(nesting, symbol, color, summary) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems this is still a code duplication

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@MoLow
Copy link
Member

MoLow commented Mar 15, 2023

@cjihrig any chance you give an extra look?

@pulkit-30 pulkit-30 requested a review from cjihrig March 15, 2023 20:07
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}

function getCoverageFixtureReport() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return coverage;
}

function getCoverageReport(pad, summary, symbol = '# ', color = white) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

MoLow commented Mar 15, 2023

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
Copy link
Contributor

cjihrig commented Mar 15, 2023

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
Copy link
Member

MoLow commented Mar 15, 2023

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

`${coverageThreshold(totals.coveredBranchPercent, color)} | ` +
`${coverageThreshold(totals.coveredFunctionPercent, color)} |\n`;

report += `${pad}${symbol}end of coverage report${white}\n`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can do in this pr 👍🏼

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 2, 2023
@nodejs-github-bot 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
Copy link
Collaborator

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

@MoLow MoLow added 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. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 2, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 6d06586 into nodejs:main Apr 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6d06586

@pulkit-30 pulkit-30 deleted the fix-46665 branch April 2, 2023 17:42
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
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
PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
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
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
PR-URL: #46674
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
dmnsgn added a commit to dmnsgn/node that referenced this pull request Apr 30, 2023
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
dmnsgn added a commit to dmnsgn/node that referenced this pull request Jun 6, 2023
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
nodejs-github-bot pushed a commit that referenced this pull request Jun 7, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
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>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
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
PR-URL: nodejs#46674
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
ruyadorno pushed a commit that referenced this pull request Aug 29, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_runner: add code coverage support to spec reporter
8 participants