test: Bring common.js up to 100% coverage #1583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Investigated possible, legit argument values. Adjusted the function to have clear defaults and remove unreachable code. Note that the only two existing tests both passed non-string values for the `body` arg when function is only ever explicitly passed a string.
While it’s not currently possible for this util to get invalid args from current callers, type checks were added per the TODO comment. Refactored the “meat” of the logic to use native JS. Added tests for invalid arg flows as well as verifying duplicate field removal.
tests/test_common.js
Outdated
| path: '/path/1', | ||
| headers: { cookie: 'fiz=baz' }, | ||
| } | ||
| test('stringifyRequest', async t => { |
There was a problem hiding this comment.
At @gr2m's suggestion I've been trying to keep the tap subtest trees flat as much as possible. Do you mind updating these?
Also if the functions under test aren't async, I think it's clearer to make the test functions synchronous and use t.end().
There was a problem hiding this comment.
These have been flattened
| throw Error('headers must be an object') | ||
| } | ||
|
|
||
| if (!_.isString(fieldNameToDelete)) { |
There was a problem hiding this comment.
Could you perform this type check using plain JS?
There was a problem hiding this comment.
I can.
I'm still not sure when we want to be using Lodash or not.
For this particular case, because _.isString is technically more robust than typeof var === 'string', I thought it would be a good candidate to keep using Lodash.
There was a problem hiding this comment.
I'm fine with that for now. Ultimately I'd lean toward removing it entirely, though type checking is an area where it's not clear to me whether we need something more robust than typeof.
There was a problem hiding this comment.
The issue with string is
var stringValue = "This is a String";
var stringObject = new String("This is a String Object");
typeof stringValue === "string"
typeof stringObject === "object"The "proper" native js check would be
typeof value === 'string' || value instanceof String| // Search through the headers and delete all values whose field name matches the given field name. | ||
| Object.keys(headers) | ||
| .filter(fieldName => fieldName.toLowerCase() === lowerCaseFieldNameToDelete) | ||
| .forEach(fieldName => delete headers[fieldName]) |
paulmelnikow
approved these changes
Jun 18, 2019
tests/test_common.js
Outdated
| @@ -339,3 +448,7 @@ test('headersArrayToObject', function(t) { | |||
|
|
|||
| t.end() | |||
| }) | |||
|
|
|||
| test('percentEncode encodes extra reserved characters', async t => { | |||
There was a problem hiding this comment.
This one needs updating too.
|
🎉 This PR is included in version 11.0.0-beta.20 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
gr2m
pushed a commit
that referenced
this pull request
Sep 4, 2019
#### Refactor and add tests for `stringifyRequest` Investigated possible, legit argument values. Adjusted the function to have clear defaults and remove unreachable code. Note that the only two existing tests both passed non-string values for the `body` arg when function is only ever explicitly passed a string. #### Refactor and add tests for `deleteHeadersField` While it’s not currently possible for this util to get invalid args from current callers, type checks were added per the TODO comment. Refactored the “meat” of the logic to use native JS. Added tests for invalid arg flows as well as verifying duplicate field removal. #### Add test for `percentEncode` The new test covers cases of characters that are reserved in RFC 3986 but not encoded by `encodeURIComponent`. Added JSDoc with info and refs. Further discussion can be found on the original issue #416 and the commit that added the logic 9bd8946
gr2m
pushed a commit
that referenced
this pull request
Sep 4, 2019
#### Refactor and add tests for `stringifyRequest` Investigated possible, legit argument values. Adjusted the function to have clear defaults and remove unreachable code. Note that the only two existing tests both passed non-string values for the `body` arg when function is only ever explicitly passed a string. #### Refactor and add tests for `deleteHeadersField` While it’s not currently possible for this util to get invalid args from current callers, type checks were added per the TODO comment. Refactored the “meat” of the logic to use native JS. Added tests for invalid arg flows as well as verifying duplicate field removal. #### Add test for `percentEncode` The new test covers cases of characters that are reserved in RFC 3986 but not encoded by `encodeURIComponent`. Added JSDoc with info and refs. Further discussion can be found on the original issue #416 and the commit that added the logic 9bd8946
gr2m
pushed a commit
that referenced
this pull request
Sep 5, 2019
#### Refactor and add tests for `stringifyRequest` Investigated possible, legit argument values. Adjusted the function to have clear defaults and remove unreachable code. Note that the only two existing tests both passed non-string values for the `body` arg when function is only ever explicitly passed a string. #### Refactor and add tests for `deleteHeadersField` While it’s not currently possible for this util to get invalid args from current callers, type checks were added per the TODO comment. Refactored the “meat” of the logic to use native JS. Added tests for invalid arg flows as well as verifying duplicate field removal. #### Add test for `percentEncode` The new test covers cases of characters that are reserved in RFC 3986 but not encoded by `encodeURIComponent`. Added JSDoc with info and refs. Further discussion can be found on the original issue #416 and the commit that added the logic 9bd8946
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #1404
The changes for each of the three functions that were tested and/or refactored are broken up into separate commits to ease review.