test: updated error message for messages count#15979
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Trott added the code-and-learn 
| assert.strictEqual(count,messages.length, | ||
| 'A worker received an invalid multicast message'); | ||
| `A worker received an invalid multicast message: | ||
| Recieved ${messages.length}, should be ${count}`); |
There was a problem hiding this comment.
Using multiline template strings is actually discouraged. This would add lots of extra whitespace to the error message. Please use this instead
'A worker received an invalid multicast message'+`Received ${messages.length}, should be ${count}`There was a problem hiding this comment.
Thank you! I didn't know about that, we do try to keep messages very short in our code base at work so I didn't know how to handle longer strings! Will fix!
dinophile commented Oct 7, 2017
Oh dear...I get caught by that 'i before e' rule when typing all the time! Thank you for the catch! |
| assert.strictEqual(count,messages.length, | ||
| 'A worker received an invalid multicast message'); | ||
| 'A worker received an invalid multicast message'+ | ||
| `Received ${messages.length}, should be ${count}`); |
There was a problem hiding this comment.
If I'm not wrong it should be the opposite: Received ${count}, should be ${messages.length}.
I also think it makes sense to remove the error message in favor of the default one.
There was a problem hiding this comment.
I thought count is the expected number and messages.length is the actual number recieved? I'm actually away from my computer until the 23rd (on vacation!) So I can't take a look until then! But I agree taking out the error message would be a good idea!
There was a problem hiding this comment.
No, it is actually the other way around.
There was a problem hiding this comment.
Even if reversed the message is still misleading as the assertion is actually run when all messages are received by all workers, so my suggestion is to get rid of the message argument completely and use the default error message.
assert.strictEqual(count,messages.length);
Trott commented Oct 27, 2017
I went and fixed the nit from @lpinca. I think this is ready to go. PTAL. |
joyeecheung commented Oct 27, 2017
tniessen commented Oct 28, 2017
Landed in 65b8d21, thank you! 🎉 |
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>
12 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)