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

test: increase coverage for URL.searchParams #10952

Closed
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
assert.strictEqual(typeof values[Symbol.iterator], 'function');
assert.strictEqual(values.next().value, 'b');
assert.strictEqual(values.next().value, 'd');
assert.strictEqual(values.next().value, undefined);
Copy link
Contributor

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


params = new URLSearchParams('a=b&c=d');
const entries = params.entries();
assert.strictEqual(typeof entries[Symbol.iterator], 'function');
Copy link
Member

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.

assert.deepStrictEqual(entries.next(), {
value: undefined,
done: true
});
Copy link
Member

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).


assert.throws(() => {
params.entries.call(undefined);
}, /^TypeError: Value of `this` is not a URLSearchParams$/);
Copy link
Member

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.

}, /^TypeError: Value of `this` is not a URLSearchParams$/);
assert.throws(() => {
params.forEach();
}, /^TypeError: The `callback` argument needs to be specified$/);
Copy link
Member

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

@hiroppy
Copy link
Member Author

hiroppy commented Jan 22, 2017

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

let params = m.searchParams;

// Until we export URLSearchParams
const URLSearchParams = params.constructor;
Copy link
Member

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.

let params = m.searchParams;

// Until we export URLSearchParams
const URLSearchParams = params.constructor;
Copy link
Member

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

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.

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

Landed in e7f4825.

@TimothyGu TimothyGu closed this Jan 29, 2017
TimothyGu pushed a commit that referenced this pull request Jan 29, 2017
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
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
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
evanlucas pushed a commit to nodejs/node-review that referenced this pull request Feb 6, 2017
* 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
Labels
test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants