test: fix test-cluster-worker-init.js flakyness #8703
Conversation
|
/cc @nodejs/testing |
|
CI stress test with pi1: |
| timer.unref(); | ||
|
|
||
| worker.on('message', function(message) { | ||
| worker.on('message', common.mustCall((message) => { |
There was a problem hiding this comment.
I think this is the only common.mustCall() you need.
There was a problem hiding this comment.
I'd prefer to keep the other common.mustCall()'s because they are actually mandatory for passing the test too. At least it gives more granularity to error msg if test fails if we have them.
There was a problem hiding this comment.
Also, earlier we had no clear picture what phase actually failed. I wanted to improve that situation.
There was a problem hiding this comment.
I understand. But I also think that introduces a slippery slope where literally every callback in every test is inside of a common.mustCall() (since it shouldn't be in the test if it isn't necessary).
EDIT: With the exception of common.fail cases.
There was a problem hiding this comment.
Sure, I'll modify and I understand your concern.
My personal opinion still remains that in cases where verifying sequence of events is important, common.mustCall can be present in the middle-steps too. I categorized this test to be one of those cases.
| timer.unref(); | ||
|
|
||
| worker.on('message', function(message) { | ||
| worker.on('message', common.mustCall((message) => { | ||
| assert(message, 'did not receive expected message'); |
There was a problem hiding this comment.
Please change this to assert.strictEqual() to prevent surprises.
There was a problem hiding this comment.
Sure thing. It will be in the format
assert.strictEqual(message, true, 'did not receive expected message');
because worker sends true as message value.
There was a problem hiding this comment.
Right. I just wanted to rule out any other truthy value somehow showing up. Thanks for making the changes.
Update test to match current test guidelines and use common.mustCall instead of unref'd timer. Fixes: nodejs#8700
|
@cjihrig Please re-review at your convenience. The changes you requested are in. |
|
New CI after requested modifications: https://ci.nodejs.org/job/node-test-pull-request/4202/ |
|
LGTM |
|
@rvagg had to boot CI Pi's (for unrelated reasons) so stress test aborted after 328 runs. Total we have 428 successfull stress test runs. Do you feel that we need to complete full 999 cycle in the stress test CI for this one? |
Not in this case. I think it's fine. |
|
Stress test against master: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/15/label=pi1-raspbian-wheezy/console Failed 10 out of 100 runs with current master. So 328 successful runs with 0 failures using the code change in this PR seems pretty convincing. |
|
@Trott Just asking: do we apply 48 hour window to this PR or does arm-fanned CI flakyness require more urgent landing? LGTMs are there and CI is green. |
|
I'm inclined to wait. There's still plenty of other armv6 failures to sort out, so waiting another 24 hours on this one isn't going to hurt anything. However, if anyone on @nodejs/testing feels differently, I will defer to them on it. |
|
Landing:
|
|
landed in 66369d0 |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test, cluster
Description of change
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.
Fixes: #8700