Test/improve deepstrict equal debug message #19629
Conversation
…com:fatahn/node into test/improve-deepstrict_equal-debug-message
|
@fatahn thanks a lot for contributing to Node.js 🎉. The change as it is right now needs some deeper look at it. Right now the same information is already logged in the line above. So logging the information again is redundant. Another issue with the change is that it uses a multiline template string with a lot of extra whitespace. So it is going to look like e.g., Since the value is already logged, there is no need to show that at all anymore and the best way is probably to keep it as is. I personally am against landing this therefore as I do not see a good way to improve the message here anymore. |
|
@BridgeAR I agree that the template-string should not span multiple lines. It should probably be this (or something similar) instead: const msg = 'timeout on http request called too much.';
assert.strictEqual(
requests_done,
requests_sent,
`${msg} requests_done: ${requests_done}, requests_sent: ${requests_sent}`
); |
|
@BridgeAR Regarding the duplication of the preceding |
|
One more possibility: Move the message to a comment and omit the message parameter entirely: // check that timeout on http request was not called too much
assert.strictEqual(requests_done, requests_sent); |
|
Thanks alot @Trott |
|
The only failure in CI appears to be another one of the "test process crashes but does not print a stack trace" issues. @BridgeAR 01:30:00 not ok 790 parallel/test-http-dump-req-when-res-ends
01:30:00 ---
01:30:00 duration_ms: 5.911
01:30:00 severity: fail
01:30:00 stack: |-
01:30:00 resume called
01:30:00 events.js:167
01:30:00 throw er; // Unhandled 'error' event
01:30:00 ^
01:30:00
01:30:00 Error: write EPIPE
01:30:00 at WriteWrap.afterWrite [as oncomplete] (net.js:832:14)
01:30:00 Emitted 'error' event at:
01:30:00 at Socket.socketErrorListener (_http_client.js:371:9)
01:30:00 at Socket.emit (events.js:182:13)
01:30:00 at onwriteError (_stream_writable.js:434:12)
01:30:00 at onwrite (_stream_writable.js:459:5)
01:30:00 at _destroy (internal/streams/destroy.js:40:7)
01:30:00 at Socket._destroy (net.js:597:3)
01:30:00 at Socket.destroy (internal/streams/destroy.js:32:8)
01:30:00 at WriteWrap.afterWrite [as oncomplete] (net.js:834:10)
01:30:00 ... |
|
Re-running the one failed CI task, as the failure was unrelated: https://ci.nodejs.org/job/node-test-commit-linux-containered/3255/ |
|
@Trott it seems like it does actually contain some debugging information? So to me it seems to be something else than the ones that did not print anything at all. I suggest to open a new issue for this one. |
|
(CI re-run was green.) |
|
Landed in 6259712 |
Use the default assert.strictEqual() message so that unequal values are shown in the event of an AssertionError. PR-URL: nodejs#19629 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
|
@fatahn Congratulations on your first contribution and thanks! 🎉 |
Use the default assert.strictEqual() message so that unequal values are shown in the event of an AssertionError. PR-URL: #19629 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Use the default assert.strictEqual() message so that unequal values are shown in the event of an AssertionError. PR-URL: #19629 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
This PR improves debugging output message from assert.strictEqual() function.
The function's 3rd param is a string, meaning that the values of
requests_doneandrequests_sentwere not being included in the message. But those values are useful for debugging.So I Changed the string to a template literal that will include those two values.
Checklist