http2: add test for head request is not finished#24308
http2: add test for head request is not finished#24308ronag wants to merge 1 commit intonodejs:masterfrom
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 |
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 |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
LGTM. I would prefer that this included a fix, but that's ok too.
|
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? |
5 participants
Adds test for #24283. Not actually fixed. Just test.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes