test: Refactor test-http2-compat-serverresponse-finished.js#21929
test: Refactor test-http2-compat-serverresponse-finished.js#21929antsmartian wants to merge 1 commit intonodejs:masterfrom
Conversation
There was a problem hiding this comment.
I see this kind of change being done frequently recently and I think this is not always a good idea. If there are other tests that cover the real value then it's ok, if not we are hiding the real type by coercing to a boolean.
What I mean is that if a regression is introduced where response.socket is set to null (or any other falsy value) instead of undefined, with this change, we will not notice.
There was a problem hiding this comment.
@lpinca I got your point. I even thought about it before I did the changes, but in this context, when the response is finished, its gonna be undefined. So socket and connection are no more valid, I felt it was fine to use ok. But yeah, if some other logic expects the socket to be undefined and we pass null, we gonna cause a regression. Good point.
antsmartian
force-pushed
the
refactor
branch
2 times, most recently
from
e9106e1 to
96a8979
Compare
benjamingr
left a comment
benjamingr
left a comment
There was a problem hiding this comment.
I agree with lpinca's point about the assert.ok though.
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
LGTM, but please change the assert.ok() calls back. I think it's fine for language-level APIs, but we should be more strict with the APIs we provide for the reasons previously noted.
|
ping @antsmartian |
|
@maclover7 Oops, sorry. Will update the PR in an hour or so. |
|
@maclover7 Ping :) |
maclover7
added
test
|
Landed in f2518c4 |
PR-URL: #21929 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #21929 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#21929 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#21929 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #22850 PR-URL: #21929 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
8 participants
Just a simple refactor change:
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes