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

process: expose process.features.inspector #25819

Closed
wants to merge 4 commits into from
Closed

process: expose process.features.inspector #25819

wants to merge 4 commits into from

Conversation

joyeecheung
Copy link
Member

Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.

Refs: #25343

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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. util Issues and PRs related to the built-in util module. labels Jan 30, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung
process: expose process.features.inspector
731f799 
Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.
@joyeecheung
Copy link
Member Author

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

richardlau
richardlau reviewed Jan 30, 2019
View reviewed changes
test/common/index.js Outdated
skip('V8 inspector is disabled');
}
}

function skipIfInspectorEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's worth increasing the surface area of common for this as it's only used in one place?

@richardlau
Copy link
Member

withoutintl build test failures are because hasInspector is currently only defined if NODE_HAVE_I18N_SUPPORT so process.features.inspector ends up being undefined instead of a boolean:

node/src/node_config.cc

Lines 48 to 70 in 5860614

#ifdef NODE_HAVE_I18N_SUPPORT
READONLY_TRUE_PROPERTY(target, "hasIntl");
#ifdef NODE_HAVE_SMALL_ICU
READONLY_TRUE_PROPERTY(target, "hasSmallICU");
#endif // NODE_HAVE_SMALL_ICU
#if NODE_USE_V8_PLATFORM
READONLY_TRUE_PROPERTY(target, "hasTracing");
#endif
#if HAVE_INSPECTOR
READONLY_TRUE_PROPERTY(target, "hasInspector");
#else
READONLY_FALSE_PROPERTY(target, "hasInspector");
#endif
#if !defined(NODE_WITHOUT_NODE_OPTIONS)
READONLY_TRUE_PROPERTY(target, "hasNodeOptions");
#endif
#endif // NODE_HAVE_I18N_SUPPORT

e.g.

not ok 1449 parallel/test-process-features 
11:23:42 not ok 1449 parallel/test-process-features
11:23:42   ---
11:23:42   duration_ms: 0.147
11:23:42   severity: fail
11:23:42   exitcode: 1
11:23:42   stack: |-
11:23:42     assert.js:86
11:23:42       throw new AssertionError(obj);
11:23:42       ^
11:23:42     
11:23:42     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
11:23:42     + actual - expected
11:23:42     
11:23:42     + 'undefined'
11:23:42     - 'boolean'
11:23:42         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-process-features.js:20:10)
11:23:42         at Module._compile (internal/modules/cjs/loader.js:737:30)
11:23:42         at Object.Module._extensions..js (internal/modules/cjs/loader.js:748:10)
11:23:42         at Module.load (internal/modules/cjs/loader.js:629:32)
11:23:42         at tryModuleLoad (internal/modules/cjs/loader.js:572:12)
11:23:42         at Function.Module._load (internal/modules/cjs/loader.js:564:3)
11:23:42         at Function.Module.runMain (internal/modules/cjs/loader.js:802:12)
11:23:42         at internal/main/run_main_module.js:27:11
11:23:42   ...

@joyeecheung
Copy link
Member Author

@richardlau Thanks for the analysis, I've removed skipIfInspectorEnabled from test/common/index.js and pulled the hasInspector property initialization out of the NODE_HAVE_I18N_SUPPORT branch - it should be fine to use HAVE_INSPECTOR alone for this.

@joyeecheung
Copy link
Member Author

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

refack
refack reviewed Jan 30, 2019
View reviewed changes
tools/test.py Outdated Show resolved Hide resolved
@refack @joyeecheung
fixup! process: expose process.features.inspector
137426e 
Co-Authored-By: joyeecheung <joyeec9h3@gmail.com>
@joyeecheung joyeecheung added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 31, 2019
@joyeecheung
Copy link
Member Author

Previous fixup didn't get the test.py indentation right. Fixed. CI: https://ci.nodejs.org/job/node-test-pull-request/20499/

@joyeecheung joyeecheung reopened this Jan 31, 2019
@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2019
@joyeecheung
Copy link
Member Author

Landed in c2359bd

@joyeecheung joyeecheung closed this Feb 1, 2019
joyeecheung added a commit that referenced this pull request Feb 1, 2019
@joyeecheung
process: expose process.features.inspector
c2359bd 
Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.

PR-URL: #25819
Refs: #25343
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

addaleax commented Feb 1, 2019

@joyeecheung This also needs to be backported to v11.x manually

@targos targos mentioned this pull request Feb 6, 2019
Closed
targos pushed a commit that referenced this pull request Feb 10, 2019
@joyeecheung @targos
process: expose process.features.inspector
1d76ba1 
Instead of using process.config.variables.v8_enable_inspector
to detect whether inspector is enabled in the build.

PR-URL: #25819
Refs: #25343
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@freenice12 freenice12 mentioned this pull request Feb 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack left review comments

@eugeneo eugeneo eugeneo approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@devsnek devsnek devsnek approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. 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.

None yet