FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

wrap_js_stream: adding test for readStop by akaila · Pull Request #16356 · nodejs/node · GitHub

/ node Public

wrap_js_stream: adding test for readStop - #16356

Closed
akaila wants to merge 1 commit into
nodejs:masterfrom
akaila:test-stream
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

akaila commented Oct 21, 2017

Copy link
Copy Markdown
Contributor

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 added the test Issues and PRs related to the tests. label Oct 21, 2017
mscdex added the tls Issues and PRs related to the tls subsystem. label Oct 21, 2017
apapirovski approved these changes Oct 22, 2017
View reviewed changes

apapirovski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM just some minor nits

Comment thread test/parallel/test-wrap-js-stream-read-stop.js Outdated

apapirovski Oct 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

akaila Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done

Comment thread test/parallel/test-wrap-js-stream-read-stop.js Outdated

apapirovski Oct 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

akaila Oct 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done

apapirovski commented Oct 25, 2017

Copy link
Copy Markdown
Contributor

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

apapirovski pushed a commit that referenced this pull request Oct 26, 2017
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 commented Oct 26, 2017

Copy link
Copy Markdown
Contributor

Landed in 9702ac5

Thanks for the contribution @akaila!

addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
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 mentioned this pull request Oct 30, 2017
Merged
gibfahn pushed a commit that referenced this pull request Oct 30, 2017
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 commented Oct 30, 2017

Copy link
Copy Markdown
Member

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 commented Oct 30, 2017

Copy link
Copy Markdown
Member

@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 mentioned this pull request Oct 30, 2017
Closed
2 tasks
akaila deleted the test-stream branch October 30, 2017 23:21
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
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 mentioned this pull request Nov 21, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
apapirovski apapirovski approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL