test: updated error message for messages count #15979
Conversation
Trott
added
the
code-and-learn
| @@ -112,7 +112,8 @@ function launchChildProcess() { | |||
| worker.pid, count); | |||
|
|
|||
| 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!
|
Oh dear...I get caught by that 'i before e' rule when typing all the time! Thank you for the catch! |
| @@ -112,7 +112,8 @@ function launchChildProcess() { | |||
| worker.pid, count); | |||
|
|
|||
| 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);There was a problem hiding this comment.
See #15998 (comment). It's the same here.
|
I went and fixed the nit from @lpinca. I think this is ready to go. PTAL. |
|
Landed in 65b8d21, thank you! 🎉 |
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: #15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs/node#15979 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)