Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test-spawn-cmd-named-pipe #2770

Closed

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.

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Sep 9, 2015
@@ -0,0 +1,91 @@
// Copyright Joyent, Inc. and other Node contributors.
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
Contributor Author

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

cjihrig commented Sep 9, 2015

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
Contributor Author

Ok, I'll take your word for it.


// This test is intended for Windows only
if (process.platform != 'win32') {
console.log('Skipping Windows-only test');
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

@orangemocha orangemocha force-pushed the orangemocha-testCmdNamedPipe branch from 0e0886b to e9003b2 Compare September 11, 2015 14:59
@orangemocha
Copy link
Contributor Author

Updated. PTAL.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2015

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.

const stdinPipeName = '\\\\.\\pipe\\' + pipeNamePrefix + '.stdin';
const stdoutPipeName = '\\\\.\\pipe\\' + pipeNamePrefix + '.stdout';

let stdinPipeServer = net.createServer(function(c) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Const

@orangemocha orangemocha force-pushed the orangemocha-testCmdNamedPipe branch from e9003b2 to 46e1d9f Compare September 17, 2015 11:06
@orangemocha
Copy link
Contributor Author

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
Contributor Author

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
@orangemocha orangemocha force-pushed the orangemocha-testCmdNamedPipe branch from 46e1d9f to ab7d0b8 Compare September 17, 2015 11:11
@orangemocha
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants