test: rearrange inspector headers into convention #13428

sam-github · 2017-06-03T02:33:34Z

Test guide describes a conventional layout for test headers, review
inspector tests and reorganize to follow the convention.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test,inspector

sam-github · 2017-06-03T02:35:42Z

@cjihrig noticed I wasn't following convention in #13228, because I'd copied test/parallel/test-inspector-invalid-args.js, so I reviewed the inspector tests. Most are OK, except for white space, but I fixed up the one that isn't using the new order convention.

Trott · 2017-06-03T04:16:12Z

test/inspector/test-inspector-port-zero-cluster.js

@@ -17,14 +18,14 @@ if (cluster.isMaster) {
    }));
    worker.send('debugPort');
  }
-  process.on('exit', () => {
+  process.on('exit', common.mustCall(() => {


IIUC, common.mustCall() on an exit handler won't do anything because the check for execution in mustCall() is itself dependent on the exit event. Any situation that would skip calling the function will also skip the check that the function has been called. If the event does not fire, then the check never happens. Unless I'm mistaken about all this, I'd say common.mustCall() should be removed because it provides a false guarantee that the function will be called.

I think this is true iff event handlers are called in the order they were registered. So the the one checking all the mustCalls would run before this one, and IIUC it would fail since this instance didn't run yet.

Anyway test-inspector-port-zero-cluster.js is a PITA, re: #13373

Ohh @sam-github we need you input in #13373

refack

LGTM % the mustCall thing

Trott

Needs one common.mustCall() removed.

cjihrig

No real opinion on the whitespace changes. The process.on('exit', common.mustCall()) change should be backed out I think. +1 to the changes in test/parallel/test-inspector-invalid-args.js

refack · 2017-06-03T16:45:08Z

CI: https://ci.nodejs.org/job/node-test-commit/10341/
(wanted to find out if my assumption was correct)

refack · 2017-06-03T17:00:14Z

Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:483:10)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/inspector/test-inspector-port-zero-cluster.js:21:29)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)
Debugger listening on ws://127.0.0.1:38692/dc906197-d686-4160-9dba-56213523da01
For help see https://nodejs.org/en/docs/inspector
Debugger listening on ws://127.0.0.1:38693/1e3ceb9e-7e0e-4221-8b53-a72fc3c7f434
For help see https://nodejs.org/en/docs/inspector
Debugger listening on ws://127.0.0.1:38694/0daabd15-183e-4f46-bd24-7076eaefdd9f
For help see https://nodejs.org/en/docs/inspector
Debugger listening on ws://127.0.0.1:38695/8d609731-d5a7-4859-b665-7138b64d88df
For help see https://nodejs.org/en/docs/inspector

sam-github · 2017-06-05T16:59:16Z

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

@Trott removed the common.mustCall, PTAL: da1602e

mustCall() removed, clearing my request for changes

Trott · 2017-06-05T18:35:20Z

This is now a whitespace-only change.

Do we truly want to enforce blank lines after common is loaded and all that stuff?

If so, we should add a lint rule (in a separate PR--not asking for a change to this one).

If not, we probably don't need to land this.

sam-github · 2017-06-05T18:38:54Z

test/parallel/test-inspector-invalid-args.js

@@ -1,11 +1,12 @@
 'use strict';
+const common = require('../common');


@Trott this is not whitespace only. I can remove all the other whitespace only changes if you'd like, I think they are more readable, and I think they are implied by the test guide, but this change, specifically, brings this test into conformance with the guide.

Oh, yes, missed that one, sorry. Anyway, yeah, it's all fine. No objections.

Test guide describes a conventional layout for test headers, review inspector tests and reorganize to follow the convention. PR-URL: nodejs#13428 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Test guide describes a conventional layout for test headers, review inspector tests and reorganize to follow the convention. PR-URL: #13428 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Jun 3, 2017

Trott reviewed Jun 3, 2017

View reviewed changes

refack approved these changes Jun 3, 2017

View reviewed changes

Trott previously requested changes Jun 3, 2017

View reviewed changes

cjihrig approved these changes Jun 3, 2017

View reviewed changes

gibfahn approved these changes Jun 3, 2017

View reviewed changes

lpinca approved these changes Jun 4, 2017

View reviewed changes

jasnell approved these changes Jun 4, 2017

View reviewed changes

sam-github commented Jun 5, 2017

View reviewed changes

sam-github force-pushed the inspector-test-cleanup branch from da1602e to aae0d45 Compare June 5, 2017 19:29

sam-github merged commit aae0d45 into nodejs:master Jun 5, 2017

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

sam-github deleted the inspector-test-cleanup branch June 14, 2017 20:33

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: rearrange inspector headers into convention #13428

test: rearrange inspector headers into convention #13428

sam-github commented Jun 3, 2017

sam-github commented Jun 3, 2017

Trott Jun 3, 2017

refack Jun 3, 2017

refack Jun 3, 2017

refack left a comment

Trott left a comment

cjihrig left a comment

refack commented Jun 3, 2017

refack commented Jun 3, 2017

sam-github commented Jun 5, 2017 •

edited

Loading

Trott commented Jun 5, 2017

sam-github Jun 5, 2017

Trott Jun 5, 2017

		@@ -1,11 +1,12 @@
		'use strict';
		const common = require('../common');

test: rearrange inspector headers into convention #13428

test: rearrange inspector headers into convention #13428

Conversation

sam-github commented Jun 3, 2017

Checklist

Affected core subsystem(s)

sam-github commented Jun 3, 2017

Trott Jun 3, 2017

Choose a reason for hiding this comment

refack Jun 3, 2017

Choose a reason for hiding this comment

refack Jun 3, 2017

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

refack commented Jun 3, 2017

refack commented Jun 3, 2017

sam-github commented Jun 5, 2017 • edited Loading

Trott commented Jun 5, 2017

sam-github Jun 5, 2017

Choose a reason for hiding this comment

Trott Jun 5, 2017

Choose a reason for hiding this comment

sam-github commented Jun 5, 2017 •

edited

Loading