Repository navigation
http: guard invalid timeout values in checkConnections - #64506
Conversation
Signed-off-by: Efe Karasakal <hi@efe.dev>
|
Review requested:
|
|
|
||
| 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!
|
|
||
| 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.
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:
|
commented
Jul 30, 2026
|
Landed in 0012a77 |
Fixes #49324