Uh oh!
There was an error while loading. Please reload this page.
wrap_js_stream: adding test for readStop - #16356
Conversation
apapirovski
left a comment
There was a problem hiding this comment.
LGTM just some minor nits
There was a problem hiding this comment.
I feel like the assert message here and in all the other instances below might be more confusing than helpful. I would just remove them and have it be the default assertion message.
Instead, comments could be added to explain all the stages of this process and what exactly is being tested.
There was a problem hiding this comment.
Could this be renamed to StreamWrap for consistency with the rest of the codebase?
apapirovski
commented
Oct 25, 2017
PR-URL: #16356 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
apapirovski
commented
Oct 26, 2017
PR-URL: nodejs/node#16356 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
PR-URL: #16356 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gibfahn
commented
Oct 30, 2017
This is failing on v8.x-staging with: Should this be backported to |
addaleax
commented
Oct 30, 2017
PR-URL: #16356 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
PR-URL: nodejs/node#16356 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Adding a test for wrap_js_stream readStop method that was skipped in coverage
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
wrap_js_stream