src: refactor coverage connection by joyeecheung · Pull Request #26513 · nodejs/node

joyeecheung · 2019-03-08T13:04:58Z

src: refactor coverage connection

Refactor the C++ class to be resuable for other types of profiles
Move the try-catch block around coverage collection callback
to be inside the callback to silence potential JSON or write
errors.
Use Function::Call instead of MakeCallback to call the coverage
message callback since it does not actually need async hook
handling. This way we no longer needs to disable the async
hooks when writing the coverage results.
Renames lib/internal/coverage-gen/with_profiler.js to
lib/internal/profiler.js because it is now the only way
to generate coverage.

test: show stderr on v8 coverage test failures

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

nodejs-github-bot · 2019-03-08T13:05:01Z

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

joyeecheung · 2019-03-08T13:06:14Z

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

gengjiawen · 2019-03-08T14:52:55Z

Use raw string here ?

bcoe

This looks reasonable to me 👍 It's nice to be able to remove the confusing disableAllAsyncHooks methods.

Let's just make sure we run the coverage CI against this before we land? @mhdawson how far did we get on making this run as part of the smoke testing process?

- Refactor the C++ class to be resuable for other types of profiles - Move the try-catch block around coverage collection callback to be inside the callback to silence potential JSON or write errors. - Use Function::Call instead of MakeCallback to call the coverage message callback since it does not actually need async hook handling. This way we no longer needs to disable the async hooks when writing the coverage results. - Renames `lib/internal/coverage-gen/with_profiler.js` to `lib/internal/profiler.js` because it is now the only way to generate coverage.

joyeecheung · 2019-03-11T19:14:31Z

Rebased and addressed reviews. CI: https://ci.nodejs.org/job/node-test-pull-request/21434/

@gengjiawen Regarding raw strings: I have no idea why but it looks like neither we nor non-test V8 code use those, so I'd prefer leaving that out of this PR, in case there's any weird compiler support oddities. Feel free to open a separate PR to update those.

joyeecheung · 2019-03-11T23:03:12Z

I don't think there is a job to run the coverage CI against a PR. I do get a successful run locally though. If this comes back breaking the coverage, we can just revert.

joyeecheung · 2019-03-11T23:04:46Z

Landed in 963ee0b...eb0bf8d

- Refactor the C++ class to be resuable for other types of profiles - Move the try-catch block around coverage collection callback to be inside the callback to silence potential JSON or write errors. - Use Function::Call instead of MakeCallback to call the coverage message callback since it does not actually need async hook handling. This way we no longer needs to disable the async hooks when writing the coverage results. - Renames `lib/internal/coverage-gen/with_profiler.js` to `lib/internal/profiler.js` because it is now the only way to generate coverage. PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

BridgeAR · 2019-03-13T15:43:28Z

This does not land cleanly on v11. It seems to rely on other commits that should be backported first. Please open a manual backport for it or change the labels accordingly.

- Refactor the C++ class to be resuable for other types of profiles - Move the try-catch block around coverage collection callback to be inside the callback to silence potential JSON or write errors. - Use Function::Call instead of MakeCallback to call the coverage message callback since it does not actually need async hook handling. This way we no longer needs to disable the async hooks when writing the coverage results. - Renames `lib/internal/coverage-gen/with_profiler.js` to `lib/internal/profiler.js` because it is now the only way to generate coverage. PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

- Refactor the C++ class to be resuable for other types of profiles - Move the try-catch block around coverage collection callback to be inside the callback to silence potential JSON or write errors. - Use Function::Call instead of MakeCallback to call the coverage message callback since it does not actually need async hook handling. This way we no longer needs to disable the async hooks when writing the coverage results. - Renames `lib/internal/coverage-gen/with_profiler.js` to `lib/internal/profiler.js` because it is now the only way to generate coverage. PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 8, 2019

joyeecheung changed the title ~~Refactor coverage~~src: refactor coverage connectionMar 8, 2019

joyeecheung requested review from addaleax and bcoe March 8, 2019 13:07

addaleax added the coverage Issues and PRs related to native coverage support. label Mar 8, 2019

addaleax approved these changes Mar 8, 2019
View reviewed changes

Comment threadsrc/inspector_profiler.cc Outdated

gengjiawen reviewed Mar 8, 2019
View reviewed changes

Comment threadsrc/inspector_profiler.cc Outdated
gengjiawenMar 8, 2019
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use raw string here ?

gengjiawen reviewed Mar 8, 2019
View reviewed changes

Comment threadsrc/inspector_profiler.cc Outdated

gengjiawen reviewed Mar 8, 2019
View reviewed changes

Comment threadsrc/inspector_profiler.cc Outdated

bcoe approved these changes Mar 11, 2019
View reviewed changes

joyeecheung added 2 commits March 12, 2019 03:08

test: show stderr on v8 coverage test failures
a709987

joyeecheung force-pushed the refactor-coverage branch from 92ffa3b to a709987Compare March 11, 2019 19:12

joyeecheung closed this Mar 11, 2019

joyeecheung added a commit that referenced this pull request Mar 11, 2019
test: show stderr on v8 coverage test failures
eb0bf8d
PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

richardlau mentioned this pull request Mar 12, 2019
test: whitelist the expected modules in test-bootstrap-modules.js #26531
Merged
3 tasks

BridgeAR added the backport-requested-v11.x label Mar 13, 2019

targos removed the backport-requested-v11.x label Mar 28, 2019

targos pushed a commit that referenced this pull request Mar 28, 2019
test: show stderr on v8 coverage test failures
91fcb02
PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

targos pushed a commit that referenced this pull request Mar 30, 2019
test: show stderr on v8 coverage test failures
03bd649
PR-URL: #26513 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Coe <bencoe@gmail.com>

BethGriggs mentioned this pull request Apr 9, 2019
v11.14.0 proposal #27163
Merged

Navigation Menu

Uh oh!

Uh oh!

src: refactor coverage connection - #26513

src: refactor coverage connection#26513
joyeecheung wants to merge 2 commits into
nodejs:masterfrom
joyeecheung:refactor-coverage

joyeecheung commented Mar 8, 2019

Uh oh!

nodejs-github-bot commented Mar 8, 2019

Uh oh!

joyeecheung commented Mar 8, 2019

Uh oh!

Uh oh!

gengjiawenMar 8, 2019

Uh oh!

Uh oh!

Uh oh!

bcoe left a comment

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

joyeecheung commented Mar 8, 2019

src: refactor coverage connection

test: show stderr on v8 coverage test failures

Checklist

Uh oh!

nodejs-github-bot commented Mar 8, 2019

Uh oh!

joyeecheung commented Mar 8, 2019

Uh oh!

Uh oh!

gengjiawenMar 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

joyeecheung commented Mar 11, 2019

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants