Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector: only write coverage in fully bootstrapped Environments #32960

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented 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: #32912
Refs: 65e18a8
Refs: 5bf4372
Refs: 8aa7ef7

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

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
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Apr 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@Trott Trott requested review from addaleax and bcoe April 21, 2020 05:47
@richardlau
Copy link
Member

Coverage: https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/501/nodes=benchmark/console

11:07:18 Gathered coveraged data for 209 files
11:07:18 Javascript coverage %: 96.68%
11:07:18 C++ coverage %: 89.5%

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

Landed in c61b388

addaleax pushed a commit that referenced this pull request Apr 24, 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>
@addaleax addaleax closed this Apr 24, 2020
BethGriggs pushed a commit that referenced this pull request 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>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
BridgeAR pushed a commit that referenced this pull request 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 pull request 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 BridgeAR mentioned this pull request Apr 28, 2020
BethGriggs pushed a commit that referenced this pull request 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 pull request 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>
@addaleax addaleax mentioned this pull request May 7, 2020
4 tasks
targos pushed a commit that referenced this pull request 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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Something wrong with coverage stats recently?
8 participants