Skip to content

test: increase coverage for URL.searchParams #10952

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
Closed
wants to merge 2 commits into from
Closed

test: increase coverage for URL.searchParams #10952

wants to merge 2 commits into from

Conversation

@hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 22, 2017

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

@cjihrig cjihrig Jan 22, 2017

Choose a reason for hiding this comment

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

Might be worth checking done here too.

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

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

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

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

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

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

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

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

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

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

test/parallel/test-whatwg-url-searchparams-foreach.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 22, 2017

Choose a reason for hiding this comment

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

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

TimothyGu commented Jan 22, 2017

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

@hiroppy
Copy link
Member Author

hiroppy commented Jan 22, 2017 •
edited
Loading

@TimothyGu Thank you for reviews. PTAL 9c326fe

EDIT: I'll squash.

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

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

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

test/parallel/test-whatwg-url-searchparams-entries.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 28, 2017

Choose a reason for hiding this comment

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

This is also not needed any more after 326e967.

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

hiroppy commented Jan 29, 2017

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

TimothyGu
TimothyGu reviewed Jan 29, 2017
View reviewed changes
test/parallel/test-whatwg-url-searchparams-keys.js Outdated
Copy link
Member

@TimothyGu TimothyGu Jan 29, 2017

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

@hiroppy hiroppy Jan 29, 2017

Choose a reason for hiding this comment

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

Thanks, I forgot to change it.

@hiroppy
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 hiroppy force-pushed the feature/add-searchparams-tests branch from 0626657 to 189de74 Compare January 29, 2017 08:06
@TimothyGu
Copy link
Member

TimothyGu commented Jan 29, 2017

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

@TimothyGu
Copy link
Member

TimothyGu commented Jan 29, 2017

Landed in e7f4825.

@TimothyGu TimothyGu closed this Jan 29, 2017
TimothyGu pushed a commit that referenced this pull request Jan 29, 2017
@hiroppy @TimothyGu
test: add tests for searchParams
e7f4825 
PR-URL: #10952
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@hiroppy 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
@TimothyGu
review: fix Metadata state operation
3939f18 
E.g. nodejs/node#10952
@TimothyGu TimothyGu mentioned this pull request Jan 29, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@hiroppy @evanlucas
test: add tests for searchParams
fcd08b8 
PR-URL: #10952
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@italoacasas 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
@TimothyGu @evanlucas
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+1 more reviewer

@TimothyGu 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

@hiroppy @TimothyGu @jasnell @cjihrig @joyeecheung @mscdex @nodejs-github-bot