test: switch to native v8 coverage #25157

bcoe · 2018-12-21T01:35:52Z

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

nodejs-github-bot · 2018-12-21T01:35:54Z

@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

bcoe · 2018-12-21T02:06:08Z

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

bcoe · 2018-12-31T16:20:25Z

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

hashseed · 2018-12-31T16:45:23Z

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

mhdawson · 2019-01-04T23:16:29Z

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,

bcoe

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

Makefile

bcoe · 2019-01-05T00:21:04Z

src/node_env_var.cc

@@ -69,6 +69,7 @@ static void EnvSetter(Local<Name> property,
  Environment* env = Environment::GetCurrent(info);
  if (env->options()->pending_deprecation && env->EmitProcessEnvWarning() &&
      !value->IsString() && !value->IsNumber() && !value->IsBoolean()) {
+    env->SetEmitProcessEnvWarning(true);


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.

mhdawson · 2019-01-07T23:06:29Z

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

addaleax · 2019-01-07T23:26:53Z

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

bcoe · 2019-01-07T23:46:06Z

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

bcoe · 2019-01-09T00:35:49Z

test/parallel/test-process-env-deprecation.js

@@ -12,5 +12,9 @@ common.expectWarning(
  'DEP0104'
 );

+// Make sure setting a valid environment variable doesn't
+// result in warning being suppressed, see:


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

mhdawson · 2019-01-09T17:11:23Z

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

mhdawson · 2019-01-09T17:21:02Z

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 · 2019-01-16T03:24:51Z

BUILDING.md

+Alternatively, for the JavaScript test suite, it's faster to run:
+
+```text
+$ CI_JS_SUITES=fs-mkdir CI_NATIVE_SUITES= make jscov


Nit: A note about unsetting CI_NATIVE_SUITES would help the readers.

Perhaps:

Note: CI_NATIVE_SUITES is unset, ensuring that the fs-mkdir tests are the only tests executed, otherwise the default suite of native tests will also be run.

Yup, that makes it crystal clear 🙂

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 we reference this variable earlier in the document already:

CI_JS_SUITES=child-process CI_NATIVE_SUITES= make coverage

and it's a nice way to kick off a partial suite of tests with coverage, which is somewhat involved:

you need to set the NODE_V8_COVERAGE variable to the appropriate path.

you need to know to run make js-coverage-report.

Long story short I'd like to expose something like this behavior to users, open to alternate suggestions for implementation.

thefourtheye · 2019-01-16T03:25:36Z

Makefile

+coverage-build-js:
+	mkdir -p node_modules
+	if [ ! -d node_modules/c8 ]; then \
+		$(NODE) ./deps/npm install c8@next --no-save --no-package-lock; fi


Nit: Can we have fi in a new line?

thefourtheye · 2019-01-16T03:27:23Z

src/node_env_var.cc

@@ -72,8 +72,9 @@ static void EnvSetter(Local<Name> property,
                      Local<Value> value,
                      const PropertyCallbackInfo<Value>& info) {
  Environment* env = Environment::GetCurrent(info);
-  if (env->options()->pending_deprecation && env->EmitProcessEnvWarning() &&
-      !value->IsString() && !value->IsNumber() && !value->IsBoolean()) {
+  if (env->options()->pending_deprecation && !value->IsString() &&


A comment indicating EmitProcessEnvWarning should be called at the end would help future readers and refactorings.

thefourtheye · 2019-01-16T03:32:16Z

Makefile

+	$(MAKE) coverage-build-js
+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage \
+	  CI_SKIP_TESTS=core_line_numbers.js,testFinalizer.js,test_function/test.js \
+    $(MAKE) jstest


Nit: This could be aligned with the previous line, right?

thefourtheye · 2019-01-16T03:42:15Z

Makefile

+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage \
+	  CI_SKIP_TESTS=core_line_numbers.js,testFinalizer.js,test_function/test.js \
+    $(MAKE) jstest
+	$(MAKE) coverage-report-js


Just trying to understand this better. Even if the previous command fails, does it make sense to run this command?

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

bcoe

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

bcoe · 2019-01-17T01:53:43Z

Makefile

+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage \
+	  CI_SKIP_TESTS=core_line_numbers.js,testFinalizer.js,test_function/test.js \
+    $(MAKE) jstest
+	$(MAKE) coverage-report-js


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

bcoe · 2019-01-17T02:00:00Z

test/parallel/test-process-env-deprecation.js

@@ -12,5 +12,9 @@ common.expectWarning(
  'DEP0104'
 );

+// Make sure setting a valid environment variable doesn't
+// result in warning being suppressed, see:


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.

bcoe · 2019-01-17T02:25:34Z

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

thefourtheye · 2019-01-17T16:08:39Z

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.

refack

Left some naming and semantic comments

test/parallel/test-bootstrap-modules.js

refack · 2019-01-17T18:56:07Z

test/parallel/test-process-env-deprecation.js

@@ -12,5 +12,9 @@ common.expectWarning(
  'DEP0104'
 );

+// Make sure setting a valid environment variable doesn't
+// result in warning being suppressed, see:


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

Makefile

refack · 2019-01-17T19:05:02Z

Makefile

+	$(RM) -r out/$(BUILDTYPE)/.coverage
+	$(MAKE) coverage-build-js
+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage \
+	  CI_SKIP_TESTS=core_line_numbers.js,testFinalizer.js,test_function/test.js \


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.

refack · 2019-01-17T19:07:19Z

BUILDING.md

+Alternatively, for the JavaScript test suite, it's faster to run:
+
+```text
+$ CI_JS_SUITES=fs-mkdir CI_NATIVE_SUITES= make jscov


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

bcoe

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

Makefile

bcoe · 2019-01-17T19:45:14Z

Makefile

+	$(RM) -r out/$(BUILDTYPE)/.coverage
+	$(MAKE) coverage-build-js
+	-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage \
+	  CI_SKIP_TESTS=core_line_numbers.js,testFinalizer.js,test_function/test.js \


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.

test/parallel/test-bootstrap-modules.js

bcoe · 2019-01-17T19:56:43Z

test/parallel/test-process-env-deprecation.js

@@ -12,5 +12,9 @@ common.expectWarning(
  'DEP0104'
 );

+// Make sure setting a valid environment variable doesn't
+// result in warning being suppressed, see:


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

mhdawson · 2019-01-17T20:11:51Z

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.

mhdawson · 2019-01-18T15:16:41Z

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>

bcoe · 2019-01-20T22:21:49Z

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 pending requests for changes, 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 pending requests for changes, 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

thefourtheye reviewed Jan 16, 2019

View reviewed changes

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

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 #25157

test: switch to native v8 coverage #25157

Conversation

bcoe commented Dec 21, 2018 • edited Loading

Checklist

nodejs-github-bot commented Dec 21, 2018

bcoe commented Dec 21, 2018

bcoe commented Dec 31, 2018

hashseed commented Dec 31, 2018

mhdawson commented Jan 4, 2019

bcoe left a 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

mhdawson commented Jan 7, 2019

addaleax commented Jan 7, 2019

bcoe commented Jan 7, 2019

Choose a reason for hiding this comment

joyeecheung Jan 10, 2019 • edited Loading

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

mhdawson commented Jan 9, 2019

mhdawson commented Jan 9, 2019

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

bcoe Jan 17, 2019 • edited Loading

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

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe commented Jan 17, 2019 • edited Loading

thefourtheye commented Jan 17, 2019

refack left a 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

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jan 17, 2019

mhdawson commented Jan 18, 2019

bcoe commented Jan 20, 2019

bcoe commented Dec 21, 2018 •

edited

Loading

joyeecheung Jan 10, 2019 •

edited

Loading

bcoe Jan 17, 2019 •

edited

Loading

bcoe commented Jan 17, 2019 •

edited

Loading