test: add cases for unescape & unescapeBuffer by watilde · Pull Request #11326 · nodejs/node

watilde · 2017-02-12T19:49:28Z

Increase coverage of querystring:

https://coverage.nodejs.org/coverage-4cafa60c99f0d755/root/querystring.js.html

This test case will cover these lines:

node/lib/querystring.js

Line 114 in 4cafa60

return QueryString.unescapeBuffer(s, decodeSpaces).toString();
node/lib/querystring.js

Line 462 in 4cafa60

return QueryString.unescape(s, true);

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, querystring

TimothyGu · 2017-02-12T21:58:50Z

test/parallel/test-querystring.js

I'd prefer having more direct tests against qs.unescapeBuffer() in addition to the indirect ones for qs.unescape(). Here are some nice test cases for unescapeBuffer().

Thanks for pointing them! I've picked them and updated the test.

hiroppy · 2017-02-12T23:39:13Z

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

jasnell · 2017-02-16T22:52:46Z

@TimothyGu ... PTAL

TimothyGu · 2017-02-17T05:43:55Z

@watilde, sorry for not being clear. What I meant is calling qs.unescapeBuffer() directly, i.e. not though qs.unescape().

watilde · 2017-02-17T18:11:05Z

@TimothyGu To test this line, I needed calling qs.unescape() instead of qs.unescapeBuffer() to make an error by passing an option { decodeURIComponent: errDecode }.

TimothyGu · 2017-02-17T20:13:49Z

@watilde, how about both?

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught.

watilde · 2017-02-17T20:34:19Z

@TimothyGu Ahh that's nice! I will add it to call the both methods.

watilde · 2017-02-17T20:41:50Z

Updates:

Added a line to test qs.unescapeBuffer(): https://github.com/watilde/node/blob/9457020c0f3943bc7b02dfcaf34e99d8278dc554/test/parallel/test-querystring.js#L364
git rebase && make test

mhdawson

LGTM

mhdawson · 2017-02-17T22:32:07Z

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

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught. PR-URL: #11326 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

hiroppy · 2017-02-25T16:04:06Z

landed in 02acea9

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught. PR-URL: nodejs#11326 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught. PR-URL: #11326 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2017-03-07T05:39:24Z

This will need a backport PR if it needs to land on v4.x-staging

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught. PR-URL: #11326 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 12, 2017

mscdex added the querystring Issues and PRs related to the built-in querystring module. label Feb 12, 2017

jasnell approved these changes Feb 12, 2017

View reviewed changes

TimothyGu suggested changes Feb 12, 2017

View reviewed changes

watilde mentioned this pull request Feb 12, 2017

test: cases to querystring related to empty string #11329

Closed

3 tasks

hiroppy approved these changes Feb 12, 2017

View reviewed changes

test: add cases for unescape & unescapeBuffer

9457020

These two functions in the querystring are used as a fallback. To test them, two test cases were added which make errors that will be caught.

watilde force-pushed the test-qs branch from 4e39162 to 9457020 Compare February 17, 2017 20:39

mhdawson approved these changes Feb 17, 2017

View reviewed changes

TimothyGu approved these changes Feb 17, 2017

View reviewed changes

cjihrig approved these changes Feb 18, 2017

View reviewed changes

hiroppy closed this Feb 25, 2017

watilde deleted the test-qs branch February 25, 2017 16:08

italoacasas mentioned this pull request Feb 25, 2017

7.7.0 Proposal #11553

Merged

jasnell added the dont-land-on-v4.x label Mar 7, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

Uh oh!

Conversation

watilde commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

watilde Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

hiroppy commented Feb 12, 2017

Uh oh!

jasnell commented Feb 16, 2017

Uh oh!

TimothyGu commented Feb 17, 2017

Uh oh!

watilde commented Feb 17, 2017

Uh oh!

TimothyGu commented Feb 17, 2017

Uh oh!

watilde commented Feb 17, 2017

Uh oh!

watilde commented Feb 17, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Feb 17, 2017

Uh oh!

hiroppy commented Feb 25, 2017

Uh oh!

jasnell commented Mar 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

watilde commented Feb 12, 2017 •

edited

Loading