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.

Sign up for GitHub

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

Jump to bottom

lib: fix coverage reporting #20035

Closed
wants to merge 2 commits into from
Closed

lib: fix coverage reporting #20035

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

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

@addaleax
lib: fix coverage reporting
5fcf108 
Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: nodejs#19912
Refs: nodejs#19524
@addaleax addaleax added build Issues and PRs related to build files or the CI. dont-land-on-v6.x labels Apr 14, 2018
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Apr 14, 2018
@addaleax addaleax added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 14, 2018
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/17759/

@richardlau
Copy link
Member

CI failure looks related, e.g., https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1010/17833/consoleFull

npm ERR! weird error structured-stack:1
npm ERR! weird error function() {
npm ERR! weird error         ^
npm ERR! weird error 
npm ERR! weird error SyntaxError: Unexpected token (
npm ERR! weird error     at new Script (vm.js:74:7)
npm ERR! weird error     at createScript (vm.js:246:10)
npm ERR! weird error     at runInNewContext (vm.js:291:10)
npm ERR! weird error     at isInsideNodeModules (internal/util.js:393:26)
npm ERR! weird error     at showFlaggedDeprecation (buffer.js:149:8)
npm ERR! weird error     at new Buffer (buffer.js:174:3)
npm ERR! weird error     at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/deps/npm/node_modules/mississippi/node_modules/duplexify/index.js:6:20)
npm ERR! weird error     at Module._compile (internal/modules/cjs/loader.js:678:30)
npm ERR! weird error     at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
npm ERR! weird error     at Module.load (internal/modules/cjs/loader.js:589:32)
make[2]: *** [tools/doc/node_modules/js-yaml/package.json] Error 1
make[1]: *** [doc-only] Error 2
make[1]: *** Waiting for unfinished jobs....

richardlau
richardlau previously requested changes Apr 15, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails CI. Feel free to dismiss my review if I'm not around when this is addressed.

lpinca
lpinca reviewed Apr 15, 2018
View reviewed changes
lib/internal/util.js Outdated
@@ -390,17 +390,16 @@ function isInsideNodeModules() {
// Use `runInNewContext()` to get something tamper-proof and
// side-effect-free. Since this is currently only used for a deprecated API,
// the perf implications should be okay.
getStructuredStack = runInNewContext('(' + function() {
getStructuredStack = runInNewContext(`function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a leading ( has been accidentally removed.

@addaleax
Copy link
Member Author

New CI: https://ci.nodejs.org/job/node-test-commit/17769/

@trivikr
Copy link
Member

trivikr commented Apr 15, 2018

The next coverage will start in ~10 hours at 0300 UTC
This PR is being fast-tracked and has got 5 approvals already. So, it should be good to merge by then.

@addaleax
Copy link
Member Author

Landed in 85e1819

@addaleax addaleax closed this Apr 15, 2018
@addaleax addaleax deleted the fix-cov branch April 15, 2018 21:18
addaleax added a commit that referenced this pull request Apr 15, 2018
@addaleax
lib: fix coverage reporting
85e1819 
Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

PR-URL: #20035
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
@addaleax @jasnell
lib: fix coverage reporting
445a89f 
Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

PR-URL: #20035
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@snyk-bot snyk-bot mentioned this pull request Jun 3, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Jun 4, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Jun 14, 2020
Open
@kissofire kissofire mentioned this pull request Nov 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@richardlau richardlau richardlau left review comments

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

coverage.nodejs.org misreporting
10 participants
@addaleax @richardlau @trivikr @jasnell @lpinca @TimothyGu @cjihrig @BridgeAR @ryzokuken @nodejs-github-bot