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: removed unhelpful message from assert.strictEqual() calls #19525

Closed
wants to merge 1 commit into from

Conversation

willhayslett
Copy link
Contributor

Converted the 'message' argument values from the last two free socket
assert.strictEqual() calls to code comments as they fail to provide the
necessary details and values specific to why the test is failing. The
default message returned from the strictEqual() call should provide
sufficient details for debugging errors.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Converted the 'message' argument values from the last two free socket
assert.strictEqual() calls to code comments as they fail to provide the
necessary details and values specific to why the test is failing. The
default message returned from the strictEqual() call should provide
sufficient details for debugging errors.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 22, 2018
@trivikr trivikr added the http Issues or PRs related to the http subsystem. label Mar 22, 2018
@Trott
Copy link
Member

Trott commented Mar 22, 2018

@Trott
Copy link
Member

Trott commented Mar 23, 2018

CI failures appear unrelated.

ARM fanned re-run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/15342/

pLinux re-run: https://ci.nodejs.org/job/node-test-commit-plinux/16259/

@willhayslett
Copy link
Contributor Author

Hi @Trott, anything else needed from me to get this merged? Can we just re-run the node-test-pull-request job or do I need changes on my end?

@Trott
Copy link
Member

Trott commented Mar 24, 2018

CI re-runs were green so this can land once it's been open for 72 hours (so in another 4 hours from now) as long as no Collaborator comes along with an objection.

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

Awesome! Thanks, @Trott!

@Trott
Copy link
Member

Trott commented Mar 24, 2018

Oh, it only needs 48 hours because it was opened on a Wednesday (in my timezone, anyway--our 48/72 week/weekend rule has some ambiguity). I'll land now.

@Trott
Copy link
Member

Trott commented Mar 24, 2018

Meh, our tooling is telling me to wait 3 more hours. I'll defer to the tooling. Sorry for all the comment noise.

@willhayslett
Copy link
Contributor Author

All good. Thanks for keeping me updated!

trivikr pushed a commit that referenced this pull request Mar 25, 2018
Converted the 'message' argument values from the last two free socket
assert.strictEqual() calls to code comments as they fail to provide the
necessary details and values specific to why the test is failing. The
default message returned from the strictEqual() call should provide
sufficient details for debugging errors.

PR-URL: #19525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@trivikr
Copy link
Member

trivikr commented Mar 25, 2018

Landed in d101942

@willhayslett Congratulations on your first commit in Node.js core! 🎉

@trivikr trivikr closed this Mar 25, 2018
@willhayslett
Copy link
Contributor Author

Whew! Thanks, @trivikr!

targos pushed a commit that referenced this pull request Mar 27, 2018
Converted the 'message' argument values from the last two free socket
assert.strictEqual() calls to code comments as they fail to provide the
necessary details and values specific to why the test is failing. The
default message returned from the strictEqual() call should provide
sufficient details for debugging errors.

PR-URL: #19525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 3, 2018
Converted the 'message' argument values from the last two free socket
assert.strictEqual() calls to code comments as they fail to provide the
necessary details and values specific to why the test is failing. The
default message returned from the strictEqual() call should provide
sufficient details for debugging errors.

PR-URL: #19525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@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. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants