test: improve test-cluster-net-send.js #9570

akito0107 · 2016-11-12T07:47:27Z

Checklist

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

Affected core subsystem(s)

test, cluster

Description of change

This commit improves test-cluster-net-send as below;

use port 0 instead of common.PORT
use common.mustCall
use assert.strictEqual instead of assert.equal
change var to const or let (if possible)
change function to arrow function

This is a part of Code And Learn at NodeFest 2016 Challenge
nodejs/code-and-learn#58

shigeki

CI submitted by @mscdex was running on https://ci.nodejs.org/job/node-test-pull-request/4828/ . Two tests were failed, one is due to missing g++ in CentOS7-64 build env and others Jenkins error on armv8. Others are fine. LGTM.

cjihrig · 2016-11-12T13:46:43Z

test/parallel/test-cluster-net-send.js

-  server.listen(common.PORT, function() {
-    socket = net.connect(common.PORT, '127.0.0.1', socketConnected);
+  server.listen(0, function() {
+    socket = net.connect(server.address().port, '127.0.0.1', socketConnected);


Instead of 127.0.0.1, couldn't you use server.address().address?

cjihrig

LGTM, but it looks like there are a few other improvements that could be made to this test (var -> const, common.mustCall(), assert.strictEqual(), etc.).

akito0107 · 2016-11-14T11:49:39Z

@cjihrig
Thank you for your advice ! I fixed it !

santigimeno · 2016-11-14T11:58:39Z

test/parallel/test-cluster-net-send.js

-      assert.equal(data.toString(), 'hello');
-    });
+      assert.strictEqual(data.toString(), 'hello');
+    }));


Now that you have added the common.mustCall wrapper I think you can remove the called variable.
Also, though highly unlikely, the data listener could be called more than once, so I would check for the data validity in the end listener.

Yes, drop the called variable and the process.once('exit', ...), move the common.mustCall() from the data handler to the end handler, and do a concatenation in the data handler.

cjihrig · 2016-11-14T13:49:58Z

test/parallel/test-cluster-net-send.js

  });

-  process.on('disconnect', function() {
+  process.on('disconnect', () => {
    process.exit();
    server.close();


Isn't this unreachable code?

It would be better to have the process exit naturally, rather than call process.exit().

cjihrig · 2016-11-14T13:51:23Z

test/parallel/test-cluster-net-send.js

-  var server = net.createServer(function(c) {
-    process.once('message', function(msg) {
-      assert.equal(msg, 'got');
+  const server = net.createServer((c) => {


Can you add common.mustCall() here too. Otherwise, there is no guarantee that the process.once('message', ...) handler is added.

jasnell · 2016-11-18T15:39:29Z

test/parallel/test-cluster-net-send.js


-    handle.on('end', function() {
+    handle.on('end', () => {


common.mustCall() here as well.

akito0107 · 2016-11-19T08:39:07Z

@cjihrig @santigimeno @jasnell
Thank you all! I fixed it.

shigeki · 2016-11-19T11:40:33Z

CI is running again on https://ci.nodejs.org/job/node-test-pull-request/4909/ .

targos · 2016-11-27T16:59:33Z

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

targos · 2016-11-27T19:21:39Z

The test fails on Windows: https://ci.nodejs.org/job/node-test-binary-windows/4954/RUN_SUBSET=3,VS_VERSION=vs2015,label=win2008r2/tapTestReport/

Example output on my PC:

[15828] master
[15660] worker
events.js:160
      throw er; // Unhandled 'error' event
      ^

Error: connect EADDRNOTAVAIL :::62283
    at Object.exports._errnoException (util.js:1022:11)
    at exports._exceptionWithHostPort (util.js:1045:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1090:14)

jasnell · 2017-01-11T00:24:25Z

@akito0107 can you please take a look at the windows test failure?

This commit improves test-cluster-net-send as below - change function to arrow function - ensure data event is emitted exactly once

akito0107 · 2017-02-03T05:33:23Z

@jasnell
Sorry for late.

This error might be caused by using sever.listen(0), so, I restored sever.listen(0) to server.listen(common.PORT).
I would like to check Windows CI test is passed.

gibfahn · 2017-02-03T14:53:37Z

test/parallel/test-cluster-net-send.js

-  server.listen(common.PORT, function() {
-    socket = net.connect(common.PORT, '127.0.0.1', socketConnected);
+  server.listen(common.PORT, () => {
+    socket = net.connect(server.address().port, server.address().address,


I think this is the same issue as the one I saw in #10854

If so, the problem is that you can't use server.address().address here. You should be listening on 0 (i.e. all IPv6 and IPv4 addresses), in which case server.address().address resolves to ::. On linux this sometimes resolves to 127.0.0.1 if there is no IPv6 support on the machine, but this isn't the case on all platforms.

Changing to:

- server.listen(common.PORT, () => { - socket = net.connect(server.address().port, server.address().address, + server.listen(0, () => { + socket = net.connect(server.address().port, 'localhost',

should fix the issue on all platforms. localhost should always resolve to either 127.0.0.1 or :: (either of which is fine).

cc/ @cjihrig

jasnell · 2017-03-24T22:51:59Z

Ping... status update on this one?

fhinkel · 2017-05-26T19:28:04Z

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 12, 2016

mscdex added cluster Issues and PRs related to the cluster subsystem. net Issues and PRs related to the net subsystem. labels Nov 12, 2016

shigeki approved these changes Nov 12, 2016

View reviewed changes

cjihrig reviewed Nov 12, 2016

View reviewed changes

cjihrig approved these changes Nov 12, 2016

View reviewed changes

akito0107 force-pushed the improve-test-cluster-net-send branch 2 times, most recently from 9829c3c to 27ad46b Compare November 14, 2016 11:46

santigimeno reviewed Nov 14, 2016

View reviewed changes

cjihrig reviewed Nov 14, 2016

View reviewed changes

jasnell reviewed Nov 18, 2016

View reviewed changes

akito0107 force-pushed the improve-test-cluster-net-send branch from 27ad46b to d7f29a0 Compare November 19, 2016 08:36

cjihrig approved these changes Nov 19, 2016

View reviewed changes

targos approved these changes Nov 27, 2016

View reviewed changes

akito0107 force-pushed the improve-test-cluster-net-send branch 2 times, most recently from 3927897 to 644967c Compare February 3, 2017 04:55

test: improve test-cluster-net-send.js

fbd8c9f

This commit improves test-cluster-net-send as below - change function to arrow function - ensure data event is emitted exactly once

akito0107 force-pushed the improve-test-cluster-net-send branch from 644967c to fbd8c9f Compare February 3, 2017 05:00

gibfahn reviewed Feb 3, 2017

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel closed this May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: improve test-cluster-net-send.js #9570

test: improve test-cluster-net-send.js #9570

akito0107 commented Nov 12, 2016 •

edited

Loading

shigeki left a comment

cjihrig Nov 12, 2016

cjihrig left a comment

akito0107 commented Nov 14, 2016

santigimeno Nov 14, 2016

cjihrig Nov 14, 2016

cjihrig Nov 14, 2016

cjihrig Nov 14, 2016

jasnell Nov 18, 2016

akito0107 commented Nov 19, 2016

shigeki commented Nov 19, 2016

targos commented Nov 27, 2016

targos commented Nov 27, 2016

jasnell commented Jan 11, 2017

akito0107 commented Feb 3, 2017 •

edited

Loading

gibfahn Feb 3, 2017 •

edited

Loading

jasnell commented Mar 24, 2017

fhinkel commented May 26, 2017

test: improve test-cluster-net-send.js #9570

test: improve test-cluster-net-send.js #9570

Conversation

akito0107 commented Nov 12, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

shigeki left a comment

Choose a reason for hiding this comment

cjihrig Nov 12, 2016

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

akito0107 commented Nov 14, 2016

santigimeno Nov 14, 2016

Choose a reason for hiding this comment

cjihrig Nov 14, 2016

Choose a reason for hiding this comment

cjihrig Nov 14, 2016

Choose a reason for hiding this comment

cjihrig Nov 14, 2016

Choose a reason for hiding this comment

jasnell Nov 18, 2016

Choose a reason for hiding this comment

akito0107 commented Nov 19, 2016

shigeki commented Nov 19, 2016

targos commented Nov 27, 2016

targos commented Nov 27, 2016

jasnell commented Jan 11, 2017

akito0107 commented Feb 3, 2017 • edited Loading

gibfahn Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Mar 24, 2017

fhinkel commented May 26, 2017

akito0107 commented Nov 12, 2016 •

edited

Loading

akito0107 commented Feb 3, 2017 •

edited

Loading

gibfahn Feb 3, 2017 •

edited

Loading