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 flaky test-net-socket-local-address #4650

Closed
wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 12, 2016

Prior to this commit, the test was flaky because it was executing the majority of its logic in a function called from the client and multiple events on the server. This commit simplifies the test by separating the server's connection and listening events, and isolating the client logic.

Refs: #4476 and #4644
R= @Trott

@r-52 r-52 added net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. labels Jan 12, 2016
@jasnell
Copy link
Member

jasnell commented Jan 12, 2016

LGTM

@Trott
Copy link
Member

Trott commented Jan 13, 2016

These changes mean that the test no longer triggers on the bug it was written for. This test is for a bug that was present in Node.js 3.0.0 and this version of the test (with appropriate modifications to remove fat arrows, etc. which were not supported in Node 3.0.0) does not fire an assertion whereas the current version does.

For the record, here's the way it ended up after removing fat arrows, etc. so that it could run in Node 3.0.0:

'use strict';
// const common = require('../common');
const assert = require('assert');
const net = require('net');

// skip test in FreeBSD jails
// if (common.inFreeBSDJail) {
//   console.log('1..0 # Skipped: In a FreeBSD jail');
//   return;
// }

var conns = 0;
var clientLocalPorts = [];
var serverRemotePorts = [];

const server = net.createServer(function(socket) {
  serverRemotePorts.push(socket.remotePort);
  socket.end();
});

server.on('close', function() {
  assert.deepEqual(clientLocalPorts, serverRemotePorts,
                   'client and server should agree on the ports used');
  assert.strictEqual(2, conns);
});

server.listen(12346, '127.0.0.1', connect);

function connect() {
  if (conns === 2) {
    server.close();
    return;
  }

  const client = new net.Socket();

  conns++;
  client.once('close', connect);
  client.connect(12346, '127.0.0.1', function() {
    clientLocalPorts.push(client.localPort);
  });
}

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 13, 2016

Good catch. Simple fix, just have to move the const client = new net.Socket(); up higher so that the same socket is reused. I'll update the PR.

Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 13, 2016

@Trott updated. Verified that moving the client socket triggers the original bug in v3.0.0.

@jbergstroem
Copy link
Member

Lets try it in CI: https://ci.nodejs.org/job/node-test-commit/1709/

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 13, 2016

CI has some red, but none in this test. We should probably run a stress test too.

@Trott
Copy link
Member

Trott commented Jan 13, 2016

@Trott
Copy link
Member

Trott commented Jan 13, 2016

Confirmed expected test failure in Node.js 3.0.0 and success in Node.js 3.3.0. So that's good.

LGTM if the stress test comes out clean.

cjihrig added a commit that referenced this pull request Jan 13, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: #4476
Refs: #4644
PR-URL: #4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 13, 2016

Thanks for the review. Stress test came through green. Landed in 6cfd0b5.

@cjihrig cjihrig closed this Jan 13, 2016
@cjihrig cjihrig deleted the local-addr branch January 13, 2016 16:44
rvagg pushed a commit that referenced this pull request Jan 14, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: #4476
Refs: #4644
PR-URL: #4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: #4476
Refs: #4644
PR-URL: #4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: #4476
Refs: #4644
PR-URL: #4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: nodejs#4476
Refs: nodejs#4644
PR-URL: nodejs#4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: nodejs#4476
Refs: nodejs#4644
PR-URL: nodejs#4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Prior to this commit, the test was flaky because it was
executing the majority of its logic in a function called from
the client and multiple events on the server. This commit
simplifies the test by separating the server's connection and
listening events, and isolating the client logic.

Refs: nodejs#4476
Refs: nodejs#4644
PR-URL: nodejs#4650
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants