Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions test/app.response.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,5 +139,45 @@ describe('app', function(){
.get('/sub/foo')
.expect(200, 'FOO', cb)
})

it('should deprecate when redirect called without a url', function (done) {
var app = express()

app.use(function (req, res) {
res.redirect('')
})

request(app)
.get('/')
.expect(302)
.expect('Location', '')
.expect(/Redirecting to/, done)
})

it('should deprecate when redirect address is not a string', function (done) {
var app = express()

app.use(function (req, res) {
res.redirect(302, 123)
})

request(app)
.get('/')
.expect(302)
.expect('Location', '123')
.expect(/Redirecting to 123/, done)
})
Comment on lines +143 to +169
Copy link
Contributor

@krzysdz krzysdz Mar 15, 2026

Choose a reason for hiding this comment

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

How are you checking here for a deprecation warning? I don't see any code that would verify if there is a warning. These tests would also pass without #6405.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
You're correct that the tests only verify the deprecated behaviour, not the warnings themselves. However, since deprecation warnings for Express are suppressed in the test environment via [NO_DEPRECATION](to avoid noise), the tests focus on ensuring the deprecated code paths are executed, which is confirmed by coverage reports showing lines 824, 828, and 832 in lib/response.js are hit.

The tests would indeed pass without the deprecation calls if the behaviour remained the same, but since the goal is coverage, these tests ensure the deprecation logic runs. If you'd like explicit warning checks, we could modify the test environment to enable them, but that might introduce noise in CI. Let me know if you'd prefer that approach!

The changes ensure the targeted lines are covered while maintaining test stability.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just "covering" some lines without checking if they do what they are supposed to do does not make sense in my opinion.

The tests are named "should deprecate", so I consider not checking this behaviour a problem.


it('should raise when redirect status is not a number', function (done) {
var app = express()

app.use(function (req, res) {
res.redirect('foo', '/bar')
})

request(app)
.get('/')
.expect(500, /Invalid status code/, done)
})
})
})