-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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: improve assertion message in dgram test #16029
test: improve assertion message in dgram test #16029
Conversation
@@ -15,7 +15,8 @@ function onsend() { | |||
if (sent++ < limit) { | |||
client.send(chunk, 0, chunk.length, port, common.localhostIPv4, onsend); | |||
} else { | |||
assert.strictEqual(async, true, 'Send should be asynchronous.'); | |||
assert.strictEqual(async, true, `Send should be asynchronous. | |||
Saw ${sent} messages, expected fewer than ${limit}.`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use multiline template strings. This adds lots of whitespace to the message. Instead a string concat should be used here (and the template string in the second line).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also just remove the message all together as that would also increase the readability a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shakeelmohamed ... thank you so much for the pull request and for participating in the code and learn! As @BridgeAR indicates, multiline template strings are less than ideal and are not something that we use generally. Also, the added message does not quite fit the check. As @BridgeAR suggests, I would just remove the message altogether and leave it as assert.strictEqual(async, true)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test