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.

Sign up for GitHub

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

Jump to bottom

test: fix and improve debugger-client test #10371

Closed
wants to merge 1 commit into from
Closed

test: fix and improve debugger-client test #10371

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Dec 21, 2016 •
edited
Loading

Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

test debugger

Description of change

This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
src/node.js, but that has been renamed to lib/internal/node.js and
then to lib/internal/bootstrap_node.js. So, the script name has been
updated.

Apart from that, using const instead of var wherever possible.

Refer: #10361

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 21, 2016
@thefourtheye thefourtheye force-pushed the fix-test-debugger-client branch from a9c33e4 to e8995ce Compare December 21, 2016 02:19
@thefourtheye thefourtheye mentioned this pull request Dec 21, 2016
Closed
@Trott
Copy link
Member

Trott commented Dec 21, 2016

You would probably do @thealphanerd a favor by figuring out how to backport sooner rather than later. I'm thinking specifically about the file that was renamed twice. (Is it one of the old names in v4 or v6? If so, then this won't work and we'll need a different PR for those)

Trott
Trott reviewed Dec 21, 2016
View reviewed changes
test/debugger/test-debugger-client.js
@@ -158,7 +158,7 @@ function doTest(cb, done) {
console.error('got stderr data %j', data);
nodeProcess.stderr.resume();
b += data;
if (didTryConnect === false && b.match(/Debugger listening on port/)) {
if (didTryConnect === false && b.match(/Debugger listening on/)) {
Copy link
Member

@Trott Trott Dec 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: Maybe leave a space at the end of the regexp? /Debugger listening on /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Trott
Trott reviewed Dec 21, 2016
View reviewed changes
test/debugger/test-debugger-client.js
@@ -168,7 +168,7 @@ function doTest(cb, done) {

function tryConnect() {
// Wait for some data before trying to connect
var c = new debug.Client();
const c = new debug.Client();
console.error('>>> connecting...');
c.connect(debug.port);
c.on('break', function(brk) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While you're in here, maybe remove brk as a callback argument. It 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.

Done!

Trott
Trott approved these changes Dec 21, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. Left a couple of nits. Also, two other callback arguments appear to be unused, if you want to clean those up too: res on line 19 and c on line 140.

@thefourtheye
test: fix and improve debugger-client test
2df054b 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: nodejs#10361
@thefourtheye thefourtheye force-pushed the fix-test-debugger-client branch from e8995ce to 2df054b Compare December 21, 2016 06:20
@thefourtheye
Copy link
Contributor Author

Thanks @Trott. Addressed all the nits.

@jasnell
Copy link
Member

jasnell commented Dec 24, 2016

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

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Dec 26, 2016
@thefourtheye
test: fix and improve debugger-client test
3ef4ec0 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: nodejs#10361

PR-URL: nodejs#10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@thefourtheye thefourtheye deleted the fix-test-debugger-client branch December 26, 2016 07:18
@thefourtheye
Copy link
Contributor Author

Landed in 3ef4ec0

@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@thefourtheye @evanlucas
test: fix and improve debugger-client test
ddaa2a7 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: #10361

PR-URL: #10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@thefourtheye @evanlucas
test: fix and improve debugger-client test
94a894a 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: #10361

PR-URL: #10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Added dont-land label. Please feel free to manually backport

thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Feb 5, 2017
@thefourtheye
test: fix and improve debugger-client test
8f61ca9 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: nodejs#10361

PR-URL: nodejs#10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@thefourtheye thefourtheye mentioned this pull request Feb 5, 2017
Closed
2 tasks
MylesBorins pushed a commit that referenced this pull request Mar 8, 2017
@thefourtheye @MylesBorins
test: fix and improve debugger-client test
25963ec 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: #10361

PR-URL: #10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@thefourtheye @MylesBorins
test: fix and improve debugger-client test
34e0bc6 
This test expects the string 'Debugger listening on port' on stderr and
since the message has been changed to 'Debugger listening on host:port'
this was failing always.

Apart from that, this test expects the main script's name to be
`src/node.js`, but that has been renamed to `lib/internal/node.js` and
then to `lib/internal/bootstrap_node.js`. So, the script name has been
updated.

Apart from that, using `const` instead of `var` wherever possible.

Refer: #10361

PR-URL: #10371

Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github approved these changes

@MylesBorins MylesBorins Awaiting requested review from MylesBorins

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@thefourtheye @Trott @jasnell @MylesBorins @sam-github @cjihrig @mscdex @nodejs-github-bot