Skip to content

test: Refactor out spawnPwd #22522

New issue

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

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

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Aug 25, 2018

Further work on common/index.js.

  • extract the gist into common.pwdCommand
  • Merge test-child-process-buffering.js into test-child-process-stdio.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@refack refack added the test Issues and PRs related to the tests. label Aug 25, 2018
@refack refack requested a review from jasnell August 25, 2018 16:35
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 25, 2018
@refack
Copy link
Contributor Author

refack commented Aug 25, 2018

/CC @nodejs/testing

@refack
Copy link
Contributor Author

refack commented Aug 25, 2018

Another reason I like this change is it adds an explicit require('child_process') to these tests who are named test-child-process-*.

P.S. I'm aware of the lint. Looking for some buy-in before I do a second pass.

@jasnell
Copy link
Member

jasnell commented Aug 27, 2018

(signed off to +1 the idea, looks like there are a few nits to fix tho)

common.spawnPwd({ stdio: ['pipe', 'pipe', 'pipe', 'ipc', 'ipc'] });
}, { code: 'ERR_IPC_ONE_PIPE', type: Error });
child.on('exit', common.mustCall(function(c) {
assert.strictEqual(0, c);
Copy link
Member

Choose a reason for hiding this comment

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

nit: We typically spell out code here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.
(also the actual, and expected args were swapped)

@refack refack force-pushed the refacotr-out-spawnPwd branch from 4103318 to b35a046 Compare August 29, 2018 17:34
@refack
Copy link
Contributor Author

refack commented Aug 29, 2018

* extract the gist into common.pwdCommand
* Merge test-child-process-buffering.js into test-child-process-stdio.js

PR-URL: nodejs#22522
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@refack refack force-pushed the refacotr-out-spawnPwd branch from b35a046 to 8569f4a Compare August 29, 2018 21:33
@refack refack merged commit 8569f4a into nodejs:master Aug 29, 2018
@refack
Copy link
Contributor Author

refack commented Aug 29, 2018

landed in 8569f4a

@refack refack deleted the refacotr-out-spawnPwd branch August 29, 2018 21:35
targos pushed a commit that referenced this pull request Aug 30, 2018
* extract the gist into common.pwdCommand
* Merge test-child-process-buffering.js into test-child-process-stdio.js

PR-URL: #22522
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 3, 2018
* extract the gist into common.pwdCommand
* Merge test-child-process-buffering.js into test-child-process-stdio.js

PR-URL: #22522
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Sep 6, 2018
* extract the gist into common.pwdCommand
* Merge test-child-process-buffering.js into test-child-process-stdio.js

PR-URL: #22522
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants