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: add two test cases for querystring #11481

Closed
wants to merge 1 commit into from
Closed

test: add two test cases for querystring #11481

wants to merge 1 commit into from

Conversation

watilde
Copy link
Contributor

@watilde watilde commented Feb 21, 2017

This test will improve querystring coverage:

The following lines will be called with the cases:

Checklist
  • make -j4 test
  • tests 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 Feb 21, 2017
@mscdex mscdex added the querystring Issues and PRs related to the built-in querystring module. label Feb 21, 2017
@mscdex
Copy link
Contributor

mscdex commented Feb 21, 2017

I think just using 'test' for the subsystem in the first line of the commit is enough, as no change to the actual querystring module is being made in this PR.

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

@watilde
Copy link
Contributor Author

watilde commented Feb 21, 2017 •
edited
Loading

Ahh ok, now the tagging rule becomes apparent to me. Thanks!
I will update the message, and also probably I should squash the commits into one commit.

@watilde
test: add two test cases for querystring
6913df6 
+ Check an empty substring: In `querystring`, if the `maxKeys` is 1
  and the state machine finds an empty substring between separators,
  it should return an empty object.
+ Test invalid encoded strings: If provided string is an invalid
  encoded string in `query.parse`, it will not be encoded.
@mscdex mscdex changed the title test,querystring: improve querystring coverage to cover two more lines test: add two test cases for querystring Feb 21, 2017
jasnell pushed a commit that referenced this pull request Feb 24, 2017
@watilde @jasnell
test: add two test cases for querystring
dd2e135 
+ Check an empty substring: In `querystring`, if the `maxKeys` is 1
  and the state machine finds an empty substring between separators,
  it should return an empty object.
+ Test invalid encoded strings: If provided string is an invalid
  encoded string in `query.parse`, it will not be encoded.

PR-URL: #11481
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 24, 2017

@addaleax ... just fyi, I was landing this PR just as your review came in so you're not listed on the reviewers

@jasnell
Copy link
Member

jasnell commented Feb 24, 2017

Landed in dd2e135

@jasnell jasnell closed this Feb 24, 2017
@addaleax
Copy link
Member

addaleax commented Feb 24, 2017 •
edited
Loading

@jasnell guess what I was just going to push to master! 😄

@watilde watilde deleted the test-querystring branch February 24, 2017 01:12
@italoacasas
Copy link
Contributor

not landing in v7.x-staging at the moment because one test is failing. Any plan to backport?

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@watilde
Copy link
Contributor Author

watilde commented Jun 18, 2017

Since this update was related to the breaking change of the querystring in v8.x, I replaced backport label with dont-land-on. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
querystring Issues and PRs related to the built-in querystring module. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@watilde @mscdex @jasnell @addaleax @italoacasas @gibfahn @TimothyGu @cjihrig @nodejs-github-bot