Skip to content

Refactor request tests for clarity and structure #7447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
uttammaji wants to merge 1 commit into from
Closed

Refactor request tests for clarity and structure #7447

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
104 changes: 84 additions & 20 deletions test/app.request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
'use strict'

var after = require('after')

Check failure on line 3 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

'after' is assigned a value but never used
var express = require('../')
, request = require('supertest');
var request = require('supertest');

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change


describe('app', function(){
describe('.request', function(){
Expand All @@ -18,15 +18,15 @@
});

request(app)
.get('/foo?name=tobi')
.expect('name=tobi', done);
.get('/foo?name=tobi')
.expect(200)
.expect('name=tobi', done);
Comment on lines +22 to +23

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The .expect(200) is ok, but not really necessary. If Express changed the default status code other tests would also catch it.

Otherwise it's just a formatting change.

})

it('should only extend for the referenced app', function (done) {
var app1 = express()
var app2 = express()
var cb = after(2, done)


Check failure on line 29 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Trailing whitespace

app1.request.foobar = function () {
return 'tobi'
}
Expand All @@ -36,23 +36,50 @@
})

app2.get('/', function (req, res) {
res.send(req.foobar())
// This should fail because foobar doesn't exist on app2's request
try {
res.send(req.foobar())
} catch (err) {
res.status(500).send(err.message)
}
Comment on lines +39 to +44

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, it should fail and Express should automatically do what was added here.

The try/catch is completely unnecessary, because Express (or router in 5.x) catches errors and forwards them to the error handling middleware, which in this case is the default error handler:

express/lib/application.js

Lines 153 to 157 in 023767f

// final handler
var done = callback || finalhandler(req, res, {
env: this.get('env'),
onerror: logerror.bind(this)
});

See also finalhandler

})

var completed = 0
var total = 2

function checkDone() {
completed++
if (completed === total) {
done()
}
}

Comment on lines +47 to +56

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is just after(2, done), but worse.

See also after

request(app1)
.get('/')
.expect(200, 'tobi', cb)
.expect(200)
.expect('tobi', checkDone)
Comment on lines +59 to +60

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change.

See also supertest docs


request(app2)
.get('/')
.expect(500, /(?:not a function|has no method)/, cb)
.expect(500)
.expect(function(res) {
// Check that the error indicates foobar is not a function
var errorMsg = res.text || res.body
if (typeof errorMsg === 'string') {
if (!errorMsg.includes('not a function') &&

Check failure on line 69 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed
!errorMsg.includes('has no method') &&
!errorMsg.includes('is not a function')) {
throw new Error('Expected error message about missing function, got: ' + errorMsg)
}
}
})
Comment on lines +65 to +75

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nothing was wrong with the previous check. I have no idea what this is supposed to do.

.end(checkDone)
})

it('should inherit to sub apps', function (done) {
var app1 = express()
var app2 = express()
var cb = after(2, done)


Check failure on line 82 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

trailing whitespace

app1.request.foobar = function () {
return 'tobi'
}
Expand All @@ -67,20 +94,32 @@
res.send(req.foobar())
})

var completed = 0
var total = 2

function checkDone(err) {
if (err) return done(err)
completed++
if (completed === total) {
done()
}
}

Comment on lines +97 to +107

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

after(2, done), but worse - can call done() multiple times

request(app1)
.get('/')
.expect(200, 'tobi', cb)
.expect(200)
.expect('tobi', checkDone)
Comment on lines +110 to +111

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change


request(app1)
.get('/sub')
.expect(200, 'tobi', cb)
.expect(200)
.expect('tobi', checkDone)
Comment on lines +115 to +116

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change

})

it('should allow sub app to override', function (done) {
var app1 = express()
var app2 = express()
var cb = after(2, done)


Check failure on line 122 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

trailing whitespace

app1.request.foobar = function () {
return 'tobi'
}
Expand All @@ -99,20 +138,32 @@
res.send(req.foobar())
})

var completed = 0
var total = 2

function checkDone(err) {
if (err) return done(err)
completed++
if (completed === total) {
done()
}
}

Comment on lines +141 to +151

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

after(2, done), but worse

request(app1)
.get('/')
.expect(200, 'tobi', cb)
.expect(200)
.expect('tobi', checkDone)
Comment on lines +154 to +155

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change


request(app1)
.get('/sub')
.expect(200, 'loki', cb)
.expect(200)
.expect('loki', checkDone)
Comment on lines +159 to +160

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Style only change

})

it('should not pollute parent app', function (done) {
var app1 = express()
var app2 = express()
var cb = after(2, done)


Check failure on line 166 in test/app.request.js

View workflow job for this annotation

GitHub Actions / Lint 

Trailing spaces not allowed

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

trailing whitespace

app1.request.foobar = function () {
return 'tobi'
}
Expand All @@ -131,13 +182,26 @@
res.send(req.foobar())
})

var completed = 0
var total = 2

function checkDone(err) {
if (err) return done(err)
completed++
if (completed === total) {
done()
}
}

Comment on lines +185 to +195

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

after(2, done), but worse

request(app1)
.get('/sub')
.expect(200, 'loki', cb)
.expect(200)
.expect('loki', checkDone)
Comment on lines +198 to +199

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

style only change


request(app1)
.get('/sub/foo')
.expect(200, 'tobi', cb)
.expect(200)
.expect('tobi', checkDone)
Comment on lines +203 to +204

@krzysdz krzysdz Sep 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

style only change

})
})
})
Loading