test: add test-benchmark-http2 #23863
Conversation
nodejs-github-bot
added
benchmark
|
Has h2load been installed on all of the CI machines? |
I doubt it. This PR uses a test double instead. |
|
@nodejs/testing |
| client.on('error', (e) => { throw e; }); | ||
| const req = client.request({ ':path': '/' }); | ||
| request(req, client); | ||
| req.end(); |
There was a problem hiding this comment.
nit: ':path' defaults to '/' and, when using a GET request, the req.end() is unnecessary because http2 will automatically end()... so this can be simplified to:
request(client.request(), client);There was a problem hiding this comment.
Great, thanks, will change.
|
|
||
| const duration = process.env.duration || 0; | ||
| const url = process.env.test_url; | ||
|
|
||
| const start = process.hrtime(); | ||
| let throughput = 0; | ||
|
|
||
| function request(res) { | ||
| function request(res, client) { | ||
| res.on('data', () => {}); |
There was a problem hiding this comment.
| res.on('data', () => {}); | |
| res.resume(); |
Trott
force-pushed
the
test-benchmark-http2
branch
from
5a73395 to
fdc18b3
Compare
| const common = require('../common'); | ||
|
|
||
| if (!common.enoughTestMem) | ||
| common.skip('Insufficient memory for HTTP benchmark test'); |
There was a problem hiding this comment.
Changed to HTTP/2. Thanks!
|
Pushed a new commit, so... CI: https://ci.nodejs.org/job/node-test-pull-request/18164/ |
|
Needs another review... /cc @joyeecheung (primary author of the existing HTTP benchmark test double) |
| } | ||
| }); | ||
| } | ||
|
|
||
| function run() { | ||
| http.get(url, request); | ||
| if (http.get) { |
There was a problem hiding this comment.
nit
| if (http.get) { | |
| if (myModule === 'http') { |
makes is clear what we're benching
There was a problem hiding this comment.
I added a comment indicating it's 'HTTP' for now. Hope that's sufficient. (If we ever need a test double for https, only the comment will need updating and not the code.)
Trott
force-pushed
the
test-benchmark-http2
branch
from
ab76e5e to
61c98d6
Compare
|
Landed in 2812759 |
PR-URL: nodejs#23863 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #23863 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
this requires a manual backport to 8.x and 10.x should it be backported? |
Low value. I'd say no. |
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes