internet: improve assert message #15998

nikkipelchat · 2017-10-06T18:28:45Z

Used default message to help with debugging when someone comes across this error.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

internet

yosuke-furukawa · 2017-10-06T19:04:46Z

test/internet/test-dgram-broadcast-multi-process.js

@@ -153,7 +153,8 @@ if (process.argv[2] !== 'child') {
              assert.strictEqual(
                count,
                messages.length,
-                'A worker received an invalid multicast message'
+                `A worker received an invalid multicast message.
+                Received ${messages.length} and wanted ${count}.`


This message includes extra spaces.

This happens because it is a multi line template strings. In this case all spaces in the beginning of the second line are real spaces in the string. In Node.js we limit every string to a single line to prevent things like that. It should therefore either be changed now or while landing.

Hi, @nikkistonge! Welcome and thanks for doing this. It would be acceptable to use concatenation here:

'A worker received an invalid multicast message.' + `Received ${messages.length} and expected {$count}.`

That would address the comments above.

Sorry about the delay, didn't see these comments.

I didn't know that was how template strings worked, I've only ever used them for SQL and html stuff and both don't care about extra spaces so I assumed they got removed.

Changed my code to remove them and added just one space after the first sentence. @Trott

lpinca · 2017-10-13T07:45:57Z

test/internet/test-dgram-broadcast-multi-process.js

@@ -153,7 +153,8 @@ if (process.argv[2] !== 'child') {
              assert.strictEqual(
                count,
                messages.length,
-                'A worker received an invalid multicast message'
+                'A worker received an invalid multicast message. ' +
+                `Received ${messages.length} and wanted ${count}.`


Shouldn't this be Received ${count} and wanted ${messages.length}.?
Personally I would just remove the "message" argument and use the default error message.

No this is the right way I think, I asked at the code and learn to make sure. If they don't want the message they can close this PR

@nikkistonge I think you might be misunderstanding what @lpinca means in their comment. I think what @lpinca is trying to say is that messages.length and count should be swapped with each other in the message string added on line 157. It's backwards in the PR as it stands now.

Ok swapped them

Looking it over again, the feedback is correct that the messages.length and count are reversed in the message.

lpinca · 2017-10-17T12:53:23Z

test/internet/test-dgram-broadcast-multi-process.js

@@ -153,7 +153,8 @@ if (process.argv[2] !== 'child') {
              assert.strictEqual(
                count,
                messages.length,
-                'A worker received an invalid multicast message'
+                'A worker received an invalid multicast message. ' +
+                `Received ${count} and wanted ${messages.length}.`


@nikkistonge sorry to pester, but I think the message is still misleading.

If you look at the code, the above assertion is run only when all workers have received all the messages. After that the test validates that each message received by the workers matches the original using a counter for each worker. If the count does not match the assertion throws.

My point is that the "Received x and wanted y." feedback is wrong as all messages have actually been received but one ore more of them is invalid/corrupted.

If you could just refactor this into assert.strictEqual(count, messages.length); that would be 10/10 imho as this will remove any possible confusion by using the default error message.

Thanks.

Ok changed, now it will use default message :)

joyeecheung · 2017-10-18T06:18:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10808/

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasnell · 2017-10-18T15:49:00Z

Landed in 9960e53. Thank you @nikkistonge!

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

internet: improve assert message

8b2b6cc

nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017

yosuke-furukawa reviewed Oct 6, 2017

View reviewed changes

mscdex added the dgram Issues and PRs related to the dgram subsystem / UDP. label Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 7, 2017

Nikki St Onge added 2 commits October 12, 2017 17:03

Remove all white spaces

7374adb

Add one space

301e7f1

jasnell previously approved these changes Oct 13, 2017

View reviewed changes

lpinca reviewed Oct 13, 2017

View reviewed changes

Swap arguments

3523ff6

lpinca approved these changes Oct 16, 2017

View reviewed changes

lpinca reviewed Oct 17, 2017

View reviewed changes

Removed message

68f8786

joyeecheung approved these changes Oct 18, 2017

View reviewed changes

jasnell approved these changes Oct 18, 2017

View reviewed changes

jasnell pushed a commit that referenced this pull request Oct 18, 2017

test: improve assert message in internet test

9960e53

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

BridgeAR closed this Oct 18, 2017

lpinca mentioned this pull request Oct 18, 2017

test: updated error message for messages count #15979

Closed

4 tasks

nikkipelchat deleted the nikki/improve-assert-messages branch October 18, 2017 16:38

MylesBorins pushed a commit that referenced this pull request Oct 23, 2017

test: improve assert message in internet test

c6b2fa9

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins pushed a commit that referenced this pull request Nov 16, 2017

test: improve assert message in internet test

edf5c17

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

MylesBorins pushed a commit that referenced this pull request Nov 21, 2017

test: improve assert message in internet test

fe6829d

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 28, 2017

test: improve assert message in internet test

ab580c3

PR-URL: #15998 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internet: improve assert message #15998

internet: improve assert message #15998

nikkipelchat commented Oct 6, 2017 •

edited

Loading

yosuke-furukawa Oct 6, 2017

BridgeAR Oct 6, 2017

Trott Oct 9, 2017 •

edited

Loading

nikkipelchat Oct 12, 2017

lpinca Oct 13, 2017

nikkipelchat Oct 13, 2017 •

edited

Loading

Trott Oct 14, 2017

nikkipelchat Oct 16, 2017

lpinca Oct 17, 2017 •

edited

Loading

nikkipelchat Oct 17, 2017

joyeecheung commented Oct 18, 2017

jasnell commented Oct 18, 2017

internet: improve assert message #15998

internet: improve assert message #15998

Conversation

nikkipelchat commented Oct 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

yosuke-furukawa Oct 6, 2017

Choose a reason for hiding this comment

BridgeAR Oct 6, 2017

Choose a reason for hiding this comment

Trott Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

nikkipelchat Oct 12, 2017

Choose a reason for hiding this comment

lpinca Oct 13, 2017

Choose a reason for hiding this comment

nikkipelchat Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Oct 14, 2017

Choose a reason for hiding this comment

nikkipelchat Oct 16, 2017

Choose a reason for hiding this comment

lpinca Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

nikkipelchat Oct 17, 2017

Choose a reason for hiding this comment

joyeecheung commented Oct 18, 2017

jasnell commented Oct 18, 2017

nikkipelchat commented Oct 6, 2017 •

edited

Loading

Trott Oct 9, 2017 •

edited

Loading

nikkipelchat Oct 13, 2017 •

edited

Loading

lpinca Oct 17, 2017 •

edited

Loading