inspector: introduce a smoke test for inspector #8429

eugeneo · 2016-09-07T01:14:31Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

Adds a new test case that checks the inspector

Description of change

This test executes a simple debug session over the inspector protocol.

CC: @ofrobots

Trott · 2016-09-07T02:15:49Z

Assuming the test is expected to work on Windows, I imagine there should probably be a test-inspector task added to vcbuild.bat too.

Trott · 2016-09-07T02:20:18Z

test-inspector takes only a second or so to run and doesn't seem to reach out to the internet or anything like that. Any reason not to add the inspector dir to the test and test-ci tasks?

Trott · 2016-09-07T02:20:39Z

@nodejs/testing

mhdawson · 2016-09-07T18:59:59Z

It would be good to have the added, provided it passes across the platforms

Trott · 2016-09-08T02:30:01Z

Nit: add an entry for the inspector directory to the test/README.md file.

(We didn't do that for pseudo-tty. If @Fishrock123 doesn't want to do that that would be a good first contribution for anyone following along. ❤️ )

ofrobots · 2016-09-08T17:23:46Z

src/inspector_socket.cc

@@ -33,7 +33,7 @@ static void dump_hex(const char* buf, size_t len) {
  while (ptr < end) {
    cptr = ptr;
    for (i = 0; i < 16 && ptr < end; i++) {
-      printf("%2.2X  ", *(ptr++));
+      printf("%2.2X  ", (unsigned char) *(ptr++));


Avoid C style casts.

eugeneo · 2016-09-08T21:02:29Z

Adressed the comments:

Added tests to test and test-ci
Updated vcbuild.bat
Added an entry to README.md
Replaced the cast.

Trott · 2016-09-08T21:07:35Z

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

santigimeno · 2016-09-08T21:14:41Z

test/inspector/test-inspector.js

@@ -0,0 +1,167 @@
+'use strict';
+const assert = require('assert');
+require('../common');


I think ../common should be the first require

Trott · 2016-09-08T21:17:43Z

Looks like this is coming up from time to time on Linux variants:

not ok 1166 inspector/test-inspector
# AssertionError: Timed out waiting for Waiting for the debugger to disconnect...
#     at Timeout.setTimeout (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos6-64/test/inspector/inspector-helper.js:235:9)
#     at tryOnTimeout (timers.js:232:11)
#     at Timer.listOnTimeout (timers.js:202:5)
# Verifying debugger stops on start (--debug-brk option)
# [err] Debugger attached.
# 
# Setting a breakpoint and verifying it is hit
# [out] A message 5
# 
# Verify we can read current application state
# Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# 
# [err] Waiting for the debugger to disconnect...
# 
  ---
  duration_ms: 15.543

santigimeno · 2016-09-08T21:18:53Z

test/fixtures/loop.js

+var k = 1;
+console.log('A message', 5);
+while (t > 0) {
+  if (t++ == 1000) {


This nit still needs to be addressed.

Thanks for pointing out - I haven't noticed that comment. Fixed.

eugeneo · 2016-09-09T18:02:35Z

Thanks for all the comments. Updated the CL:

Addressed a race condition when "Waiting for the debugger to disconnect" arrives before the test set the watcher.
s/==/===/ and s/!=/!==/
Removed brackets around single-line statements (retained the brackets if another branch of the same if statement has it.
Did other changes suggested by @cjihrig

santigimeno · 2016-09-09T19:31:45Z

test/inspector/inspector-helper.js

+      [ '--inspect', '--debug-brk', mainScript ]);
+
+  const timeoutId = setTimeout(() => {
+    assert(false, 'Child process did not start properly');


maybe use common.fail here

bnoordhuis · 2016-09-14T10:34:18Z

src/inspector_agent.cc

@@ -620,6 +622,7 @@ bool AgentImpl::OnInspectorHandshakeIO(inspector_socket_t* socket,
    return false;
  default:
    UNREACHABLE();
+    return false;


Unrelated change. Is it to squelch a compiler warning?

Seems to be a VC++ warning. I fixed it as I was testing on Windows.

ofrobots · 2016-09-14T17:27:13Z

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

ofrobots

LGTM w/ nits.

ofrobots · 2016-09-15T23:34:07Z

test/inspector/inspector-helper.js

+      callback);
+}
+
+function processIncomingMessage(socket, buffer, handler) {


socket is unused?

Thanks. Fixed.

Did you fix this? Either the new github review mechanism is lying to me, or socket is still a parameter here.

Not sure what I did wrong - but I reuploaded the fix.

eugeneo

Thank you for the review. I addressed the comments and uploaded a new version.

eugeneo · 2016-09-16T18:54:15Z

test/inspector/inspector-helper.js

+      callback);
+}
+
+function processIncomingMessage(socket, buffer, handler) {


Thanks. Fixed.

eugeneo · 2016-09-19T17:17:23Z

Addressed the nit. Please take another look.

This test executes a simple debug session over the inspector protocol.

ofrobots · 2016-09-19T17:32:47Z

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

This test executes a simple debug session over the inspector protocol. PR-URL: #8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

ofrobots · 2016-09-19T20:13:42Z

Thanks. Landed as b4a249f.

Trott · 2016-09-19T23:35:55Z

Just got a failure for this test on CI: https://ci.nodejs.org/job/node-test-commit-linux/5205/nodes=centos5-32/tapTestReport/test.tap-1174/

not ok 1174 inspector/test-inspector
# AssertionError: Have not terminated
# at Object.exports.fail (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/common.js:442:10)
# at Timeout.setTimeout (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/inspector/inspector-helper.js:102:34)
# at tryOnTimeout (timers.js:232:11)
# at Timer.listOnTimeout (timers.js:202:5)
# [err] Debugger listening on port 9229.
# [err] Warning: This is an experimental feature and could change at any time.
# [err] To start debugging, open the following URL in Chrome:
# [err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
# [err] 
# [test] Verifying debugger stops on start (--debug-brk option)
# [err] Debugger attached.
# [err] 
# [test] Setting a breakpoint and verifying it is hit
# [out] A message 5
# [out] 
# [test] Verify we can read current application state
# [test] Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# [out] 
# [err] Waiting for the debugger to disconnect...
# [err]

Trott · 2016-09-19T23:36:52Z

Stress test: https://ci.nodejs.org/job/node-stress-single-test/921/nodes=centos5-32/console

eugeneo · 2016-09-19T23:41:41Z

@Trott How do I run this test locally? Looking at the output, it could be that this is simply a timeout while the node is shutting down. Are there any standards for how long the test can wait on failure?

Trott · 2016-09-19T23:52:27Z

@eugeneo python tools/test.py -p tap --mode=release inspector/test-inspector

On the stress test, it looks like it's failing about one out of every 50 runs on CentOS 5 (32-bit). So you'll probably want to put that in a loop or something.

eugeneo · 2016-09-20T00:15:02Z

It is not failing on my Ubuntu 64 workstation. Can you try this commit - https://github.com/eugeneo/node/commit/542c02348a5f2236f0c98c719067790a0a0d6332
on that environment? I added some more debug output and renamed an event, might've been the reason for the falure.

Trott · 2016-09-20T05:34:32Z

Here you go. This time, the test timed out after 8 successful runs.
¯_(ツ)_/¯
https://ci.nodejs.org/job/node-stress-single-test/922/nodes=centos5-32/console

not ok 1 inspector/test-inspector
# TIMEOUT
# [err] Debugger listening on port 9229.
# [err] Warning: This is an experimental feature and could change at any time.
# [err] To start debugging, open the following URL in Chrome:
# [err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
# [err] 
# [test] Verifying debugger stops on start (--debug-brk option)
# [err] Debugger attached.
# [err] 
# [test] Setting a breakpoint and verifying it is hit
# [out] A message 5
# [out] 
# [test] Verify we can read current application state
# [test] Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# [out] 
# [err] Waiting for the debugger to disconnect...
# [err] 
# [test] Connection terminated

Trott · 2016-09-20T05:57:45Z

I should mention that the timeout is enforced by test.py and is 60 seconds.

eugeneo · 2016-09-20T17:03:38Z

Thank you for looking into this.

I still have no idea where this failure is coming from... What I see in the trace is that the test does not get notification when the child process exits. I do not know if the child process exits. I uploaded a new commit - https://github.com/eugeneo/node/commit/972120d1dcd1d6c0913c56a98f7048b7d3cb8466 - that should indicate if the child exits. Can we try it on CentOS?

Note that this commit adds some tracing and thus breaks some tests other then test-inspector.

Is there a way for me to access one of the machines where the failure occurs?

Trott · 2016-09-20T17:41:05Z

Is there a way for me to access one of the machines where the failure occurs?

Not sure what the process is, but that would be something handled by the Build working group. @nodejs/build

Trott · 2016-09-20T17:44:08Z

I queued up the test in Jenkins but it's waiting on a host to become available, I think.... Coming soon, though...

Trott · 2016-09-20T17:49:28Z

Test is running... https://ci.nodejs.org/job/node-stress-single-test/925/nodes=centos5-32/console

eugeneo · 2016-09-20T18:01:13Z

Thank you! This was most helpful - looks like the child node does not actually exit, which is a bug. I'm looking into it.

Trott · 2016-09-20T18:03:26Z

Because the Jenkins results get cleared out from time to time, here's the result, in case someone needs to refer back to it later:

# TIMEOUT
# [err] Debugger listening on port 9229.
# [err] Warning: This is an experimental feature and could change at any time.
# [err] To start debugging, open the following URL in Chrome:
# [err]     chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
# [err] 
# [test] Verifying debugger stops on start (--debug-brk option)
# [err] Debugger attached.
# [err] 
# [test] Setting a breakpoint and verifying it is hit
# [out] A message 5
# [out] 
# [test] Verify we can read current application state
# [test] Verify node waits for the frontend to disconnect
# [out] Outputed message #1
# [out] 
# [err] Waiting for the debugger to disconnect...
# [err] 
# [test] Connection terminated
# [err] [../src/inspector_agent.cc:781] Frontend's gone, can exit
# [err] 
# [err] [../src/inspector_agent.cc:790] Frontend's disconnect handled
# [err]

eugeneo · 2016-09-20T18:59:29Z

#8669 was created for this issue, I will be posting patches there.

gibfahn · 2016-09-20T22:18:00Z

@Trott @eugeneo FYI, to repeat tests with the test runner you can just do:

python tools/test.py -p tap --repeat 100 --mode=release inspector/test-inspector

eugeneo · 2016-09-20T22:28:35Z

@gibfahn I cannot reproduce it outside of the CI - I just ran it again on my Ubuntu 64 system...

This test executes a simple debug session over the inspector protocol. PR-URL: nodejs#8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

This test executes a simple debug session over the inspector protocol. PR-URL: #8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. 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 Sep 7, 2016

mscdex added test Issues and PRs related to the tests. and removed build Issues and PRs related to build files or the CI. labels Sep 7, 2016

ofrobots reviewed Sep 8, 2016
View reviewed changes

santigimeno reviewed Sep 8, 2016
View reviewed changes

ofrobots mentioned this pull request Sep 8, 2016

Port the CLI debugger to use the v8 inspector protocol #7266

Closed

santigimeno reviewed Sep 8, 2016
View reviewed changes

santigimeno reviewed Sep 9, 2016
View reviewed changes

bnoordhuis reviewed Sep 14, 2016
View reviewed changes

ofrobots approved these changes Sep 16, 2016

View reviewed changes

eugeneo commented Sep 16, 2016

View reviewed changes

inspector: introduce a smoke test

a876c38

This test executes a simple debug session over the inspector protocol.

ofrobots pushed a commit that referenced this pull request Sep 19, 2016

inspector: introduce a smoke test

b4a249f

This test executes a simple debug session over the inspector protocol. PR-URL: #8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

ofrobots closed this Sep 19, 2016

eugeneo deleted the inspector_smoke branch September 20, 2016 16:56

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016

inspector: introduce a smoke test

c36b33c

This test executes a simple debug session over the inspector protocol. PR-URL: nodejs#8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016

inspector: introduce a smoke test

0b90ff7

This test executes a simple debug session over the inspector protocol. PR-URL: #8429 Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins added the dont-land-on-v4.x label Sep 30, 2016

inspector: introduce a smoke test for inspector #8429

inspector: introduce a smoke test for inspector #8429

Conversation

eugeneo commented Sep 7, 2016

Checklist

Affected core subsystem(s)

Description of change

Trott commented Sep 7, 2016

Trott commented Sep 7, 2016

Trott commented Sep 7, 2016

mhdawson commented Sep 7, 2016

Trott commented Sep 8, 2016

Choose a reason for hiding this comment

eugeneo commented Sep 8, 2016

Trott commented Sep 8, 2016

Choose a reason for hiding this comment

Trott commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Sep 9, 2016

santigimeno Sep 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofrobots commented Sep 14, 2016

ofrobots left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugeneo commented Sep 19, 2016

ofrobots commented Sep 19, 2016

ofrobots commented Sep 19, 2016

Trott commented Sep 19, 2016

Trott commented Sep 19, 2016

eugeneo commented Sep 19, 2016

Trott commented Sep 19, 2016

eugeneo commented Sep 20, 2016 • edited Loading

Trott commented Sep 20, 2016

Trott commented Sep 20, 2016

eugeneo commented Sep 20, 2016 • edited Loading

Trott commented Sep 20, 2016

Trott commented Sep 20, 2016

Trott commented Sep 20, 2016

eugeneo commented Sep 20, 2016

Trott commented Sep 20, 2016

eugeneo commented Sep 20, 2016

gibfahn commented Sep 20, 2016

eugeneo commented Sep 20, 2016

santigimeno Sep 9, 2016 •

edited

Loading

eugeneo commented Sep 20, 2016 •

edited

Loading

eugeneo commented Sep 20, 2016 •

edited

Loading