Skip to content

test: qs.escape with multibyte characters under 0x800#11251

Closed
watilde wants to merge 1 commit intonodejs:masterfrom
watilde:test-qs-escape
Closed

test: qs.escape with multibyte characters under 0x800#11251
watilde wants to merge 1 commit intonodejs:masterfrom
watilde:test-qs-escape

Conversation

@watilde
Copy link
Member

@watilde watilde commented Feb 9, 2017

Improve test coverage of querystring:

This test case will cover these lines:

  • node/lib/querystring.js

    Lines 169 to 173 in fcedd71

    if (c < 0x800) {
    lastPos = i + 1;
    out += hexTable[0xC0 | (c >> 6)] + hexTable[0x80 | (c & 0x3F)];
    continue;
    }
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 Feb 9, 2017
@hiroppy hiroppy added the querystring Issues and PRs related to the built-in querystring module. label Feb 9, 2017
bnoordhuis
bnoordhuis approved these changes Feb 9, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but can you trim the commit log to <= 50 characters? Thanks.

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

@watilde
test: querystring.escape with multibyte characters
8bb75cb 
Add a test case for querystring.parse with multibyte characters
under 0x800.

PR-URL: nodejs#11251
@watilde
Copy link
Member Author

watilde commented Feb 9, 2017

@bnoordhuis Sure thing! Done :)

@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

Landed in 81ef56b

@jasnell jasnell closed this Feb 11, 2017
@watilde watilde deleted the test-qs-escape branch February 11, 2017 18:28
@italoacasas italoacasas mentioned this pull request Feb 13, 2017
Merged
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

would need a backport PR to land on v4

@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

5 participants

@watilde @jasnell @bnoordhuis @hiroppy @nodejs-github-bot