Skip to content

wrap_js_stream: adding test for readStop#16356

Closed
akaila wants to merge 1 commit into
nodejs:masterfrom
akaila:test-stream
Closed

wrap_js_stream: adding test for readStop#16356
akaila wants to merge 1 commit into
nodejs:masterfrom
akaila:test-stream

Conversation

@akaila
Copy link
Copy Markdown
Contributor

@akaila akaila commented Oct 21, 2017

Adding a test for wrap_js_stream readStop method that was skipped in coverage

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

wrap_js_stream

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 21, 2017
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 21, 2017
apapirovski
apapirovski approved these changes Oct 22, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just some minor nits

Comment thread test/parallel/test-wrap-js-stream-read-stop.js Outdated
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski Oct 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@akaila akaila Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread test/parallel/test-wrap-js-stream-read-stop.js Outdated
Copy link
Copy Markdown
Contributor

@apapirovski apapirovski Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be renamed to StreamWrap for consistency with the rest of the codebase?

Copy link
Copy Markdown
Contributor Author

@akaila akaila Oct 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@apapirovski
Copy link
Copy Markdown
Contributor

apapirovski commented Oct 25, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/10982/

apapirovski pushed a commit that referenced this pull request Oct 26, 2017
@akaila @apapirovski
test: add test for WrapStream readStop
9702ac5 
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
Copy link
Copy Markdown
Contributor

apapirovski commented Oct 26, 2017

Landed in 9702ac5

Thanks for the contribution @akaila!

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@akaila @addaleax
test: add test for WrapStream readStop
cf30f57 
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>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
@akaila @gibfahn
test: add test for WrapStream readStop
7296988 
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
Copy link
Copy Markdown
Member

gibfahn commented Oct 30, 2017

This is failing on v8.x-staging with:

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Error: Cannot find module 'internal/wrap_js_stream'
    at Function.Module._resolveFilename (module.js:534:15)
    at Function.Module._load (module.js:464:25)
    at Module.require (module.js:577:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/home/gib/node/test/parallel/test-wrap-js-stream-read-stop.js:6:20)
    at Module._compile (module.js:633:30)
    at Object.Module._extensions..js (module.js:644:10)
    at Module.load (module.js:552:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:487:3)
Command: out/Release/node --expose-internals /home/gib/node/test/parallel/test-wrap-js-stream-read-stop.js
[01:47|% 100|+ 2024|-   3]: Done                                               
make: *** [test] Error 1

@addaleax
Copy link
Copy Markdown
Member

addaleax commented Oct 30, 2017

@gibfahn This depends on #16158, there is no point in a manual backport for this (but I can take a stab at the other PR)

@MylesBorins MylesBorins mentioned this pull request Oct 30, 2017
Closed
2 tasks
@akaila akaila deleted the test-stream branch October 30, 2017 23:21
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
@akaila @gibfahn
test: add test for WrapStream readStop
47b1c3b 
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 gibfahn mentioned this pull request Nov 21, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@akaila @addaleax
test: add test for WrapStream readStop
e39a103 
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>
This was referenced May 13, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell