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 async hooks test error messages #13243

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Improve error messages in the async hooks tests, mostly by removing
unhelpful message parameters for assertions.

(@Trott, maybe we could have a lint rule that forbids literals as messages if this PR is somewhat well-received?)

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

test/async_hooks

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. dont-land-on-v4.x test Issues and PRs related to the tests. labels May 26, 2017
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels May 26, 2017
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack
Copy link
Contributor

refack commented May 29, 2017

From a partial scan I agree with 97% of removals.
Some might have value... like: assert.strictEqual(code, 1, 'exit code 1');
Would you wait a day or two before landing, I really want to review them all (It will be a shame to lose good information if it's in there)

@refack
Copy link
Contributor

refack commented May 29, 2017

(@Trott, maybe we could have a lint rule that forbids literals as messages if this PR is somewhat well-received?)

Sounds good, but maybe a little bit more lax, like if it's more than n characters long, or has at least 4 white space groups (meaning 5 words), or has some keywords like should, it might be good as well.
[edit] meanwhile it should be communicated to Collaborators not to approve these sort of unhelpful messages, as they actually hide the default which is better.

@addaleax
Copy link
Member Author

addaleax commented May 29, 2017

Some might have value... like: assert.strictEqual(code, 1, 'exit code 1');

I wouldn’t agree. exit code 1 is an unhelpful error message, because it just states the expected result; but that I can easily look up by looking at where the stack trace points. Providing the actual exit code is a lot more helpful.

Would you wait a day or two before landing, I really want to review them all (It will be a shame to lose good information if it's in there)

Sure, this isn’t urgent.

@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

ping @refack ... have you had a chance to review yet?

@refack
Copy link
Contributor

refack commented Jun 1, 2017

halfway there.

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Thanks for the cleanup.

Improve error messages in the async hooks tests, mostly by removing
unhelpful `message` parameters for assertions.
@addaleax
Copy link
Member Author

addaleax commented Jun 3, 2017

Rebased. @refack I think the “a day or two before landing” have passed by now, are you okay with reviewing this later and fixing up anything you’d like to see addressed yourself?

CI: https://ci.nodejs.org/job/node-test-commit/10331/

@refack
Copy link
Contributor

refack commented Jun 3, 2017

Rebased. @refack I think the “a day or two before landing” have passed by now, are you okay with reviewing this later and fixing up anything you’d like to see addressed yourself?

CI: https://ci.nodejs.org/job/node-test-commit/10331/

Yep.

@addaleax
Copy link
Member Author

addaleax commented Jun 3, 2017

Landed in 1dc3272

@addaleax addaleax closed this Jun 3, 2017
@addaleax addaleax deleted the async-hooks-test-message branch June 3, 2017 21:11
addaleax added a commit that referenced this pull request Jun 3, 2017
Improve error messages in the async hooks tests, mostly by removing
unhelpful `message` parameters for assertions.

PR-URL: #13243
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
jasnell pushed a commit that referenced this pull request Jun 5, 2017
Improve error messages in the async hooks tests, mostly by removing
unhelpful `message` parameters for assertions.

PR-URL: #13243
Reviewed-By: Kunal Pathak <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
@jasnell jasnell mentioned this pull request Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants