Skip to content

Commit

Permalink
test: cases to querystring related to empty string
Browse files Browse the repository at this point in the history
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: #11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
  • Loading branch information
watilde authored and jasnell committed Feb 16, 2017
1 parent 5ddf722 commit eec216f
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions test/parallel/test-querystring.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,20 @@ assert.doesNotThrow(function() {
assert.strictEqual(f, 'a:b;q:x%3Ay%3By%3Az');
}

// empty string
assert.strictEqual(qs.stringify(), '');
assert.strictEqual(qs.stringify(0), '');
assert.strictEqual(qs.stringify([]), '');
assert.strictEqual(qs.stringify(null), '');
assert.strictEqual(qs.stringify(true), '');

check(qs.parse(), {});

// empty sep
check(qs.parse('a', []), { a: '' });

// empty eq
check(qs.parse('a', null, []), { '': 'a' });

// Test limiting
assert.strictEqual(
Expand Down

5 comments on commit eec216f

@Trott
Copy link
Member

@Trott Trott commented on eec216f Feb 17, 2017

Choose a reason for hiding this comment

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

Minor nit, no big deal, but for the future: first word of commit message after subsystem should be an imperative verb. This probably should have been something like test: add empty-string cases to querystring

@jasnell
Copy link
Member

Choose a reason for hiding this comment

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

oh, right, completely overlooked that... sorry about that

@Trott
Copy link
Member

@Trott Trott commented on eec216f Feb 17, 2017

Choose a reason for hiding this comment

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

oh, right, completely overlooked that... sorry about that

Kind of been wondering if we should make that a recommendation rather than a hard requirement anyway, to be honest.

@Trott
Copy link
Member

@Trott Trott commented on eec216f Feb 17, 2017

Choose a reason for hiding this comment

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

(And, actually, I see the doc says "should" and not "must" so I guess you could make the case it's already a recommendation but not a requirement. Thus ends my commit-comment storm. You may now go back to enjoying your day.)

@gibfahn
Copy link
Member

Choose a reason for hiding this comment

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

IMHO something with a verb like test: add empty string cases to querystring would have been clearer, it's not the end of the world, but looking at git log --oneline requires parsing a lot of very dense information, and every bit of readability helps.

Please sign in to comment.