test: improve test-net-server-address.js #8586

akito0107 · 2016-09-17T09:12:45Z

Checklist

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

Affected core subsystem(s)

test

Description of change

Refactored test as a below;

'var' to 'const'
function to arrow function

yosuke-furukawa · 2016-09-17T09:39:27Z

LGTM

cjihrig · 2016-09-17T13:28:50Z

Generally, LGTM, but we might be able to improve this further using common.mustCall() and common.fail() in a few places.

akito0107 · 2016-09-17T14:40:44Z

@cjihrig Thank you for your advice, I fixed!

targos

LGTM with a suggestion

targos · 2016-09-17T14:48:55Z

test/sequential/test-net-server-address.js

-server_ipv4.on('error', function(e) {
-  console.log('Error on ipv4 socket: ' + e.toString());
-});
+server_ipv4.on('error', common.fail);


maybe assert.ifError instead ?

Agreed. Using assert.ifError will print a bit more information than common.fail.

targos · 2016-09-17T14:49:15Z

test/sequential/test-net-server-address.js

-server_ipv6.on('error', function(e) {
-  console.log('Error on ipv6 socket: ' + e.toString());
-});
+server_ipv6.on('error', common.fail);


ditto and others below

cjihrig · 2016-09-17T13:25:55Z

test/sequential/test-net-server-address.js

  console.log('Error on ipv4 socket: ' + e.toString());
 });

-server_ipv4.listen(common.PORT, common.localhostIPv4, function() {
-  var address_ipv4 = server_ipv4.address();
+server_ipv4.listen(common.PORT, common.localhostIPv4, () => {


Can you add a common.mustCall() here?

cjihrig · 2016-09-17T13:27:04Z

test/sequential/test-net-server-address.js


-server_ipv6.on('error', function(e) {
+server_ipv6.on('error', (e) => {


I know you aren't the original author of this test, but is it expected that this will never be called? If so, this callback could be replaced with common.fail.

cjihrig

LGTM if the CI is happy.

cjihrig · 2016-09-17T16:59:10Z

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

imyller

LGTM

imyller · 2016-09-19T21:07:55Z

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

imyller · 2016-09-19T21:08:44Z

If CI is green, I'll start the process of landing this.

imyller · 2016-09-19T21:25:13Z

We might have a problem here.

CI failed on multiple platforms with this refactored test:

not ok 1226 sequential/test-net-server-address
# 
# assert.js:85
#   throw new assert.AssertionError({
#   ^
# AssertionError: Error: listen EADDRINUSE 127.0.0.1:12346
#     at Server.exports.fail (/home/iojs/build/workspace/node-test-commit-linux-fips/nodes/ubuntu1404-64/test/common.js:438:10)
#     at emitOne (events.js:96:13)
#     at Server.emit (events.js:188:7)
#     at emitErrorNT (net.js:1274:8)
#     at _combinedTickCallback (internal/process/next_tick.js:74:11)
#     at process._tickCallback (internal/process/next_tick.js:98:9)
#     at Module.runMain (module.js:596:11)
#     at run (bootstrap_node.js:382:7)
#     at startup (bootstrap_node.js:137:9)
#     at bootstrap_node.js:497:3
  ---
  duration_ms: 0.113

-1 for landing this until CI failure is resolved.

/cc @akito0107

imyller · 2016-09-20T10:22:55Z

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

santigimeno · 2016-09-20T11:31:31Z

I think the problem is that the test should have been failing before this change, but was not asserting on errors, just printing on console.error, so it did not. Solutions could be: increasing common.PORT on every call to server listen or wait for the current server to be closed before start listening on the next.

imyller · 2016-09-20T11:54:13Z

I think you're absolute right @santigimeno, Thank you.

btw @addaleax Nice to see that code+learn session cleanups have now resulted us in finding an pre-existing unnoticed test issue.

jasnell

Generally LGTM. If the assert.ifError was used I'd be even happier tho ;-)

imyller · 2016-09-20T19:33:29Z

test/sequential/test-net-server-address.js


-server_ipv4.listen(common.PORT, common.localhostIPv4, function() {
-  var address_ipv4 = server_ipv4.address();
+server_ipv4.listen(common.PORT, common.localhostIPv4, common.mustCall(() => {


common.PORT+1

imyller · 2016-09-20T19:33:40Z

test/sequential/test-net-server-address.js

-server_ipv4.listen(common.PORT, common.localhostIPv4, function() {
-  var address_ipv4 = server_ipv4.address();
+server_ipv4.listen(common.PORT, common.localhostIPv4, common.mustCall(() => {
+  const address_ipv4 = server_ipv4.address();
  assert.strictEqual(address_ipv4.address, common.localhostIPv4);
  assert.strictEqual(address_ipv4.port, common.PORT);


common.PORT+1

imyller · 2016-09-20T19:33:52Z

test/sequential/test-net-server-address.js


-server_ipv6.listen(common.PORT, localhost_ipv6, function() {
-  var address_ipv6 = server_ipv6.address();
+server_ipv6.listen(common.PORT, localhost_ipv6, common.mustCall(() => {


common.PORT+2

imyller · 2016-09-20T19:34:07Z

test/sequential/test-net-server-address.js

-server_ipv6.listen(common.PORT, localhost_ipv6, function() {
-  var address_ipv6 = server_ipv6.address();
+server_ipv6.listen(common.PORT, localhost_ipv6, common.mustCall(() => {
+  const address_ipv6 = server_ipv6.address();
  assert.strictEqual(address_ipv6.address, localhost_ipv6);
  assert.strictEqual(address_ipv6.port, common.PORT);


common.PORT+2

imyller · 2016-09-20T19:36:14Z

test/sequential/test-net-server-address.js


 // Specify the port number
-server1.listen(common.PORT, function() {
-  var address = server1.address();
+server1.listen(common.PORT, common.mustCall(() => {


common.PORT+3

imyller · 2016-09-20T19:36:21Z

test/sequential/test-net-server-address.js

-server1.listen(common.PORT, function() {
-  var address = server1.address();
+server1.listen(common.PORT, common.mustCall(() => {
+  const address = server1.address();
  assert.strictEqual(address.address, anycast_ipv6);
  assert.strictEqual(address.port, common.PORT);


common.PORT+3

imyller · 2016-09-20T19:39:01Z

@akito0107

To fix issue with multiple servers listening on same port (EADDRINUSE), could you please upgrade each common.PORT reference so that each server gets a separate port.

I've pointed out the lines in my previous code comments.

You can do common.PORT + <n> or alternatively common.PORT++ for each server.listen() call.

imyller · 2016-09-25T19:28:03Z

/ping @akito0107

yosuke-furukawa · 2016-09-28T03:38:22Z

According to this PR, #8694
we would be better to use listen(0) instead of common.PORT.

imyller · 2016-09-28T07:17:49Z

@yosuke-furukawa

Generally I'd agree, but this PR is already referenced for this in #8694. Specific purpose here is to test with fixed port numbers instead of automatic port assignment.

#8694 (comment)

yosuke-furukawa · 2016-09-28T09:50:45Z

@imyller
oops, I didn't see the test detail. Thank you. @akito0107 please ignore my comment.

Refactored test as a below - 'var' to 'const' - functon to arrow function - using common.mustCall() and common.fail()

akito0107 · 2016-09-28T10:09:43Z

@imyller
Sorry for late reply...

I fixed it!

imyller · 2016-09-28T19:59:28Z

Thank you, @akito0107 !

Let's see how this works.

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

imyller · 2016-09-28T20:32:33Z

CI run is ok. Failures are known/unrelated issues.

imyller · 2016-09-29T07:34:35Z

I'll start landing this:

Five LGTMs
No objections
CI tests passed (only known/unrelated failures)

imyller · 2016-09-29T07:38:40Z

Landed in 506cda6

Thank you for your effort and contribution, @akito0107 👍

Refactored test: - 'var' to 'const' - functon to arrow function - using common.mustCall() and common.fail() PR-URL: #8586 Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

mscdex added the net Issues and PRs related to the net subsystem. label Sep 17, 2016

akito0107 force-pushed the fix-test-net-server-address branch from d30af72 to 6a9052d Compare September 17, 2016 14:39

targos approved these changes Sep 17, 2016

View reviewed changes

cjihrig reviewed Sep 17, 2016

View reviewed changes

cjihrig approved these changes Sep 17, 2016

View reviewed changes

imyller approved these changes Sep 19, 2016

View reviewed changes

imyller self-assigned this Sep 19, 2016

jasnell approved these changes Sep 20, 2016

View reviewed changes

imyller reviewed Sep 20, 2016

View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

imyller mentioned this pull request Sep 21, 2016

doc: Recommend using port 0 instead of common.PORT #8694

Closed

2 tasks

imyller added the wip Issues and PRs that are still a work in progress. label Sep 27, 2016

imyller removed the wip Issues and PRs that are still a work in progress. label Sep 27, 2016

test: improve test-net-server-address.js

7550376

Refactored test as a below - 'var' to 'const' - functon to arrow function - using common.mustCall() and common.fail()

akito0107 force-pushed the fix-test-net-server-address branch from 6a9052d to 7550376 Compare September 28, 2016 10:03

imyller closed this Sep 29, 2016

imyller removed their assignment Sep 29, 2016

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016


		server_ipv6.on('error', function(e) {
		server_ipv6.on('error', (e) => {

test: improve test-net-server-address.js #8586

test: improve test-net-server-address.js #8586

Conversation

akito0107 commented Sep 17, 2016

Checklist

Affected core subsystem(s)

Description of change

yosuke-furukawa commented Sep 17, 2016

cjihrig commented Sep 17, 2016

akito0107 commented Sep 17, 2016

targos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig commented Sep 17, 2016

imyller left a comment

Choose a reason for hiding this comment

imyller commented Sep 19, 2016

imyller commented Sep 19, 2016

imyller commented Sep 19, 2016

imyller commented Sep 20, 2016

santigimeno commented Sep 20, 2016

imyller commented Sep 20, 2016

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imyller Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imyller commented Sep 20, 2016

imyller commented Sep 25, 2016

yosuke-furukawa commented Sep 28, 2016

imyller commented Sep 28, 2016

yosuke-furukawa commented Sep 28, 2016

akito0107 commented Sep 28, 2016

imyller commented Sep 28, 2016

imyller commented Sep 28, 2016

imyller commented Sep 29, 2016 • edited Loading

imyller commented Sep 29, 2016

targos Sep 17, 2016 •

edited

Loading

imyller Sep 20, 2016 •

edited

Loading

imyller commented Sep 29, 2016 •

edited

Loading