test: switch to native v8 coverage by bcoe · Pull Request #25157 · nodejs/node · GitHub

Switches to using v8's built in coverage support.

Note: these results will get better once https://chromium-review.googlesource.com/c/v8/v8/+/1339119 is landed.

CC: @schuay, @hashseed we're almost over the finish line \o/

TODO:

update documentation on how to run coverage.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2022/pipeline

@mhdawson I note that cctest is commented out currently in the make file, is this a known issue?

@mhdawson @schuay I've finally been able to test our work on v8, here are some findings:

tests run about 24% slower for the entire Node.js test suite; it feels like it might be worth moving some of our work into Torque (see if we can get this a bit better) but I believe that the historical approach ran closer to 100% slower.

tldr; I think the current approach is better, but performance improvements would certainly be good for the community.

internal modules now have no wrapper, making it easy to run reports.
the ends of if statements (in most cases) no longer show up as missing coverage:

I have found one additional edge case, we do not handle throw statements -- I think this is an oversight we could correct in v8:

coverage is looking good with the new approach, we have 95.88% coverage (very close to the instrumented coverage); only three tests fail when NODE_V8_COVERAGE is enabled (all related to code that is shared with the coverage writing logic).

Things are working well enough that I'd vote:

we land the v8 changes in Node.js.
update documentation.
switch our approach to coverage.

@nodejs/build 👆

The throw case should be handled just like a return as last statement.

Lets get a CI job running which uses the new approach to validate and then plan the steps to land the PRs to enable in core. @bcoe let's try to catch up on Monday to work on the CI job,

@mhdawson 👍 I think this is pretty much ready to go, I've been playing a bit of whack a mole with a few edge-case tests, but the vast majority run effectively under test.

EmitProcessEnvWarning has the side effect of setting emit_env_nonstring_warning_ = false; which I believe shouldn't be set unless a warning has actually been emitted?

Why would this be necessary? Does c8 use the the process.env setter in the deprecated way?

@joyeecheung I'll dig a bit further, I don't believe that coverage.js is setting process.env incorrectly (c8 doesn't come into play until the reporting step). It just seemed like setting the environment variable in internal/process/coverage.js was suppressing the warning message generated by the unit test.

@bcoe working on a job here to test it out: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-new

@bcoe Nice! I think this is ready for merging?

@addaleax I thought of a slightly less invasive patch to correct the one test that was failing, I'll get this checked in tonight; also, it would be good to wait for the experimenting that @mhdawson is doing.

@joyeecheung confirmed that this failure wasn't coverage related, setting an environment variable correctly would suppress future warnings; easy fix is simply calling env->EmitProcessEnvWarning() last.

Is this change relevant to this PR (making a switch in coverage)?

Turning on coverage surfaced this bug, and made this test not run if coverage was enabled. I figured it was an innocent enough change that it was worth patching; If you'd like me to break it out into another patch I can.

BTW: After this PR will the test suite in make coverage pass (RE: #25543 (comment))

@refack not quite, there are a couple addon tests that fail; I think related to the inspector session creating resources that break a few assertions.

I would be happy to help isolate these, and maybe we could use TEST_TO_SKIP_FOR_COVERAGE to remove them, then maybe we could get to the point where @mhdawson and I can abort on failure for coverage tests?

Would folks be okay with keeping this patch in, if @mhdawson and I work on making this pull request run coverage without ignoring errors?

Latest CI run (new job to test out) ran ok, and generated coverage results and artifacts.

I do see that some of the end brackets are not covered (already mentioned by @bcoe ).

A few other things I've noticed so far:

It shows significantly less branch coverage. For example in internal/crypto/random.js.html it shows additions lines (for example 64x return offset >>> 0; // Convert to uint32. not being covered. For the file it reports 76.36% as opposed to 93.02%. It shows 42/55 branches but I only see 5 highlighted (including the ones not highlighted by the existing reports) as opposed to 13 reported as not covered.
the path shows with lib instead of root, which is an improvement :). For example: / lib/internal/crypto random.js instead of / root/internal/crypto random.js

Also noticed it does not show coverage as missing in:

/lib/internal/fs/utils.js.html

It shows these 2 paths as being covered, where as the existing report does not.

if (throwError) {
    Error.stackTraceLimit = 0;
  }
  const err = new ERR_INVALID_ARG_VALUE(
    propName,
    path,
    'must be a string or Uint8Array without null bytes'
  );
 
 if (throwError) {
    Error.stackTraceLimit = tmpLimit;
    Error.captureStackTrace(err, nullCheck);
    throw err;
  }
  return err;

@thefourtheye I think I've addressed your review.

@mhdawson I've rebased this with the back ported v8 changes, and am quite happy with how things are looking; you should be able to run your coverage tests against this pull now.

Coverage tests failing should no longer break coverage report outputting -- going forward, I can certainly get onboard with the argument that we should fail tests, maybe instead of reporting on failure, we should identify the specific tests that fail and add them to an ignore list?

@mhdawson and I are having a discussion around this right now -- currently (and I believe historically) there are a small minority of tests that fail when under coverage, and we've simply opted to not fail when running under coverage and still run reports.

If we start failing the coverage test suite on failures, we'll need top explicitly exclude a few tests, and be more diligent about not introducing new tests that fail under coverage going forward.

Turning on coverage surfaced this bug, and made this test not run if coverage was enabled. I figured it was an innocent enough change that it was worth patching; If you'd like me to break it out into another patch I can.

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

we should identify the specific tests that fail and add them to an ignore list?

Could it be possible that a file with a lot of tests in it fail during the coverage? If so, we can work towards breaking them down to multiple smaller tests.

Left some naming and semantic comments

BTW: After this PR will the test suite in make coverage pass (RE: #25543 (comment))

Could the list of tests be refactored to a new var (TEST_TO_SKIP_FOR_COVERAGE)? and D.R.Y. L307?

I wonder if we could use TEST_TO_SKIP_FOR_COVERAGE to isolate the tests that fail under coverage (@mhdawson?) and stop using - to suppress erros.

I'm not sure we should recommend the use of CI_* variables to the public. AFAICT they are meant for internal use by our CI systems, and should be considered "private".

@refack a few things I think we need to discuss with @mhdawson.

I wonder if we could use TEST_TO_SKIP_FOR_COVERAGE to isolate the tests that fail under coverage (@mhdawson?) and stop using - to suppress erros.

@refack not quite, there are a couple addon tests that fail; I think related to the inspector session creating resources that break a few assertions.

I would be happy to help isolate these, and maybe we could use TEST_TO_SKIP_FOR_COVERAGE to remove them, then maybe we could get to the point where @mhdawson and I can abort on failure for coverage tests?

Would folks be okay with keeping this patch in, if @mhdawson and I work on making this pull request run coverage without ignoring errors?

We have chosen to allow failing tests when run with coverage. With unlimited resources, we would fix that. I'd be concerned with the ongoing maintenance (for those keeping coverage going) as well as showing things that are covered as not-covered that we'd get from excluding tests.

My personal feeling is that allowing failing tests is a reasonable balance between work/benefit but of course I'm open to other thoughts.

Coverage CI run with all the changes - https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-new/9/

PR-URL: nodejs#25157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Landed in d1dee49

PR-URL: #25157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Update the comments for the `coverage` Makefile target. - Source files under `lib` are no longer instrumented since d1dee49 - Fix the name of the related CI job. PR-URL: #39365 Refs: #25157 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bcoe requested a review from mhdawson December 21, 2018 01:35

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Dec 21, 2018

bcoe requested a review from addaleax December 21, 2018 01:40

bcoe mentioned this pull request Dec 24, 2018

Node.js' internal scripts should be hidden in the inspector. #11893

Open

garthk mentioned this pull request Dec 31, 2018

Make coverage pluggable hapijs/lab#902

Closed

bcoe closed this Jan 1, 2019

bcoe force-pushed the native-coverage branch from 48527fc to da13c44 Compare January 1, 2019 22:57

bcoe reopened this Jan 2, 2019

bcoe force-pushed the native-coverage branch from e411a90 to 07b8fff Compare January 5, 2019 00:16

bcoe changed the title [WIP] test: switch to native v8 coverage test: switch to native v8 coverage Jan 5, 2019

bcoe added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 5, 2019

bcoe commented Jan 5, 2019

View reviewed changes

bcoe added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests. labels Jan 5, 2019

addaleax approved these changes Jan 7, 2019

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 7, 2019

bcoe mentioned this pull request Jan 8, 2019

process: refactor coverage setup during bootstrap #25398

Closed

2 tasks

bcoe commented Jan 9, 2019

View reviewed changes

bcoe force-pushed the native-coverage branch 2 times, most recently from 39c3179 to 5730abd Compare January 9, 2019 01:52

bcoe force-pushed the native-coverage branch from f61a979 to 3c08794 Compare January 17, 2019 01:42

bcoe commented Jan 17, 2019

View reviewed changes

thefourtheye approved these changes Jan 17, 2019

View reviewed changes

bcoe force-pushed the native-coverage branch from b70cc3c to a6c140b Compare January 17, 2019 18:17

refack suggested changes Jan 17, 2019

View reviewed changes

bcoe commented Jan 17, 2019

View reviewed changes

bcoe force-pushed the native-coverage branch from 00c77a6 to ffe1bdb Compare January 17, 2019 23:22

refack approved these changes Jan 17, 2019

View reviewed changes

bcoe force-pushed the native-coverage branch from eb6be14 to ffe1bdb Compare January 18, 2019 21:26

test: switch to native v8 coverage

8d13005

bcoe force-pushed the native-coverage branch from 1222cc6 to 8d13005 Compare January 18, 2019 21:47

bcoe mentioned this pull request Jan 18, 2019

process: check env->EmitProcessEnvWarning() last #25575

Closed

3 tasks

bcoe closed this Jan 20, 2019

bcoe deleted the native-coverage branch January 20, 2019 22:17

bcoe restored the native-coverage branch January 20, 2019 22:18

bcoe deleted the native-coverage branch January 20, 2019 22:21

addaleax pushed a commit that referenced this pull request Jan 23, 2019

test: switch to native v8 coverage

7490fc8

PR-URL: #25157 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

richardlau mentioned this pull request Jul 12, 2021

build: update coverage Makefile target comments #39365

Closed

test: switch to native v8 coverage by bcoe · Pull Request #25157 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: switch to native v8 coverage - #25157

test: switch to native v8 coverage#25157bcoe wants to merge 1 commit intonodejs:masternodejs/node:masterfrom bcoe:native-coveragebcoe/node-1:native-coverageCopy head branch name to clipboard

Conversation

bcoe commented Dec 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Dec 21, 2018

Uh oh!

bcoe commented Dec 21, 2018

Uh oh!

bcoe commented Dec 31, 2018

Uh oh!

hashseed commented Dec 31, 2018

Uh oh!

mhdawson commented Jan 4, 2019

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 7, 2019

Uh oh!

addaleax commented Jan 7, 2019

Uh oh!

bcoe commented Jan 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 9, 2019

Uh oh!

mhdawson commented Jan 9, 2019

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thefourtheye commented Jan 17, 2019

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

test: switch to native v8 coverage#25157
bcoe wants to merge 1 commit into
nodejs:masterfrom
bcoe:native-coverage

bcoe commented Dec 21, 2018 •
edited
Loading

joyeecheung Jan 10, 2019 •
edited
Loading

bcoe commented Jan 17, 2019 •
edited
Loading