test: fix test-cluster-worker-init.js flakyness#8703
Uh oh!
There was an error while loading. Please reload this page.
Conversation
imyller commented Sep 21, 2016
/cc @nodejs/testing |
CI stress test with pi1: |
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
I'm very +1 for removing timers from tests.
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.
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.
imyller commented Sep 21, 2016
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. |
cjihrig left a comment
cjihrig left a comment There was a problem hiding this comment.
The change to arrow functions seems pointless, but I won't fight it. LGTM
imyller commented Sep 21, 2016
New CI after requested modifications: https://ci.nodejs.org/job/node-test-pull-request/4202/ |
Trott commented Sep 22, 2016
LGTM |
imyller commented Sep 22, 2016
@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? |
Trott commented Sep 22, 2016
Not in this case. I think it's fine. |
Trott commented Sep 22, 2016
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. |
imyller commented Sep 22, 2016
@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. |
Trott commented Sep 22, 2016
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:
|
imyller commented Sep 23, 2016
landed in 66369d0 |
Update test to match current test guidelines and use common.mustCall instead of unref'd timer. PR-URL: #8703Fixes: #8700 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Update test to match current test guidelines and use common.mustCall instead of unref'd timer. PR-URL: #8703Fixes: #8700 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Update test to match current test guidelines and use common.mustCall instead of unref'd timer. PR-URL: #8703Fixes: #8700 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
Update test to match current test guidelines and use common.mustCall instead of unref'd timer. PR-URL: #8703Fixes: #8700 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>
6 participants
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