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: refactor/cleanup a number of cluster tests #8261

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jasnell
Member

jasnell commented Aug 24, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, cluster

Description of change

Some general refactoring / improvements to various cluster related tests:

  • Move shared code into common
  • Favor use of strictEqual
  • Add some missing common.mustCalls
  • Other general cleanup
@Trott

View changes

Show outdated Hide outdated test/parallel/test-cluster-dgram-1.js
@@ -82,15 +79,15 @@ function worker() {
// Create udp socket and start listening.
var socket = dgram.createSocket('udp4');
socket.on('message', function(data, info) {
socket.on('message', common.mustCall((data, info) => {
received++;
// Every 10 messages, notify the master.
if (received == PACKETS_PER_WORKER) {

This comment has been minimized.

@Trott

Trott Aug 24, 2016

Member

Nit: Since you're already working nearby, maybe change == to ===?

@Trott

Trott Aug 24, 2016

Member

Nit: Since you're already working nearby, maybe change == to ===?

@Trott

View changes

Show outdated Hide outdated test/parallel/test-cluster-dgram-2.js
@@ -28,19 +28,18 @@ function master() {
// Disconnect workers when the expected number of messages have been
// received.
socket.on('message', function(data, info) {
socket.on('message', common.mustCall((data, info) => {
received++;
if (received == PACKETS_PER_WORKER * NUM_WORKERS) {

This comment has been minimized.

@Trott

Trott Aug 24, 2016

Member

Nit: == -> ===?

@Trott

Trott Aug 24, 2016

Member

Nit: == -> ===?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-basic.js
//Kill process when worker is killed
cluster.on('exit', function() {
cluster.on('exit', common.mustCall(() => {
process.exit(0);

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

While you're here... I think process.exit(0) can be removed.

@santigimeno

santigimeno Aug 25, 2016

Member

While you're here... I think process.exit(0) can be removed.

b.send('START');
});
}));

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

If I read this correctly, this listener might be called more than once, so using common.mustCall can be a problem

@santigimeno

santigimeno Aug 25, 2016

Member

If I read this correctly, this listener might be called more than once, so using common.mustCall can be a problem

This comment has been minimized.

@jasnell

jasnell Aug 25, 2016

Member

Could be, but is not. The test calls the listener exactly once.

@jasnell

jasnell Aug 25, 2016

Member

Could be, but is not. The test calls the listener exactly once.

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

Sorry, I don't follow :(. Is it possible that READY message is never received?

@santigimeno

santigimeno Aug 25, 2016

Member

Sorry, I don't follow :(. Is it possible that READY message is never received?

This comment has been minimized.

@santigimeno

santigimeno Aug 30, 2016

Member

For the same reason as the other message listener I would remove the if (typeof m === 'object') return; here.

@santigimeno

santigimeno Aug 30, 2016

Member

For the same reason as the other message listener I would remove the if (typeof m === 'object') return; here.

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-bind-twice.js
ok = true;
a.send('QUIT');
b.send('QUIT');
});
}));

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

Same as above

@santigimeno

santigimeno Aug 25, 2016

Member

Same as above

This comment has been minimized.

@jasnell

jasnell Aug 25, 2016

Member

Ditto. The test calls the listener exactly once.

@jasnell

jasnell Aug 25, 2016

Member

Ditto. The test calls the listener exactly once.

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

Then, as ok must be true on exit, can the if (typeof m === 'object') return; be removed?

@santigimeno

santigimeno Aug 25, 2016

Member

Then, as ok must be true on exit, can the if (typeof m === 'object') return; be removed?

This comment has been minimized.

@jasnell

jasnell Aug 26, 2016

Member

No, because that filters out the possibility of system messages... which is a whole other issue. I don't believe we're actually getting any system messages but if we did, then the changes to this test would catch it and flag it as a mustCall count error.

@jasnell

jasnell Aug 26, 2016

Member

No, because that filters out the possibility of system messages... which is a whole other issue. I don't believe we're actually getting any system messages but if we did, then the changes to this test would catch it and flag it as a mustCall count error.

This comment has been minimized.

@santigimeno

santigimeno Aug 27, 2016

Member

Understood. In the case of receiving system messages if the if (...) were to be removed the assert would catch the failure, but I'm fine anyway.

@santigimeno

santigimeno Aug 27, 2016

Member

Understood. In the case of receiving system messages if the if (...) were to be removed the assert would catch the failure, but I'm fine anyway.

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-bind-twice.js
} else if (id === 'two') {
if (cluster.isMaster) return startWorker();
let ok = false;
process.on('exit', function() {
process.on('exit', () => {

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

I think you can get rid of the ok variable and the exit listener because server.on('error') listener is wrapped in common.mustCall()

@santigimeno

santigimeno Aug 25, 2016

Member

I think you can get rid of the ok variable and the exit listener because server.on('error') listener is wrapped in common.mustCall()

This comment has been minimized.

@jasnell

jasnell Aug 25, 2016

Member

I don't believe so. The ok is only set under certain conditions that are not covered simply by the addition of common.mustCall(). For instance, if the message event is triggered only because of system messages, the necessary common.mustCall() won't be triggered.

@jasnell

jasnell Aug 25, 2016

Member

I don't believe so. The ok is only set under certain conditions that are not covered simply by the addition of common.mustCall(). For instance, if the message event is triggered only because of system messages, the necessary common.mustCall() won't be triggered.

This comment has been minimized.

@santigimeno

santigimeno Aug 27, 2016

Member

I understand that, but if common.mustCall is not triggered, wouldn't the test fail anyway?

@santigimeno

santigimeno Aug 27, 2016

Member

I understand that, but if common.mustCall is not triggered, wouldn't the test fail anyway?

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-dgram-1.js
worker.on('disconnect', function() {
worker.on('disconnect', common.mustCall(() => {
assert(received === PACKETS_PER_WORKER);

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

assert.strictEqual?

@santigimeno

santigimeno Aug 25, 2016

Member

assert.strictEqual?

// buffer result
var result = '';
socket.on('data', function(chunk) { result += chunk; });
socket.on('data', common.mustCall((chunk) => { result += chunk; }));

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

There's no guarantees that this listener is called only once.

@santigimeno

santigimeno Aug 25, 2016

Member

There's no guarantees that this listener is called only once.

This comment has been minimized.

@jasnell

jasnell Aug 25, 2016

Member

Within this test, it should only be called once.

@jasnell

jasnell Aug 25, 2016

Member

Within this test, it should only be called once.

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

Maybe I'm wrong, but my understanding is that there's a chance (remote chance) that the data arrives in multiple chunks.

@santigimeno

santigimeno Aug 25, 2016

Member

Maybe I'm wrong, but my understanding is that there's a chance (remote chance) that the data arrives in multiple chunks.

This comment has been minimized.

@jasnell

jasnell Aug 26, 2016

Member

Remote chance yes, but in every run of this test that I've seen, it's only happened once. If it starts happening more than once, then there's a chance that something internal has changed that may not be correct. This would highlight such a change, and if it's legitimate, then simply updating the mustCall count should be sufficient.

@jasnell

jasnell Aug 26, 2016

Member

Remote chance yes, but in every run of this test that I've seen, it's only happened once. If it starts happening more than once, then there's a chance that something internal has changed that may not be correct. This would highlight such a change, and if it's legitimate, then simply updating the mustCall count should be sufficient.

This comment has been minimized.

@santigimeno

santigimeno Aug 27, 2016

Member

I've seen test failures in the past because multiple data event calls were not considered, but we can wait for the test to fail before making any change.

@santigimeno

santigimeno Aug 27, 2016

Member

I've seen test failures in the past because multiple data event calls were not considered, but we can wait for the test to fail before making any change.

This comment has been minimized.

@Trott

Trott Aug 27, 2016

Member

I'm also not going to stand in the way of this landing over it, but I agree with @santigimeno that there's value in accounting for the possibility of multiple data events even if it is something that will never actually happen in this specific test.

People should be able to use our tests as model code. Obviously we can't always accomplish that. Sometimes, we need to test a deprecated or otherwise ill-advised practice. But this is not the situation here. Assuming all your data will arrive in a single data event is a gotcha of Node.js that we should do our level best to discourage.

But this is a largely philosophical position. If it lands as-is, then heck, I can just go open a good first contribution issue for changing it. :-D

@Trott

Trott Aug 27, 2016

Member

I'm also not going to stand in the way of this landing over it, but I agree with @santigimeno that there's value in accounting for the possibility of multiple data events even if it is something that will never actually happen in this specific test.

People should be able to use our tests as model code. Obviously we can't always accomplish that. Sometimes, we need to test a deprecated or otherwise ill-advised practice. But this is not the situation here. Assuming all your data will arrive in a single data event is a gotcha of Node.js that we should do our level best to discourage.

But this is a largely philosophical position. If it lands as-is, then heck, I can just go open a good first contribution issue for changing it. :-D

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-disconnect.js
online += 1;
if (online === workers * servers) {
cb();
}
});
}, 2));

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

s/2/servers ?

@santigimeno

santigimeno Aug 25, 2016

Member

s/2/servers ?

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-master-error.js
// Add worker pid to list and progress tracker
if (data.cmd === 'worker') {
workers.push(data.workerPID);
}
});
}, 2));

This comment has been minimized.

@santigimeno

santigimeno Aug 25, 2016

Member

Maybe move totalWorkers to the top of the test and use it here?

@santigimeno

santigimeno Aug 25, 2016

Member

Maybe move totalWorkers to the top of the test and use it here?

@Trott

View changes

Show outdated Hide outdated test/parallel/test-cluster-worker-exit.js
try {
checkResults(expected_results, results);
} catch (exc) {
console.error('FAIL: ' + exc.message);
if (exc.name != 'AssertionError') {

This comment has been minimized.

@Trott

Trott Aug 25, 2016

Member

Maybe change != to !== while you're in here?

@Trott

Trott Aug 25, 2016

Member

Maybe change != to !== while you're in here?

This comment has been minimized.

@jasnell

jasnell Aug 25, 2016

Member

Done!

@jasnell

jasnell Aug 25, 2016

Member

Done!

@santigimeno

View changes

Show outdated Hide outdated test/parallel/test-cluster-bind-twice.js
process.on('exit', function() {
process.on('exit', () => {
assert(ok);
});

This comment has been minimized.

@santigimeno

santigimeno Aug 27, 2016

Member

I think the process.on('exit') and the ok variable can also be removed here.

@santigimeno

santigimeno Aug 27, 2016

Member

I think the process.on('exit') and the ok variable can also be removed here.

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Aug 27, 2016

Member

The changes LGTM with a couple of comments / suggestions.

Member

santigimeno commented Aug 27, 2016

The changes LGTM with a couple of comments / suggestions.

test: refactor/cleanup a number of cluster tests
* Move shared code into common
* Favor use of strictEqual
* Add some missing common.mustCalls
* Other general cleanup
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

@santigimeno ... updated! PTAL!

Member

jasnell commented Aug 30, 2016

@santigimeno ... updated! PTAL!

@santigimeno

This comment has been minimized.

Show comment
Hide comment
@santigimeno

santigimeno Aug 30, 2016

Member

LGTM with one tiny comment that you can ignore. Thanks!

Member

santigimeno commented Aug 30, 2016

LGTM with one tiny comment that you can ignore. Thanks!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

Trying again due to a build bot failure: https://ci.nodejs.org/job/node-test-pull-request/3913/

Member

jasnell commented Aug 31, 2016

Trying again due to a build bot failure: https://ci.nodejs.org/job/node-test-pull-request/3913/

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 1, 2016

Member

CI was green. woo!

Member

jasnell commented Sep 1, 2016

CI was green. woo!

jasnell added a commit that referenced this pull request Sep 1, 2016

test: refactor/cleanup a number of cluster tests
* Move shared code into common
* Favor use of strictEqual
* Add some missing common.mustCalls
* Other general cleanup

PR-URL: #8261
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 1, 2016

Member

Landed in baa0ffd

Member

jasnell commented Sep 1, 2016

Landed in baa0ffd

@jasnell jasnell closed this Sep 1, 2016

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 1, 2016

Member

Looking at the AIX runs I've seen a few failures of: parallel/test-cluster-dgram-1

I wonder if its related to this change ?

Member

mhdawson commented Sep 1, 2016

Looking at the AIX runs I've seen a few failures of: parallel/test-cluster-dgram-1

I wonder if its related to this change ?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 2, 2016

Member

If the failures are new, entirely possible. Have a stack trace?

Member

jasnell commented Sep 2, 2016

If the failures are new, entirely possible. Have a stack trace?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Sep 2, 2016

Member

opened this issue to track #8380

Member

mhdawson commented Sep 2, 2016

opened this issue to track #8380

mhdawson added a commit to mhdawson/io.js that referenced this pull request Sep 2, 2016

test: revert changes to test/parallel/test-cluster-dgram-1.js
We've started to see frequent failures in
test/parallel/test-cluster-dgram-1.js on AIX since the refactoring
in nodejs#8261.  Revert changes
until we have time to investigate properly.

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

test: refactor/cleanup a number of cluster tests
* Move shared code into common
* Favor use of strictEqual
* Add some missing common.mustCalls
* Other general cleanup

PR-URL: nodejs#8261
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

test: refactor/cleanup a number of cluster tests
* Move shared code into common
* Favor use of strictEqual
* Add some missing common.mustCalls
* Other general cleanup

PR-URL: #8261
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 30, 2016

Member

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

Member

MylesBorins commented Sep 30, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment