http2: add test for head request is not finished #24308
Conversation
ronag
force-pushed
the
fix-head-finished
branch
3 times, most recently
from
8cb5560
to
75d254a
Compare
ronag
changed the title
|
@ronag I’ve added the WIP label, feel free to ask for it to be removed if that’s not accurate. If you have a hard time fixing this, you can also consider moving this test to |
ronag
force-pushed
the
fix-head-finished
branch
from
75d254a
to
74b4cf6
Compare
|
@addaleax: Moved to known_issues. Ready for merge? |
|
@ronag This would still need to be reviewed – I didn’t take an in-depth look yet (i.e. I don’t know if the current behaviour might be okay, or how hard it would be to fix this). /cc @nodejs/http2 |
| .listen(0, common.mustCall(function() { | ||
| const port = server.address().port; | ||
| server.once('request', common.mustCall((req, res) => { | ||
| assert.ok(!res.finished); |
There was a problem hiding this comment.
This is the assertion that fails? Can you please add a comment?
ronag
force-pushed
the
fix-head-finished
branch
from
74b4cf6
to
1b4371e
Compare
|
@mcollina Comments added |
|
Sorry, I tried... it's a bit over my head unfortunately |
|
|
||
| const common = require('../common'); | ||
| if (!common.hasCrypto) | ||
| common.skip('missing crypto'); |
There was a problem hiding this comment.
@mcollina For future reference, what is the purpose of this? I just followed the other tests, not sure if or why it would be needed?
There was a problem hiding this comment.
@ronag The http2 module requires encryption, so if Node.js was compiled without crypto, then http2 tests need to be skipped.
|
Close in favour of #24339? |
Adds test for #24283. Not actually fixed. Just test.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes