Something wrong with coverage stats recently? #32912
Comments
|
If it was due to a code change it was between these commits: 6ed912d...c849f2d
|
|
https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/486/nodes=benchmark/console is the job that generated coverage for c849f2d. https://codecov.io/github/nodejs/node/commit/c849f2d4f8ff19c830b6a97bd6978bf41f878e8b is the codecov.io that was uploaded from that job. The discrepency is certainly odd. |
|
Looks like the cctest is crashing in https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/486/nodes=benchmark/consoleFull which means the js tests aren't being run: |
|
Crashes when |
Looking at the commits in that range, the most likely suspect is 8aa7ef7 (#32672), particularly as the inspector is showing up in the stack frames of the back trace where the crash occurred. I've run the coverage job (in non-publish mode) against a branch with 8aa7ef7 reverted and it looks to have avoided the crash and is back to the coverage numbers we're used to: https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/499/nodes=benchmark/console cc @addaleax |
3 tasks
|
It seems the true cause was 5bf4372 which changes it to |
|
There should be multiple way to fix this
|
|
Upon further inspection, simply give up for those half-baked Environments should make more sense - porting the coverage serialization in C++ might not be worth it, considering we'll always need to reach into the JS land to compute source maps for user modules. This should at least fix the issue for cctests, I am verifying if there are other places where this mismatch could lead to issues. This does mean that we won't have JS coverage data from code run in most cctests (that don't do pre-execution), but I guess that's fine. diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc
index cc4c309175..03cf2f6e5c 100644
--- a/src/inspector_profiler.cc
+++ b/src/inspector_profiler.cc
@@ -209,6 +209,16 @@ void V8CoverageConnection::WriteProfile(Local<String> message) {
HandleScope handle_scope(isolate);
Context::Scope context_scope(context);
+ // This is only set up during pre-execution (when the environment variables
+ // becomes available in the JS land). If it's empty, we don't have coverage
+ // directory path (which is resolved in JS land at the moment) either, so
+ // the best we could to is to just discard the profile and do nothing.
+ // This should only happen in half-baked Environments created using the
+ // embedder API.
+ if (env_->source_map_cache_getter().IsEmpty()) {
+ return;
+ }
+
// Get message.result from the response.
Local<Object> result;
if (!ParseProfile(env_, message, type()).ToLocal(&result)) { |
|
Hm, another part of the cause was #31910 - before that we always run pre-execution for |
joyeecheung
added a commit
to joyeecheung/node
that referenced
this issue
Apr 21, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: nodejs#32912 Refs: nodejs@65e18a8 Refs: nodejs@5bf4372 nodejs@8aa7ef7
4 tasks
|
Fix in #32960 JS coverage got back to 96.58% locally. C++ coverage doesn't work on my machine anyway. |
BethGriggs
pushed a commit
that referenced
this issue
Apr 27, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR
pushed a commit
that referenced
this issue
Apr 28, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR
pushed a commit
that referenced
this issue
Apr 28, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs
pushed a commit
that referenced
this issue
Apr 28, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos
pushed a commit
that referenced
this issue
Apr 30, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos
pushed a commit
that referenced
this issue
May 13, 2020
The NODE_V8_COVERAGE folder and the source map computation are setup during pre-execution since they rely on environment variables as well as JS states. Therefore, we need to give up serialization of JS coverage profiles for Environments that have not go through pre-execution. Currently this is only possible for Environments created using the embedder API CreateEnvironment(). As a result we won't have JS coverage data for most cctests, but if that proves to be necessary we could just run lib/internal/main/environment.js for these Environments created for cctests. Fixes: #32912 Refs: 65e18a8 Refs: 5bf4372 8aa7ef7 PR-URL: #32960 Refs: 8aa7ef7 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
On or around April 9, coverage.nodejs.org shows coverage plummeting on both JS and C++. Seems like some kind of problem was introduced into the coverage job? codecov.io results seem fine, in comparison.
The text was updated successfully, but these errors were encountered: