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

Updated assertions in urlsearchparams-getall tests #4711

Closed

Conversation

IvanJov
Copy link

@IvanJov IvanJov commented Feb 3, 2017

Hi,

I've updated tests, made them nicer for debugging. Let me know what do you think.

Code was also sent as PR to Node repo: nodejs/node#11142

Thanks!

@wpt-pr-bot
Copy link
Collaborator

Notifying @Sebmaster, @domenic, @mikewest, @rubys, @sideshowbarker, @smola, @tomalec, @xiaojunwu, and @zcorpan. (Learn how reviewing works.)

These tests will be available on w3c-test.org as soon as they are approved by a repository collaborator.

@wpt-pr-bot wpt-pr-bot added the url label Feb 5, 2017
@annevk
Copy link
Member

annevk commented Feb 7, 2017

This looks reasonable to me, but I'm never quite sure whether the assertion description needs to describe the assertion or the unexpected result.

@gsnedders @jgraham? Seems that http://testthewebforward.org/docs/testharness-library.html#making-assertions should say this, no?

@IvanJov
Copy link
Author

IvanJov commented Feb 7, 2017

@annevk I think that it should describe why the test failed. This is what docs say: The description parameter is used to present more useful error messages when a test fails. 🙂

@domenic
Copy link
Member

domenic commented Feb 7, 2017

I've always gone with "should" or "must" in my assertion messages. So 'Search params object must have name "a"

@zcorpan
Copy link
Member

zcorpan commented Feb 10, 2017

w3c-test:mirror

@zcorpan
Copy link
Member

zcorpan commented Feb 10, 2017

For assert_equals I generally avoid saying what the expected value is in the message, since the message already contains that and it tends to get out of sync if the test is later updated to change what is expected.

@annevk
Copy link
Member

annevk commented Feb 17, 2017

@IvanJov it says to be useful when the test fails, it doesn't say describe when it fails. Given the feedback from others I guess we should either close this or simply drop these messages and rely on the default handling. Are you up for doing that?

@annevk
Copy link
Member

annevk commented Feb 28, 2017

Closing due to lack of feedback. Sorry it didn't work out.

@annevk annevk closed this Feb 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants