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

test: increase coverage for URL.searchParams by hiroppy · Pull Request #10952 · nodejs/node · GitHub

/ node Public

test: increase coverage for URL.searchParams - #10952

Closed
hiroppy wants to merge 2 commits into
nodejs:masterfrom
hiroppy:feature/add-searchparams-tests
ConversationCommits (2)ChecksFiles changed
Closed

test: increase coverage for URL.searchParams#10952
hiroppy wants to merge 2 commits into
nodejs:masterfrom
hiroppy:feature/add-searchparams-tests

Conversation

hiroppy commented Jan 22, 2017

Copy link
Copy Markdown
Member

Add exceptions for all cases.
Add entries, keys and values files.(Validation tests and exception tests are included.)

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 added the test Issues and PRs related to the tests. label Jan 22, 2017
hiroppy force-pushed the feature/add-searchparams-tests branch 2 times, most recently from 8d0ef6f to c7f159a Compare January 22, 2017 16:22
mscdex added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 22, 2017
cjihrig reviewed Jan 22, 2017
View reviewed changes
Comment thread test/parallel/test-whatwg-url-searchparams-values.js Outdated

cjihrig Jan 22, 2017

Copy link
Copy Markdown
Contributor

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

Might be worth checking done here too.

hiroppy force-pushed the feature/add-searchparams-tests branch from c7f159a to 91ec5aa Compare January 22, 2017 21:28
TimothyGu reviewed Jan 22, 2017
View reviewed changes

TimothyGu left a comment

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

Most of these tests were actually derived from W3C's web-platform-tests, so I'm sure they would appreciate these changes as well.

Comment thread test/parallel/test-whatwg-url-searchparams-entries.js Outdated

TimothyGu Jan 22, 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

Also useful might be making sure entries[Symbol.iterator]() === entries.

Comment thread test/parallel/test-whatwg-url-searchparams-entries.js Outdated

TimothyGu Jan 22, 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

You may also test calling entries.next again (to simulate overread).

Comment thread test/parallel/test-whatwg-url-searchparams-entries.js Outdated

TimothyGu Jan 22, 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

entries.next.call(undefined) should throw as well.

Comment thread test/parallel/test-whatwg-url-searchparams-foreach.js Outdated

TimothyGu Jan 22, 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 test seems to overlap with a series of similar, but more comprehensive tests in #10905. I'm fine with keeping it in this PR, but I'd prefer just dropping it here.

TimothyGu commented Jan 22, 2017

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/5984/

hiroppy commented Jan 22, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@TimothyGu Thank you for reviews. PTAL 9c326fe

EDIT: I'll squash.

hiroppy force-pushed the feature/add-searchparams-tests branch from 9c326fe to b031c07 Compare January 22, 2017 22:46
TimothyGu suggested changes Jan 28, 2017
View reviewed changes

TimothyGu left a comment

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

@abouthiroppy, this PR seems to stop working after ed0086f. Can you please rebase and make sure it works on master? Thanks.

Comment thread test/parallel/test-whatwg-url-searchparams-entries.js Outdated

TimothyGu Jan 28, 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 is also not needed any more after 326e967.

hiroppy force-pushed the feature/add-searchparams-tests branch from b031c07 to 0626657 Compare January 29, 2017 03:08

hiroppy commented Jan 29, 2017

Copy link
Copy Markdown
Member Author

@TimothyGu I rebased and confirmed that the test passed. Thanks.

TimothyGu reviewed Jan 29, 2017
View reviewed changes
Comment thread test/parallel/test-whatwg-url-searchparams-keys.js Outdated

TimothyGu Jan 29, 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

Actually do you mind fixing this and in -values.js as well?

const URLSearchParams = require('url').URLSearchParams;

hiroppy Jan 29, 2017

Copy link
Copy Markdown
Member 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

Thanks, I forgot to change it.

test: add tests of entries, keys, values for searchParams
189de74 
Improve coverage for entries, keys and values.
Validation tests and exception tests are included.
hiroppy force-pushed the feature/add-searchparams-tests branch from 0626657 to 189de74 Compare January 29, 2017 08:06

TimothyGu commented Jan 29, 2017

Copy link
Copy Markdown
Member

Updated CI: https://ci.nodejs.org/job/node-test-pull-request/6094/

TimothyGu commented Jan 29, 2017

Copy link
Copy Markdown
Member

Landed in e7f4825.

TimothyGu closed this Jan 29, 2017
TimothyGu pushed a commit that referenced this pull request Jan 29, 2017
test: add tests for searchParams
e7f4825 
PR-URL: #10952
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
hiroppy deleted the feature/add-searchparams-tests branch January 29, 2017 09:53
TimothyGu added a commit to TimothyGu/node-review that referenced this pull request Jan 29, 2017
review: fix Metadata state operation
3939f18 
E.g. nodejs/node#10952
TimothyGu mentioned this pull request Jan 29, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
test: add tests for searchParams
fcd08b8 
PR-URL: #10952
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas mentioned this pull request Jan 31, 2017
Merged
evanlucas pushed a commit to nodejs/node-review that referenced this pull request Feb 6, 2017
Improvements (#7)
0b323f0 
* index: allow #fragments in PR URLs

Also check tightened PR_RE against pathname

* review: fix Metadata state operation

E.g. nodejs/node#10952

* review: simplify Fixes creation

* review: overhaul getCollaborators()

- Make regex static and more concise
- Iterate over RE.exec
- Use Map

* review: remove extra whitespace

Fixes: #5

* review: only look for LGTMs in <p>'s

Fixes: nodejs/node#10657
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 approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
+1 more reviewer
TimothyGu TimothyGu approved these changes
Reviewers whose approvals may not affect merge requirements

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