process: start coverage collection before bootstrap by joyeecheung · Pull Request #26006 · nodejs/node · GitHub

This patch moves the dispatch of Profiler.takePreciseCoverage
to a point before the bootstrap scripts are run to ensure that
we can collect coverage data for all the scripts run after
the inspector agent is ready.

Before this patch lib/internal/bootstrap/primordials.js was not
covered by make coverage, after this patch it is.

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

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is.

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

This also seems to fix the async-hooks/test-callback-error error in make coverage for me - probably because it no longer triggers the creation of an additional JSBindingsConnection (AsyncWrap) anymore.

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

Addressed reviews from @addaleax and fixed the quotes in the warning. Also I notice that sometimes undefined could be written to the file, so I've restored to an approach similar to the previous one and always overwrite the file in the callback once the callback is set.

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

this seems to be async now? should the try/catch be moved to the callback?

this looks good to me, and I'm excited that we'll be able to collect coverage for a few of the internal libraries we're missing right now.

Before we land this, I'd like to test the branch with a few user-land modules, paying special attention to how subprocesses are handled. I will have cycles to do so this weekend.

nice, seems like a huge benefit to move coverage standup and teardown into C++.

Looks like there are related failures in CI:

Windows:

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

134 !== 0

    at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-v8-coverage.js:92:10)

--without-intl:

/home/iojs/build/workspace/node-test-commit-linux-containered/out/Release/obj.target/node_lib/src/node_binding.o: In function `node::binding::RegisterBuiltinModules()':
node_binding.cc:(.text+0x14ae): undefined reference to `_register_coverage()'
collect2: error: ld returned 1 exit status

The failures on Windows:

C:\Users\joyee\projects\node>C:\Users\joyee\projects\node\Release\node.exe C:\Users\joyee\projects\node\test\parallel\test-v8-coverage.js
uv loop at [000001D549520AD0] has 2 active handles
[000001D54AF20CE0] async
        Close callback: 00007FF6CDFC2C40 <lambda_1e91a1e10c4ec4799db2c780f02b0099>::<lambda_invoker_cdecl> [c:\users\joyee\projects\node\src\node_platform.cc]:L275
        Data: 000001D54AEE2830
        (First field): 00019A740001A116
[000001D54AF21730] async
        Close callback: 00007FF6CDF4D1A0 node::inspector::`anonymous namespace'::DisposePairCallback [c:\users\joyee\projects\node\src\inspector\main_thread_interface.cc]:L99
        Data: 0000000000000000
Command Prompt - C:\Users\joyee\projects\node\Release\node.exe  C:\Users\joyee\projects\node\test\parallel\test-v8-coverage.js[7760]: c:\users\joyee\projects\node\src\debug_utils.cc:290: Assertion `0 && "uv_loop_close() while having open handles"' failed.
 1: 00007FF6CE0A723F node::DumpBacktrace+143 [c:\users\joyee\projects\node\src\debug_utils.cc]:L276
 2: 00007FF6CE047BD6 node::Abort+22 [c:\users\joyee\projects\node\src\node_errors.cc]:L166
 3: 00007FF6CE0481A3 node::Assert+131 [c:\users\joyee\projects\node\src\node_errors.cc]:L183
 4: 00007FF6CE0A6E28 node::CheckedUvLoopClose+216 [c:\users\joyee\projects\node\src\debug_utils.cc]:L290
 5: 00007FF6CDFA251E node::worker::Worker::`scalar deleting destructor'+126
 6: 00007FF6CE0A008D node::Environment::RunCleanup+701 [c:\users\joyee\projects\node\src\env.cc]:L524
 7: 00007FF6CE073836 node::Start+1334 [c:\users\joyee\projects\node\src\node.cc]:L831
 8: 00007FF6CE0731EE node::Start+382 [c:\users\joyee\projects\node\src\node.cc]:L877
 9: 00007FF6CE072D30 node::Start+1040 [c:\users\joyee\projects\node\src\node.cc]:L937
10: 00007FF6CDECC58C wmain+444 [c:\users\joyee\projects\node\src\node_main.cc]:L72
11: 00007FF6CEFE819C __scrt_common_main_seh+268 [d:\agent\_work\3\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl]:L288
12: 00007FF8B5643034 BaseThreadInitThunk+20
13: 00007FF8B7F13691 RtlUserThreadStart+33

I'll try looking into it later. (Meanwhile the test probably needs to reverse the order of checks to print something more helpful on failures..)

quick update, haven't tested on any user-land modules yet; compiling the branch right now and will test tomorrow.

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: nodejs#26089 Refs: nodejs#26006

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

I haven’t tried it for this PR myself (yet), but I think #26138 and #26137 should take care of the Windows failures?

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

New CI: https://ci.nodejs.org/job/node-test-pull-request/20845/

@addaleax @joyeecheung I apologize for the slow turnaround, I've tested this change on a few user-land modules and it's working great 👍

as soon as tests pass, +1 from me.

Added HAVE_INSPECTOR guard for the coverage binding.

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

Landed in 2ae45d3

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This is redundant to the platform notification mechanism, and the handle may not be cleaned up util we attempt to close the loop. Refs: #26089 Refs: #26006 PR-URL: #26137 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: #26089 Refs: #26006 PR-URL: #26138 Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

This patch moves the dispatch of `Profiler.takePreciseCoverage` to a point before the bootstrap scripts are run to ensure that we can collect coverage data for all the scripts run after the inspector agent is ready. Before this patch `lib/internal/bootstrap/primordials.js` was not covered by `make coverage`, after this patch it is. PR-URL: #26006 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

joyeecheung requested a review from bcoe February 8, 2019 12:53

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 8, 2019

addaleax approved these changes Feb 8, 2019

View reviewed changes

Comment thread src/inspector_coverage.cc Outdated

Comment thread src/inspector_coverage.cc Outdated

Comment thread src/inspector_coverage.cc Outdated

joyeecheung added 2 commits February 8, 2019 22:24

fixup! process: start coverage collection before bootstrap

0bf9ae9

fixup! fixup! process: start coverage collection before bootstrap

44ff18b

targos reviewed Feb 8, 2019

View reviewed changes

bcoe approved these changes Feb 8, 2019

View reviewed changes

jasnell approved these changes Feb 12, 2019

View reviewed changes

addaleax mentioned this pull request Feb 15, 2019

test,inspector: add heap allocation tracker test #26089

Closed

3 tasks

addaleax mentioned this pull request Feb 15, 2019

src: remove inspector main_thread_request_ field #26137

Closed

2 tasks

addaleax added a commit to addaleax/node that referenced this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop

06e09f7

On Windows, the Platform’s `uv_async_t` may need two iterations before closing when it was previously in use. Refs: nodejs#26089 Refs: nodejs#26006

addaleax mentioned this pull request Feb 15, 2019

worker: spin uv_run twice before closing loop #26138

Closed

2 tasks

fixup! fixup! fixup! process: start coverage collection before bootstrap

de2845b

joyeecheung closed this Feb 18, 2019

BridgeAR mentioned this pull request Feb 26, 2019

v11.11.0 proposal #26322

Merged

-                  }));
-                  const coverageInfo = JSON.parse(msg).result;
-                  writeFileSync(target, JSON.stringify(coverageInfo));
+                  internalBinding('coverage').end((msg) => {

+                  explicit V8CoverageSessionDelegate(V8CoverageConnection* connection)
+                      : connection_(connection) {}
+                  void SendMessageToFrontend(

process: start coverage collection before bootstrap by joyeecheung · Pull Request #26006 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: start coverage collection before bootstrap - #26006

process: start coverage collection before bootstrap#26006joyeecheung wants to merge 4 commits intonodejs:masternodejs/node:masterfrom joyeecheung:coverage-cppjoyeecheung/node:coverage-cppCopy head branch name to clipboard

Conversation

joyeecheung commented Feb 8, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 8, 2019

Uh oh!

joyeecheung commented Feb 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joyeecheung commented Feb 8, 2019

Uh oh!

targos Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 9, 2019

Uh oh!

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

Uh oh!

Uh oh!

bcoe commented Feb 12, 2019

Uh oh!

addaleax commented Feb 15, 2019

Uh oh!

addaleax commented Feb 17, 2019

Uh oh!

bcoe commented Feb 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

joyeecheung commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

process: start coverage collection before bootstrap#26006
joyeecheung wants to merge 4 commits into
nodejs:masterfrom
joyeecheung:coverage-cpp

joyeecheung commented Feb 12, 2019 •
edited
Loading

joyeecheung commented Feb 18, 2019 •
edited
Loading