http: guard invalid timeout values in checkConnections - #64506
Conversation
Signed-off-by: Efe Karasakal <hi@efe.dev>
|
Review requested:
|
nodejs-github-bot
added
http
|
|
||
| server.listen(0, '127.0.0.1', () => { | ||
| setTimeout(() => { | ||
| server.close(() => process.exit(0)); |
There was a problem hiding this comment.
Why not just check whether .close() has been called? And that way we avoid the child part
There was a problem hiding this comment.
Because the buggy behaviour crashes the whole process at native level, so the JS assertion wouldn't run
There was a problem hiding this comment.
I agree with @bjohansebas, I don't think we need this extra dance. This could just make normal assertions - if it crashes that will be a clear failure anyway. The example below is a test that is supposed to crash (god knows why, I'm not going to dig there) which needs the wrapper to turn the crash into success.
For things that shouldn't crash at all, we can just keep it simple and the test harness will handle & report non-zero exits as test failures for us. It won't fail the whole run, every test runs in a separate process.
There was a problem hiding this comment.
Ok, all reviewers disagree with me and I'm not willing to die on this hill 😅
Jokes aside, fair points, I've updated the test. Thanks both!
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Actual fix looks perfectly reasonable, but I do think the test is doing a bunch of work it doesn't need to.
|
|
||
| server.listen(0, '127.0.0.1', () => { | ||
| setTimeout(() => { | ||
| server.close(() => process.exit(0)); |
There was a problem hiding this comment.
I agree with @bjohansebas, I don't think we need this extra dance. This could just make normal assertions - if it crashes that will be a clear failure anyway. The example below is a test that is supposed to crash (god knows why, I'm not going to dig there) which needs the wrapper to turn the crash into success.
For things that shouldn't crash at all, we can just keep it simple and the test harness will handle & report non-zero exits as test failures for us. It won't fail the whole run, every test runs in a separate process.
Ethan-Arrowood
left a comment
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
agree with other reviews. nice fix
Signed-off-by: Efe Karasakal <hi@efe.dev>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #64506 +/- ##
==========================================
- Coverage 90.23% 90.14% -0.10%
==========================================
Files 741 743 +2
Lines 241692 242415 +723
Branches 45541 45656 +115
==========================================
+ Hits 218097 218521 +424
- Misses 15113 15393 +280
- Partials 8482 8501 +19
🚀 New features to boost your workflow:
|
gurgunday
added
request-ci
github-actions
Bot
removed
the
request-ci
bjohansebas
added
commit-queue-squash
pimterry
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 0012a77 |
Signed-off-by: Efe Karasakal <hi@efe.dev> PR-URL: #64506 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Tim Perry <pimterry@gmail.com>
Signed-off-by: Efe Karasakal <hi@efe.dev> PR-URL: #64506 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Tim Perry <pimterry@gmail.com>
8 participants
Fixes #49324