Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

orangemocha
Copy link
Contributor

See #7345

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

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Alexis Campailla

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@orangemocha
Copy link
Contributor Author

This test requires libuv fix joyent/libuv#1223 in order to pass

function parent() {
var net = require('net');
var spawn = require('child_process').spawn;
var assert = require('assert');
Copy link
Member

Choose a reason for hiding this comment

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

Double require?

@indutny
Copy link
Member

indutny commented Apr 10, 2014

Minor style nits, otherwise looking good!

@orangemocha
Copy link
Contributor Author

Thanks @indutny , updated per your feedback.

});
c.on('end', function() {
console.log('stdoutPipeServer disconnected');
gotResponse = (buf.toString() == 'hello');
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather use .join('') instead of toString() here.

@indutny
Copy link
Member

indutny commented Apr 12, 2014

Looking good, one minor nit, and will land it as soon as we will do libuv update again

See #7345

Adding a test to verify that a node process spawned via cmd with
named pipes can access its stdio streams.
@orangemocha
Copy link
Contributor Author

Updated to use push() and join()

@indutny
Copy link
Member

indutny commented Apr 14, 2014

LGTM, let's land it after updating libuv. cc @tjfontaine

@chrisdickinson
Copy link

It looks like this is failing as of latest v0.12.

@jasnell jasnell added the test label Aug 15, 2015
@jasnell
Copy link
Member

jasnell commented Aug 15, 2015

@orangemocha ... still needed?

jasnell pushed a commit to jasnell/node that referenced this pull request Aug 16, 2015
Per: nodejs/node-v0.x-archive#7433

Adding a test to verify that a node process spawned via cmd with
named pipes can access its stdio streams.
@orangemocha
Copy link
Contributor Author

Still valid. Will land shortly.

orangemocha added a commit that referenced this pull request Aug 17, 2015
See #7345

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

PR-URL: #7433
Reviewed-By: Fedor Indutny <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2015

Awesome... we'll want to make sure that the commits in master land over in nodejs/node master also

@jasnell jasnell closed this Aug 17, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants