Skip to content
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 deepstrict equal debug message #19629

Closed

Conversation

q3e
Copy link
Contributor

@q3e q3e commented Mar 27, 2018

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 27, 2018
@BridgeAR
Copy link
Member

@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
Copy link
Member

Trott commented Mar 28, 2018

@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
Copy link
Member

Trott commented Mar 28, 2018

@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
Copy link
Member

Trott commented Mar 28, 2018

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
Copy link
Contributor Author

q3e commented Mar 28, 2018

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

@Trott
Copy link
Member

Trott commented Mar 28, 2018

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

Yes.

@Trott
Copy link
Member

Trott commented Mar 29, 2018

@q3e
Copy link
Contributor Author

q3e commented Mar 29, 2018

Thanks alot @Trott

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2018
@Trott
Copy link
Member

Trott commented Mar 29, 2018

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
Copy link
Member

Trott commented Mar 29, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented Apr 3, 2018

@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
Copy link
Member

Trott commented Apr 3, 2018

(CI re-run was green.)

@Trott
Copy link
Member

Trott commented Apr 4, 2018

Landed in 6259712

Trott pushed a commit to Trott/io.js that referenced this pull request Apr 4, 2018
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>
@Trott Trott closed this Apr 4, 2018
@Trott
Copy link
Member

Trott commented Apr 4, 2018

@fatahn Congratulations on your first contribution and thanks! 🎉

targos pushed a commit that referenced this pull request Apr 4, 2018
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>
@targos targos mentioned this pull request Apr 4, 2018
BethGriggs pushed a commit that referenced this pull request Dec 4, 2018
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>
@BethGriggs BethGriggs mentioned this pull request Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants