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: expose original console #21659

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jul 4, 2018

Adds require('inspector').console, mapping it to the original
global.console of V8. This enables applications to send messages to
the inspector console programmatically.

Fixes: #21651

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

@nodejs-github-bot nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Jul 4, 2018
@mcollina
Copy link
Member Author

mcollina commented Jul 4, 2018

cc @targos @devsnek

@mcollina mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 4, 2018
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

you might want to mention that node's console and v8's debug console don't have API parity. (e.g. a method might exist for one that doesn't with the other)

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM after the CI failures have been sorted out.

async function runTests() {
const script = `
require('inspector').console.log('hello world');
`;
Copy link
Member

Choose a reason for hiding this comment

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

No need for extraneous new lines.

assert.deepStrictEqual(msg.params.args, [{
type: 'string',
value: 'hello world'
}]);
Copy link
Member

Choose a reason for hiding this comment

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

This should also check if stdout is empty.

@Trott
Copy link
Member

Trott commented Jul 5, 2018

Here's the CI failure:

https://ci.nodejs.org/job/node-test-commit-linuxone/2755/nodes=rhel72-s390x/console

11:08:23 not ok 490 parallel/test-eslint-require-buffer
11:08:23   ---
11:08:23   duration_ms: 0.650
11:08:23   severity: fail
11:08:23   stack: |-
11:08:23     /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:148
11:08:23             throw err;
11:08:23             ^
11:08:23     
11:08:23     AssertionError [ERR_ASSERTION]: 'Use const { Buffer } = require(\'buffer\'); at the beginning of this file' strictEqual 'Use const Buffer = require(\'buffer\').Buffer; at the beginning of this file' ('Use const { Buffer } = require(\'buffer\'); at the beginning of this file' strictEqual 'Use const Buffer = require(\'buffer\').Buffer; at the beginning of this file')
11:08:23         at assertMessageMatches (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:442:24)
11:08:23         at testInvalidTemplate (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:494:29)
11:08:23         at Function.RuleTester.it (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:581:25)
11:08:23         at Function.itDefaultHandler (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:143:23)
11:08:23         at test.invalid.forEach.invalid (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:580:32)
11:08:23         at Array.forEach (<anonymous>)
11:08:23         at Function.RuleTester.describe (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:579:30)
11:08:23         at Function.describeDefaultHandler (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:160:19)
11:08:23         at Function.RuleTester.describe (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:578:24)
11:08:23         at Function.describeDefaultHandler (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/tools/node_modules/eslint/lib/testers/rule-tester.js:160:19)
11:08:23   ...

Sure seems unrelated. This will need a full CI anyway, so we'll see what happens when a full CI is run. (This was the automatic lite CI.)

@mcollina
Copy link
Member Author

mcollina commented Jul 5, 2018

@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label Jul 5, 2018
Adds require('inspector').console, mapping it to the original
global.console of V8. This enables applications to send messages to
the inspector console programmatically.

Fixes: nodejs#21651
@mcollina
Copy link
Member Author

mcollina commented Jul 5, 2018

@mcollina
Copy link
Member Author

mcollina commented Jul 5, 2018

There are two failures, the first is #21233:

04:46:16 not ok 572 sequential/test-inspector-stop-profile-after-done
04:46:16   ---
04:46:16   duration_ms: 0.739
04:46:16   severity: fail
04:46:16   exitcode: 1
04:46:16   stack: |-
04:46:16     [test] Connecting to a child Node process
04:46:16     [test] Testing /json/list
04:46:16     [err] Debugger listening on ws://127.0.0.1:54651/3215a22e-09d3-4951-bd7c-11a8d2b1955b
04:46:16     [err] For help, see: https://nodejs.org/en/docs/inspector
04:46:16     [err] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [out] {}
04:46:16     [out] 
04:46:16     [err] Debugger attached.
04:46:16     [err] Waiting for the debugger to disconnect...
04:46:16     [err] 
04:46:16     { AssertionError [ERR_ASSERTION]: Input A expected to strictly equal input B:
04:46:16     + expected - actual
04:46:16     
04:46:16     - 3221225477
04:46:16     + 0
04:46:16         at runTests (c:\workspace\node-test-binary-windows\test\sequential\test-inspector-stop-profile-after-done.js:28:10)
04:46:16         at process._tickCallback (internal/process/next_tick.js:68:7)
04:46:16       generatedMessage: true,
04:46:16       name: 'AssertionError [ERR_ASSERTION]',
04:46:16       code: 'ERR_ASSERTION',
04:46:16       actual: 3221225477,
04:46:16       expected: 0,
04:46:16       operator: 'strictEqual' }
04:46:16     1
04:46:16   ...

And the second is definitely unrelated:

04:44:45 not ok 494 parallel/test-worker-relative-path-double-dot
04:44:45   ---
04:44:45   duration_ms: 120.98
04:44:45   severity: fail
04:44:45   exitcode: 1
04:44:45   stack: |-
04:44:45     timeout

@Trott I think this can land.

@Trott
Copy link
Member

Trott commented Jul 5, 2018

Here's a re-run of Windows (the only platform that failed):

https://ci.nodejs.org/job/node-test-commit-windows-fanned/19126/

@mcollina
Copy link
Member Author

mcollina commented Jul 5, 2018

test-https-foafssl.js failed. I've hit resume, but probably I've caused more harm than good.

@mcollina
Copy link
Member Author

mcollina commented Jul 5, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15746/ (maybe with more luck)

@mcollina
Copy link
Member Author

mcollina commented Jul 6, 2018

Landed in 19795d8

@mcollina mcollina closed this Jul 6, 2018
@mcollina mcollina deleted the add-inspector-console branch July 6, 2018 22:05
mcollina added a commit that referenced this pull request Jul 6, 2018
Adds require('inspector').console, mapping it to the original
global.console of V8. This enables applications to send messages to
the inspector console programmatically.

Fixes: #21651

PR-URL: #21659
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Jul 8, 2018
Adds require('inspector').console, mapping it to the original
global.console of V8. This enables applications to send messages to
the inspector console programmatically.

Fixes: #21651

PR-URL: #21659
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mcollina
Copy link
Member Author

mcollina commented Jul 9, 2018

@targos this should land after #21378 or a backport PR being created to work around it.

targos pushed a commit that referenced this pull request Jul 18, 2018
Adds require('inspector').console, mapping it to the original
global.console of V8. This enables applications to send messages to
the inspector console programmatically.

Fixes: #21651

PR-URL: #21659
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
targos added a commit that referenced this pull request Jul 18, 2018
Notable changes:

* console:
  * The `console.timeLog()` method has been implemented.
    (#21312)
* deps:
  * Upgrade to libuv 1.22.0. (#21731)
  * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1).
    (#21728)
* http:
  * Added support for passing both `timeout` and `agent` options to
    `http.request`. (#21204)
* inspector:
  * Expose the original console API in `require('inspector').console`.
    (#21659)
* napi:
  * Added experimental support for functions dealing with bigint numbers.
    (#21226)
* process:
  * The `process.hrtime.bigint()` method has been implemented.
    (#21256)
  * Added the `--title` command line argument to set the process title on
    startup. (#21477)
* trace_events:
  * Added process_name metadata.
    (#21477)
* Added new collaborators
  * codebytere - Shelley Vohr

PR-URL: #21851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants