Skip to content

test: Use common module API on child exec - #17751

Closed
sreepurnajasti wants to merge 1 commit into
nodejs:masterfrom
sreepurnajasti:commonAPI
Closed

test: Use common module API on child exec#17751
sreepurnajasti wants to merge 1 commit into
nodejs:masterfrom
sreepurnajasti:commonAPI

Conversation

@sreepurnajasti

@sreepurnajastisreepurnajasti commented Dec 19, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Dec 19, 2017
@danbev

danbev commented Dec 19, 2017

Copy link
Copy Markdown
Contributor

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

cjihrig
cjihrig approved these changes Dec 19, 2017
View reviewed changes

@cjihrigcjihrig 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.

LGTM with a suggestion.

Comment threadtest/parallel/test-child-process-exec-stdout-stderr-data-string.js Outdated

@cjihrigcjihrigDec 19, 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.

I'd use common.mustCallAtLeast() here too, just to be safe because it's dealing with data events.

@sreepurnajastisreepurnajastiDec 20, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@cjihrig Done!! Please have a look.

lpinca
lpinca approved these changes Dec 20, 2017
View reviewed changes
Comment threadtest/parallel/test-child-process-exec-stdout-stderr-data-string.js Outdated

@lpincalpincaDec 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: we can remove the data argument.

@sreepurnajastisreepurnajastiDec 21, 2017•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@lpinca Thanks. Done!!

@maclover7

maclover7 commented Dec 22, 2017

Copy link
Copy Markdown
Contributor

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

BridgeAR
BridgeAR approved these changes Dec 22, 2017
View reviewed changes
Comment threadtest/parallel/test-child-process-exec-stdout-stderr-data-string.js Outdated

@BridgeARBridgeARDec 22, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: the callback is actually optional.

@sreepurnajastisreepurnajastiDec 22, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@BridgeAR I missed it. Its done. Thanks!!

@maclover7

maclover7 commented Dec 22, 2017

Copy link
Copy Markdown
Contributor

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

@maclover7

maclover7 commented Dec 24, 2017

Copy link
Copy Markdown
Contributor

Landing...

@maclover7maclover7 self-assigned this Dec 24, 2017
@maclover7

maclover7 commented Dec 24, 2017

Copy link
Copy Markdown
Contributor

Thank you for your contribution, landed in 4d74b52.
❤️ 💚 💙 💛 💜

@sreepurnajasti
sreepurnajasti deleted the commonAPI branch December 27, 2017 06:35
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
3be3176
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
fd9f8ab
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
ded097a
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Jan 10, 2018
Merged
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@sreepurnajasti@gibfahn
test: use common module API in test-child-process-exec-stdout-stderr-…
377a23a
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@sreepurnajasti@gibfahn
test: use common module API in test-child-process-exec-stdout-stderr-…
ae28013
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@sreepurnajasti@gibfahn
test: use common module API in test-child-process-exec-stdout-stderr-…
59aa505
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
78e0cb9
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
87baf4d
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@sreepurnajasti@MylesBorins
test: use common module API in test-child-process-exec-stdout-stderr-…
5314754
…data-string
PR-URL: #17751
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@cjihrigcjihrigcjihrig approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+2 more reviewers
@maclover7maclover7maclover7 approved these changes
@hiroppyhiroppyhiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@maclover7maclover7

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@sreepurnajasti@danbev@maclover7@jasnell@lpinca@cjihrig@hiroppy@BridgeAR@gibfahn@nodejs-github-bot