test: refactor test-http-information-processing #32547
Conversation
Trott
added
the
author ready
| console.error('Server sending full response...'); | ||
| res.writeHead(200, { | ||
| 'Content-Type': 'text/plain', | ||
| 'ABCD': '1' | ||
| }); | ||
| res.end(test_res_body); | ||
| res.end(testResBody); |
There was a problem hiding this comment.
common.mustCall variant can be used here.
There was a problem hiding this comment.
We could, but I'm not sure we should. This test isn't checking to see if res.end() fires its callback or not. If the response isn't sent, the test will fail. We don't need the extra check. Adding checks that are unrelated to the test case makes it harder for someone reading the code to understand what the test is actually supposed to be testing.
This makes it sound like I'm much more opposed to the idea than I actually am. I'm on the fence, around a -0 or so.
I was a -1 about 10 minutes ago, so 10 minutes from now, I'll probably be an enthusiastic +1.
There was a problem hiding this comment.
This test isn't checking to see if res.end() fires its callback or not
correct me if I am wrong, but isn't that the case with most of 100+ http test cases?
- intent of the test does not cover explicit checking on
res.end - but as a matter of practice, they do that check
if you can generalize a statement aroundwhether http tests should check life cycle events are met or notI can also work towards implementing this across other tests too.
There was a problem hiding this comment.
- intent of the test does not cover explicit checking on
res.end- but as a matter of practice, they do that check
I'm not seeing that with res.end() calls. I'm using grep and doing a naive/superficial check, so results may be imperfect but close enough to draw reasonable conclusions:
- Almost none of the 200+ calls to
res.end()inparallel/test-http*have a callback wrapped inmustCall(). The majority of them are like the call here whereres.endI()is called and no callback is provided. - Of those that do wrap a callback in
muscCall(), only one file checks empty callback:test-http-outgoing-end-multiple.js. That makes sense for that test because it is literally checking that callingres.end()multiple times will work as expected.
I don't think we should add a check for a callback here that we're not using. Again, I'm not strongly opposed, but I am mildly resistant. 😀
There was a problem hiding this comment.
I used this command grep -e "res.on('end" -e "response.on('end" test-http*.js | grep "mustCall" | wc -l and got around 50 instances. Unfortunately I don't have the insight to validate their context. so I will leave it at that. Thank you!
Replace magic numbers with constant. Use for loop for repeated code. Reduce console logging. Remove unneeded countdown module use. PR-URL: nodejs#32547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
|
Landed in 8905be2 |
|
I honestly have no idea what happened. I did make that change, but yeah, clearly I didn't push it up or something.... |
|
I wonder if I did it on some other PR but left the comment and clicked "Resolve" in this one.... |
|
No worries, I'll mark it as a todo for a future round of edits. |
|
Replace magic numbers with constant. Use for loop for repeated code. Reduce console logging. Remove unneeded countdown module use. PR-URL: #32547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Replace magic numbers with constant. Use for loop for repeated code. Reduce console logging. Remove unneeded countdown module use. PR-URL: #32547 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Replace magic numbers with constant.
Use for loop for repeated code.
Reduce console logging.
Remove unneeded countdown module use.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes