Test/improve deepstrict equal debug message

[q3e]

This PR improves debugging output message from assert.strictEqual() function.

The function's 3rd param is a string, meaning that the values of requests_done and requests_sent were 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

• commit message follows commit guidelines

[BridgeAR]

@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., abc\n [a lot of whitespace] def. Instead, please concat strings if they exceed the 80 character limit.

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.

[Trott]

@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}`
  );

[Trott]

@BridgeAR Regarding the duplication of the preceding console.error() contents: Would you be amenable to this change if that console.error() call was removed? Although I personally have suggested that it's not always a good idea, there are certainly folks who have argued for removing console.error() and console.log() from tests. In this particular case, it seems like a good idea, especially because this test mixes console.error() and console.log(). That's generally bad idea in our tests because it could mean that a test run without test.py will have output ordered differently than a test run without test.py. (test.py outputs stdout and stderr in their entirety, first one, then the other. Without test.py, the output ends up intermingled in the terminal unless of course someone redirects them to different places.)

[Trott]

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);

[q3e]

Thanks @BridgeAR and @Trott for your feedback. @Trott you mean not showing the error message "check that timeout..." at all?

[Trott]

Thanks @BridgeAR and @Trott for your feedback. @Trott you mean not showing the error message "check that timeout..." at all?

Yes.

[Trott]

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

[q3e]

Thanks alot @Trott

[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

https://ci.nodejs.org/job/node-test-commit-linux-containered/3245/nodes=ubuntu1604_sharedlibs_debug_x64/console

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   ...

[Trott]

Re-running the one failed CI task, as the failure was unrelated: https://ci.nodejs.org/job/node-test-commit-linux-containered/3255/

[BridgeAR]

@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.

[Trott]

(CI re-run was green.)

[Trott]

Landed in 6259712

[Trott]

@fatahn Congratulations on your first contribution and thanks! 🎉