test: cover req.is() array argument and res.redirect() misuse branches - #7405
Closed
arjun2075 wants to merge 1 commit into
Closed
test: cover req.is() array argument and res.redirect() misuse branches#7405arjun2075 wants to merge 1 commit into
arjun2075 wants to merge 1 commit into
Conversation
Two branches were unexercised by the existing suite: - lib/request.js: req.is() only had tests passing a bare string, so the !Array.isArray(types) branch never took its false path. - lib/response.js: res.redirect()'s deprecation guards for a missing or non-string address were never triggered. Adds: - req.is.js: req.is() given an array of types (matching type, no match) - res.redirect.js: res.redirect() with no address, and with a non-string address request.js reaches 100% branch coverage. response.js improves from 93.71% to 94.68% branch coverage; the remaining uncovered branch (non-number status) can't be exercised via a successful redirect, since res.status() throws synchronously on any non-integer value -- noted in the PR description rather than forcing a test around it.
Author
1 similar comment
Author
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Two branches were unexercised by the existing test suite:
lib/request.js—req.is()was only ever called with a bare string in tests, so!Array.isArray(types)never took itsfalsepath (line 273).lib/response.js—res.redirect()'s deprecation guards for a missing address (!address) and a non-string address were never triggered (lines 827, 831).Changes
test/req.is.js—req.is()given an array of types (matching, and no match)test/res.redirect.js—res.redirect()called with no address, and with a non-string addressCoverage (measured locally with
npm run test-cov)lib/request.jslib/response.jsLine 835 (
res.redirect()called with a non-numberstatus) is not covered here: any non-integer status value is rejected byres.status()later in the same function, so there's no way to exercise that deprecation branch through a request that completes successfully. Flagging it in case it's worth a follow-up (either loosening the validation order, or accepting it stays untested).Testing
Ran the full suite locally (
npm test) multiple times with this change — passes consistently. Note: I also observed an intermittent, pre-existing"WebSockets request was expected"failure inres.render.jsonmaster(~1 in 10 runs, unrelated to this change, appears to be a port-reuse race between adjacentsupertestservers). Happy to open a separate issue for that if useful — didn't want to scope-creep this PR.