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: introduce a smoke test for inspector #8429

Closed
wants to merge 1 commit into from
Closed

inspector: introduce a smoke test for inspector #8429

wants to merge 1 commit into from

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Sep 7, 2016

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

@nodejs-github-bot 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 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
@Trott
Copy link
Member

Trott commented Sep 7, 2016

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
Copy link
Member

Trott commented Sep 7, 2016

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
Copy link
Member

Trott commented Sep 7, 2016

@nodejs/testing

@mhdawson
Copy link
Member

mhdawson commented Sep 7, 2016

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

@Trott
Copy link
Member

Trott commented Sep 8, 2016

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. ❤️ )

@@ -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++));
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid C style casts.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 8, 2016

Adressed the comments:

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

@Trott
Copy link
Member

Trott commented Sep 8, 2016

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

Choose a reason for hiding this comment

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

I think ../common should be the first require

@Trott
Copy link
Member

Trott commented Sep 8, 2016

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

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

Choose a reason for hiding this comment

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

s/==/===

Copy link
Contributor

Choose a reason for hiding this comment

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

This nit still needs to be addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 9, 2016

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

[ '--inspect', '--debug-brk', mainScript ]);

const timeoutId = setTimeout(() => {
assert(false, 'Child process did not start properly');
Copy link
Member

@santigimeno santigimeno Sep 9, 2016

Choose a reason for hiding this comment

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

maybe use common.fail here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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

Choose a reason for hiding this comment

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

Unrelated change. Is it to squelch a compiler warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@ofrobots
Copy link
Contributor

Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM w/ nits.

callback);
}

function processIncomingMessage(socket, buffer, handler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

socket is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@eugeneo eugeneo left a comment

Choose a reason for hiding this comment

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

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

callback);
}

function processIncomingMessage(socket, buffer, handler) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 19, 2016

Addressed the nit. Please take another look.

This test executes a simple debug session over the inspector protocol.
@ofrobots
Copy link
Contributor

ofrobots pushed a commit that referenced this pull request Sep 19, 2016
This test executes a simple debug session over the inspector protocol.

PR-URL: #8429
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
@ofrobots
Copy link
Contributor

Thanks. Landed as b4a249f.

@ofrobots ofrobots closed this Sep 19, 2016
@Trott
Copy link
Member

Trott commented Sep 19, 2016

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
Copy link
Member

Trott commented Sep 19, 2016

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 19, 2016

@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
Copy link
Member

Trott commented Sep 19, 2016

@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
Copy link
Contributor Author

eugeneo commented Sep 20, 2016

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
Copy link
Member

Trott commented Sep 20, 2016

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
Copy link
Member

Trott commented Sep 20, 2016

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

@eugeneo eugeneo deleted the inspector_smoke branch September 20, 2016 16:56
@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 20, 2016

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
Copy link
Member

Trott commented Sep 20, 2016

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
Copy link
Member

Trott commented Sep 20, 2016

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

@Trott
Copy link
Member

Trott commented Sep 20, 2016

@eugeneo
Copy link
Contributor Author

eugeneo commented Sep 20, 2016

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
Copy link
Member

Trott commented Sep 20, 2016

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
Copy link
Contributor Author

eugeneo commented Sep 20, 2016

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

@gibfahn
Copy link
Member

gibfahn commented Sep 20, 2016

@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
Copy link
Contributor Author

eugeneo commented Sep 20, 2016

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

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2016
This test executes a simple debug session over the inspector protocol.

PR-URL: nodejs#8429
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
This test executes a simple debug session over the inspector protocol.

PR-URL: #8429
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.