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 |
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.
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
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) |
refack
left a comment
refack
left a comment
There was a problem hiding this comment.
Looks good, but left some nits
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. |
7 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes