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: fix debug-port-cluster flakiness #4310

Merged
merged 1 commit into from
Dec 16, 2015

Conversation

bnoordhuis
Copy link
Member

Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

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

Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

PR-URL: nodejs#4310
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis bnoordhuis added debugger test Issues and PRs related to the tests. labels Dec 16, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Dec 16, 2015

I'm going to close #4258 in favor of this one. This approach seems less prone to flakiness.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 16, 2015

LGTM

@bnoordhuis bnoordhuis force-pushed the fix-debug-port-cluster branch from 6757182 to 2859f9e Compare December 16, 2015 16:29
@bnoordhuis bnoordhuis closed this Dec 16, 2015
@bnoordhuis bnoordhuis deleted the fix-debug-port-cluster branch December 16, 2015 16:29
@bnoordhuis bnoordhuis merged commit 2859f9e into nodejs:master Dec 16, 2015

var args = [
'--debug=' + port,
'--debug=' + PORT_MIN,
Copy link
Member

Choose a reason for hiding this comment

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

very minor nit... since we're starting to use template strings more frequently... perhaps:

`--debug=${PORT_MIN}`

Copy link
Member Author

Choose a reason for hiding this comment

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

I had that in my first (unpushed) revision but then I thought "meh, change for the sake of it and incongruous with the line below" so I left it out.

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

LGTM

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

ha! ok, nevermind, just noticed that it landed lol

bnoordhuis added a commit that referenced this pull request Dec 16, 2015
Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

PR-URL: #4310
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rvagg rvagg mentioned this pull request Dec 17, 2015
bnoordhuis added a commit that referenced this pull request Dec 30, 2015
Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

PR-URL: #4310
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

PR-URL: #4310
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Rewrite the test so that stderr reordering of the child processes won't
confuse the test's expectations.

PR-URL: nodejs#4310
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants