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

improve message for test-domain-http-server failure cases to include actual value #16048

Closed
wants to merge 2 commits into from
Closed

Conversation

twk-b
Copy link
Contributor

@twk-b twk-b commented Oct 6, 2017

improve assert failure message in parallel/test-domain-http-server failure cases to include actual value with more meaningufl message

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the domain Issues and PRs related to the domain subsystem. label Oct 6, 2017
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Do we want to land this? I prefer the plain message from strictEqual as the value would already be in the returned error message here.

@gibfahn
Copy link
Member

gibfahn commented Oct 7, 2017

I think the changes here make things less clear. I didn't see the original issue, but if you could undo the existing changes and then remove the 'Data should not be undefined' message on Line 54 I think that would be a really useful change.

assert.notStrictEqual(data, undefined, 'Data should not be undefined');

@twk-b if you could push a commit to your branch that makes this change then that would be good.

@joyeecheung
Copy link
Member

@BridgeAR Yeah...I kind of prefer that as well. This task is probably not a suitable one. Sorry about the confusion @twk-b

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but this will need line wrapping to pass linting before land

@twk-b
Copy link
Contributor Author

twk-b commented Oct 10, 2017 via email

@tniessen
Copy link
Member

I would prefer to follow up on #16048 (comment).

@tniessen
Copy link
Member

@gibfahn PTAL

@tniessen tniessen self-assigned this Oct 15, 2017
@tniessen
Copy link
Member

@twk-b You might want to force-push with your name and email address set correctly to an email address associated with your GitHub account. This is not strictly necessary, but the commits will not be associated with your account otherwise. You can view the data of a commit of your branch using git show --format=fuller.

@tniessen
Copy link
Member

failure cases to include actual value with more meaningufl message
@twk-b
Copy link
Contributor Author

twk-b commented Oct 15, 2017 via email

tniessen pushed a commit that referenced this pull request Oct 15, 2017
PR-URL: #16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@tniessen
Copy link
Member

Landed in 018375c, thank you for your contribution! 🎉

hopefully those are the right steps.. --amend push --force sounds evil...

Yes, everything seems to be correct now, thanks for fixing this!

@tniessen tniessen closed this Oct 15, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
PR-URL: #16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
PR-URL: #16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
PR-URL: #16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
PR-URL: #16048
Reviewed-By: James Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.