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

test: allow inspector to reopen with same port #14320

Merged
merged 1 commit into from
Aug 12, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jul 17, 2017

Test checks that if you open the inspector with '0' (pick a random free
port), close it, then reopen it, you get a different port. However this
isn't necessarily true.

Fixes: #14316

Refs: #14316 (comment)

So this assertion checks that when you close and reopen you don't get the same port. However AIUI node just picks a free port, so there is a (very small) chance you'll get the same port.

cc/ @sam-github as you wrote the original test.

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

test, inspector

@gibfahn gibfahn requested a review from sam-github July 17, 2017 07:02
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 17, 2017
@gibfahn gibfahn added inspector Issues and PRs related to the V8 inspector protocol flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Jul 17, 2017
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I wonder why the assertion was there before. LGTM assuming that the inspector correctly closes the socket.

@gibfahn
Copy link
Member Author

gibfahn commented Jul 18, 2017

LGTM assuming that the inspector correctly closes the socket.

That's tested above by closeWhenOpen()

@gibfahn
Copy link
Member Author

gibfahn commented Aug 12, 2017

CI: https://ci.nodejs.org/job/node-test-commit/11724/

CI was green except for macOS, which failed with:

not ok 827 parallel/test-cluster-send-handle-large-payload
  ---
  duration_ms: 120.14
  severity: fail
  stack: |-
    timeout
  ...

which is #14747

Test checks that if you open the inspector with '0' (pick a random free
port), close it, then reopen it, you get a different port. However this
isn't necessarily true.

PR-URL: nodejs#14320
Fixes: nodejs#14316
Refs: nodejs#14316 (comment)
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn gibfahn merged commit 8780e1d into nodejs:master Aug 12, 2017
@gibfahn gibfahn deleted the flake-inspector-open branch August 12, 2017 18:55
addaleax pushed a commit that referenced this pull request Aug 12, 2017
Test checks that if you open the inspector with '0' (pick a random free
port), close it, then reopen it, you get a different port. However this
isn't necessarily true.

PR-URL: #14320
Fixes: #14316
Refs: #14316 (comment)
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@gibfahn
Copy link
Member Author

gibfahn commented Aug 12, 2017

Adding dont-land labels as the test doesn't exist on v6.x.

@addaleax addaleax mentioned this pull request Aug 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. 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.

Investigate flaky test-inspector-open on Linux
7 participants