Skip to content

test: cover deprecation warnings for res.redirect - #7571

Closed
saiyam-jain-dev wants to merge 1 commit into
expressjs:masterfrom
saiyam-jain-dev:test/redirect-deprecation-warnings
Closed

saiyam-jain-dev wants to merge 1 commit into
expressjs:masterfrom
saiyam-jain-dev:test/redirect-deprecation-warnings

Conversation

@saiyam-jain-dev

@saiyam-jain-dev saiyam-jain-dev commented Oct 28, 2026 •

Copy link
Copy Markdown

Add regression test coverage in test/res.redirect.js for the deprecation warnings emitted by res.redirect() when called with missing or invalid arguments:

  • Omitted url argument emits Provide a url argument and Url must be a string
  • Non-string url argument emits Url must be a string
  • Non-number status code argument emits Status must be a number

The tests attach a listener to process.on('deprecation', ...) to verify that depd emits the expected warning messages and clean up the listener in the completion callback.

All 1,293 tests pass (npm test), line coverage across lib/response.js is 100%, and npm run lint passes with zero errors.

@yxshee yxshee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ran this branch: 1293 passing (master is 1290), and the three new tests pass on their own as well.

One suggestion for the third test: res.redirect('301', url) throws in 5.x (TypeError: Invalid status code: "301". Status code must be an integer. from res.status), and the empty catch swallows it. As written, the test would still pass if redirect started throwing something else, or stopped throwing and redirected with a string status. Asserting the error explicitly would pin both halves of the behavior:

app.use(function (req, res) {
  assert.throws(function () {
    res.redirect('301', 'http://google.com')
  }, TypeError)
  res.end()
})

Also a small point: depd reports each deprecation once per call site, so these tests stay reliable as long as each res.redirect(...) call keeps its own line. A one-line comment saying so would stop someone from refactoring them into a shared helper and breaking the assertions.

Signed-off-by: Saiyam Jain <jimisierra2@gmail.com>
@saiyam-jain-dev
saiyam-jain-dev force-pushed the test/redirect-deprecation-warnings branch from 0176cea to e3f5eed Compare November 5, 2026 18:24
@bjohansebas

Copy link
Copy Markdown
Member

Please don’t open PRs to address that deprecation.

@bjohansebas bjohansebas closed this Nov 6, 2026
@saiyam-jain-dev

Copy link
Copy Markdown
Author

Hi @bjohansebas, thanks for the note! Just to clarify, this PR was not attempting to change or resolve the deprecation behavior; it was only adding test coverage for the existing warning emissions. Completely understand if tests for temporary deprecations are not needed right now though. Thanks for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants