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.

Sign up for GitHub

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

Jump to bottom

test, url: updated assertions in url-searchparams-getall tests #11142

Closed
wants to merge 2 commits into from
Closed

test, url: updated assertions in url-searchparams-getall tests #11142

wants to merge 2 commits into from

Conversation

IvanJov
Copy link

@IvanJov IvanJov commented Feb 3, 2017

I have updated assertions in url-searchparams-getall tests, made them more clear
for debugging and improved test messages.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

no

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 3, 2017
@hiroppy hiroppy added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 3, 2017
jasnell
jasnell requested changes Feb 3, 2017
View reviewed changes
test/parallel/test-whatwg-url-searchparams-getall.js Outdated
@@ -25,15 +25,17 @@ test(function() {

test(function() {
var params = new URLSearchParams('a=1&a=2&a=3&a');
assert_true(params.has('a'), 'Search params object has name "a"');
assert_true(params.has('a'), `Search params object doesn't have name "a"`);
Copy link
Member

Choose a reason for hiding this comment

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

This will not pass make lint given that it is a plain string without any replacements. Please simply avoid the use of the contraction and make it a normal string.. e.g. 'Search params object does not have name "a"'.

Copy link
Author

@IvanJov IvanJov Feb 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed, tested with make lint.

Copy link
Member

Choose a reason for hiding this comment

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

We disable eslint for WPT tests though, because the styles are too different.. :D

Copy link
Member

Choose a reason for hiding this comment

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

Still better to keep it consistent

test/parallel/test-whatwg-url-searchparams-getall.js Outdated
assert_true(matches && matches.length == 4, 'Search params object has values for name "a"');
assert_array_equals(matches, ['1', '2', '3', ''], 'Search params object has expected name "a" values');
assert(matches);
assert_equals(matches.length, 4, `Unexpected length of name "a" values in search params object: ${matches.length}`);
Copy link
Member

Choose a reason for hiding this comment

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

Please line wrap at 80 chars

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in separate commit, I can squash them at the end.

@TimothyGu
Copy link
Member

@IvanJov, this specific test was imported directly from web-platform-tests, and we don't make any downstream changes. Can you submit the PR there instead?

@joyeecheung
Copy link
Member

+1 on upstreaming the changes first. The tests are in their url directory, just send in a PR with the changes applied to a corresponding .html file would be enough I believe.

@IvanJov
Copy link
Author

IvanJov commented Feb 3, 2017

@TimothyGu @joyeecheung Ah sorry, didn't know that. I am updating code on web-platform-tests. Let's keep this PR opened, in case that they accept it.

What about other tests, can you tell me what tests are also imported from some other repo? 🙂

Thanks!

@IvanJov IvanJov mentioned this pull request Feb 3, 2017
Closed
@joyeecheung
Copy link
Member

Generally the imported tests should have a comment mentioning the source (I remember the old test-url-* has a few from an abandoned project), because attribution is required by a lot of licenses. I am not really sure about the history of other tests though. At the moment the WHATWG URL implementation is the only one in core that is really meant to be compatible with web standards I believe?

I'll add a note to guides/writing-tests.md about this.

@joyeecheung joyeecheung mentioned this pull request Feb 3, 2017
Closed
3 tasks
@Trott
Copy link
Member

Trott commented Feb 3, 2017

@IvanJov Thanks for taking the time to work on Node.js!

It looks like that test went through some significant modifications about a week after you and I discussed it. The part that I was referring to was removed in 10b687b and replaced with a port of the actual WPT test harness and tests.

Sorry about that, and I hope you don't feel discouraged!

If you want to try to find other places to contribute but you're not sure what to start with, http://nodetodo.org/next-steps/ might be helpful.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 3, 2017 •
edited
Loading

Ah, didn't know it's a NodeTodo PR :D Sorry about the confusion.

FWIW, I think the upstreaming process for WPT is not really a must, at least not as strict as we are about deps updates, it's just that having the same test suites is one of the points of implementing a web standard IMO, and it benefits other people using the tests, so why not :)

@IvanJov
Copy link
Author

IvanJov commented Feb 3, 2017

@Trott Thanks, I will find something else to work on! Thanks a lot for your help!

@jasnell
Copy link
Member

jasnell commented Feb 3, 2017

Given that, I'm going to close this. We can reopen if necessary after changes have been upstreamed.

@jasnell jasnell closed this Feb 3, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 9, 2017
@joyeecheung @italoacasas
doc: improve testing guide
3ce763f 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@joyeecheung @italoacasas
doc: improve testing guide
abae264 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
@joyeecheung @krydos
doc: improve testing guide
505f8d3 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: nodejs#11150
Ref: nodejs#11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@joyeecheung @jasnell
doc: improve testing guide
660137e 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
@joyeecheung @jasnell
doc: improve testing guide
3213768 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@joyeecheung @MylesBorins
doc: improve testing guide
3746eee 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@joyeecheung @MylesBorins
doc: improve testing guide
2b6ee39 
Add guide on choice of assertions, use of ES.Next features,
and the WPT upstream.

PR-URL: #11150
Ref: #11142
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@jasnell jasnell jasnell requested changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@IvanJov @TimothyGu @joyeecheung @Trott @jasnell @hiroppy @nodejs-github-bot