process: expose process.features.inspector by joyeecheung · Pull Request #25819 · nodejs/node

joyeecheung · 2019-01-30T16:15:12Z

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

Checklist

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

nodejs-github-bot · 2019-01-30T16:15:17Z

@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

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

joyeecheung · 2019-01-30T16:17:57Z

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

richardlau · 2019-01-30T17:36:23Z

test/common/index.js

  }
 }

+function skipIfInspectorEnabled() {


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

richardlau · 2019-01-30T17:47:38Z

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 · 2019-01-30T18:40:16Z

@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 · 2019-01-30T18:40:52Z

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

tools/test.py

Co-Authored-By: joyeecheung <[email protected]>

joyeecheung · 2019-01-31T23:26:51Z

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

joyeecheung · 2019-02-01T21:42:13Z

Landed in 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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2019-02-01T22:14:59Z

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

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 <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #54982 Refs: aa0308d Refs: 9010f5f Refs: 52a40e0 Refs: b3ef289 Refs: #2564 Refs: #25819 Refs: #27311 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#54982 Refs: nodejs@aa0308d Refs: nodejs@9010f5f Refs: nodejs@52a40e0 Refs: nodejs@b3ef289 Refs: nodejs#2564 Refs: nodejs#25819 Refs: nodejs#27311 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #54982 Refs: aa0308d Refs: 9010f5f Refs: 52a40e0 Refs: b3ef289 Refs: #2564 Refs: #25819 Refs: #27311 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: nodejs#54982 Refs: nodejs@aa0308d Refs: nodejs@9010f5f Refs: nodejs@52a40e0 Refs: nodejs@b3ef289 Refs: nodejs#2564 Refs: nodejs#25819 Refs: nodejs#27311 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

PR-URL: #54982 Backport-PR-URL: #56948 Refs: aa0308d Refs: 9010f5f Refs: 52a40e0 Refs: b3ef289 Refs: #2564 Refs: #25819 Refs: #27311 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> PR-URL: #54897 Refs: #54295

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

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 force-pushed the inspector-feature branch from 479900c to 731f799 Compare January 30, 2019 16:17

eugeneo approved these changes Jan 30, 2019

View reviewed changes

devsnek approved these changes Jan 30, 2019

View reviewed changes

jasnell approved these changes Jan 30, 2019

View reviewed changes

richardlau reviewed Jan 30, 2019

View reviewed changes

fixup! process: expose process.features.inspector

66aebeb

richardlau approved these changes Jan 30, 2019

View reviewed changes

refack reviewed Jan 30, 2019

View reviewed changes

tools/test.py Outdated Show resolved Hide resolved

fixup! process: expose process.features.inspector

137426e

Co-Authored-By: joyeecheung <[email protected]>

cjihrig approved these changes Jan 30, 2019

View reviewed changes

addaleax approved these changes Jan 30, 2019

View reviewed changes

joyeecheung added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 31, 2019

fixup! fixup! process: expose process.features.inspector

67e20d1

joyeecheung closed this Jan 31, 2019

joyeecheung reopened this Jan 31, 2019

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 closed this Feb 1, 2019

addaleax added the backport-requested-v11.x label Feb 1, 2019

targos mentioned this pull request Feb 6, 2019

Help wanted: Backporting to v11.x #25403

Closed

targos removed the backport-requested-v11.x label Feb 10, 2019

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

freenice12 mentioned this pull request Feb 20, 2019

translate v11.10.0 nodejs/nodejs-ko#808

Merged

BethGriggs added the dont-land-on-v10.x label Aug 20, 2019

This was referenced Sep 12, 2024

doc: add documentation for process.features #54897

Merged

doc: fix history of process.features #54982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

process: expose process.features.inspector#25819

process: expose process.features.inspector#25819
joyeecheung wants to merge 4 commits intonodejs:masterfrom
joyeecheung:inspector-feature

joyeecheung commented Jan 30, 2019

Uh oh!

nodejs-github-bot commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

richardlau Jan 30, 2019

Uh oh!

richardlau commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

Uh oh!

joyeecheung commented Jan 31, 2019

Uh oh!

joyeecheung commented Feb 1, 2019

Uh oh!

addaleax commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Uh oh!

Conversation

joyeecheung commented Jan 30, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

richardlau Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

richardlau commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

joyeecheung commented Jan 30, 2019

Uh oh!

Uh oh!

joyeecheung commented Jan 31, 2019

Uh oh!

joyeecheung commented Feb 1, 2019

Uh oh!

addaleax commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants