Skip to content

Conversation

@orangemocha
Copy link
Contributor

Adding a Windows test to verify that a node process spawned via
cmd with named pipes can access its stdio streams.

Ref: nodejs/node-v0.x-archive#7345

This test was orphaned in joyent/node:master. It is still useful.

@mscdexmscdex added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Sep 9, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this section.

@orangemocha
Copy link
ContributorAuthor

Thanks @cjihrig for all the feedback! I will revamp this test according to your suggestions.

Can the Copyright notice really be removed? I don't think so. Even with a permissive license, one of the clauses is to preserve the copyright notice.

@cjihrig
Copy link
Contributor

Yea, the copyright in all the files was removed a long time ago in io.js. I'm not a lawyer, but I think I remember something about only needing the license in the root of the project.

@orangemocha
Copy link
ContributorAuthor

Ok, I'll take your word for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the format as in #2109

@orangemochaorangemochaforce-pushed the orangemocha-testCmdNamedPipe branch from 0e0886b to e9003b2CompareSeptember 11, 2015 14:59
@orangemocha
Copy link
ContributorAuthor

Updated. PTAL.

@cjihrig
Copy link
Contributor

The commented out console.log()s should probably go. You could optionally change a few lets to consts. Other than that, LGTM if the CI is happy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Const

@orangemochaorangemochaforce-pushed the orangemocha-testCmdNamedPipe branch from e9003b2 to 46e1d9fCompareSeptember 17, 2015 11:06
@orangemocha
Copy link
ContributorAuthor

Updated according to feedback. Thanks again for all the help! Much more slick now 👍

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

@orangemocha
Copy link
ContributorAuthor

argh.. linter errors

Adding a Windows test to verify that a node process spawned via cmd with named pipes can access its stdio streams. Ref: nodejs/node-v0.x-archive#7345
@orangemochaorangemochaforce-pushed the orangemocha-testCmdNamedPipe branch from 46e1d9f to ab7d0b8CompareSeptember 17, 2015 11:11
@orangemocha
Copy link
ContributorAuthor

Fixed whitespace. One more try: https://ci.nodejs.org/job/node-test-pull-request/334/

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_processIssues and PRs related to the child_process subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@orangemocha@cjihrig@evanlucas@saghul@thefourtheye@mscdex