You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Something wrong with coverage stats recently? #32912
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.
If it was due to a code change it was between these commits: 6ed912d...c849f2d
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:
It seems the true cause was 5bf4372 which changes it to EndStartedProfilers() at exit unconditionally (which relies on callbacks setup in pre-execution), instead of only in places where there is a fully-baked Environment. It happened to be fine until 8aa7ef7 since before that we always only call StartProfilers in Environments that will be fully baked, but 8aa7ef7 made the half-baked Environments used in the cctest hit this bug. (i.e. when LoadEnvironment() is not called to set up the source map cache getter at pre-execution)
Make the profile serialization return early from pre-execution (makes sense because environment variables are only processed at pre-execution, well in JS anyway) and ignore the commands to write the profiles
Move the source map caching stuff into C++ so they'll always be functional, even in half-baked Environments,. This kind of diagnostics runtime states in C++ usually doesn't matter in snapshots so keeping them out of the JS world should be OK (what V8 does is to just discard debugger states in snapshot and always start from scratch anyway)
Somehow make sure that we only start the profilers when we are sure that the Environment will be fully baked. I am not sure how that's possible if we want to have --inspect-brk-node for half-baked Environments (for snapshots)
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 CreateEnvironment(), so another possible fix would be to implement what I suggested in #31910 (comment) and introduce a separate step to fully bootstrap the environment (maybe an internal API would already suffice), then we will still have JS coverage from cctest. This would also re-enable that cctest being commented out in #31910
Activity
richardlau commentedon Apr 18, 2020
If it was due to a code change it was between these commits: 6ed912d...c849f2d
richardlau commentedon Apr 18, 2020
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.
richardlau commentedon Apr 18, 2020
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:
richardlau commentedon Apr 20, 2020
Crashes when
NODE_V8_COVERAGEis set. Here's a stack trace for the crash with a Debug build:richardlau commentedon Apr 20, 2020
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
joyeecheung commentedon Apr 20, 2020
It seems the true cause was 5bf4372 which changes it to
EndStartedProfilers()at exit unconditionally (which relies on callbacks setup in pre-execution), instead of only in places where there is a fully-baked Environment. It happened to be fine until 8aa7ef7 since before that we always only callStartProfilers inEnvironments that will be fully baked, but 8aa7ef7 made the half-baked Environments used in the cctest hit this bug. (i.e. whenLoadEnvironment()is not called to set up the source map cache getter at pre-execution)joyeecheung commentedon Apr 20, 2020
There should be multiple way to fix this
--inspect-brk-nodefor half-baked Environments (for snapshots)joyeecheung commentedon Apr 20, 2020
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.
joyeecheung commentedon Apr 20, 2020
Hm, another part of the cause was #31910 - before that we always run pre-execution for
CreateEnvironment(), so another possible fix would be to implement what I suggested in #31910 (comment) and introduce a separate step to fully bootstrap the environment (maybe an internal API would already suffice), then we will still have JS coverage from cctest. This would also re-enable that cctest being commented out in #31910inspector: only write coverage in fully bootstrapped Environments
joyeecheung commentedon Apr 21, 2020
Fix in #32960 JS coverage got back to 96.58% locally. C++ coverage doesn't work on my machine anyway.
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments
inspector: only write coverage in fully bootstrapped Environments