FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test, url: updated assertions in url-searchparams-getall tests by IvanJov · Pull Request #11142 · nodejs/node · GitHub

/ node Public

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

Closed
IvanJov wants to merge 2 commits into
nodejs:masterfrom
IvanJov:url-searchparams
ConversationCommits (2)ChecksFiles changed
Closed

test, url: updated assertions in url-searchparams-getall tests#11142
IvanJov wants to merge 2 commits into
nodejs:masterfrom
IvanJov:url-searchparams

Conversation

IvanJov commented Feb 3, 2017

Copy link
Copy Markdown

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 added the test Issues and PRs related to the tests. label Feb 3, 2017
hiroppy added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 3, 2017
jasnell requested changes Feb 3, 2017
View reviewed changes
Comment thread test/parallel/test-whatwg-url-searchparams-getall.js Outdated
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"`);

jasnell Feb 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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"'.

IvanJov Feb 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed, tested with make lint.

joyeecheung Feb 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell Feb 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Still better to keep it consistent

Comment thread 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}`);

jasnell Feb 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please line wrap at 80 chars

IvanJov Feb 3, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

TimothyGu commented Feb 3, 2017

Copy link
Copy Markdown
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 commented Feb 3, 2017

Copy link
Copy Markdown
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 commented Feb 3, 2017

Copy link
Copy Markdown
Author

@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 mentioned this pull request Feb 3, 2017
Closed

joyeecheung commented Feb 3, 2017

Copy link
Copy Markdown
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 mentioned this pull request Feb 3, 2017
Closed
3 tasks

Trott commented Feb 3, 2017

Copy link
Copy Markdown
Member

@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 commented Feb 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Feb 3, 2017

Copy link
Copy Markdown
Author

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

jasnell commented Feb 3, 2017

Copy link
Copy Markdown
Member

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

jasnell closed this Feb 3, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 9, 2017
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
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
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
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell requested changes
joyeecheung joyeecheung left review comments

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

Back | FazBrowse Home | New Git URL