GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: Use common module API on child exec by sreepurnajasti · Pull Request #17751 · nodejs/node

Conversation

@sreepurnajasti

@sreepurnajasti sreepurnajasti 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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. 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/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with a suggestion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread test/parallel/test-child-process-exec-stdout-stderr-data-string.js Outdated

@cjihrig cjihrig Dec 19, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sreepurnajasti sreepurnajasti Dec 20, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell approved these changes Dec 19, 2017
View reviewed changes
hiroppy
hiroppy approved these changes Dec 19, 2017
View reviewed changes
@sreepurnajasti
sreepurnajasti force-pushed the commonAPI branch from 366ec2a to 5d20c21 Compare December 20, 2017 04:40
lpinca
lpinca approved these changes Dec 20, 2017
View reviewed changes
Comment thread test/parallel/test-child-process-exec-stdout-stderr-data-string.js Outdated

@lpinca lpinca Dec 20, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sreepurnajasti sreepurnajasti Dec 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@lpinca Thanks. Done!!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sreepurnajasti
sreepurnajasti force-pushed the commonAPI branch from 5d20c21 to 48b6672 Compare December 21, 2017 04:08
maclover7
maclover7 approved these changes Dec 22, 2017
View reviewed changes
@maclover7

maclover7 commented Dec 22, 2017

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

@BridgeAR BridgeAR Dec 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@sreepurnajasti sreepurnajasti Dec 22, 2017

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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!!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@sreepurnajasti
test: Use common module API on child exec
c363495
@sreepurnajasti
sreepurnajasti force-pushed the commonAPI branch from 48b6672 to c363495 Compare December 22, 2017 05:26
@maclover7

maclover7 commented Dec 22, 2017

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@maclover7

maclover7 commented Dec 24, 2017

Copy link
Copy Markdown
Contributor

Landing...

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@maclover7 maclover7 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 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@maclover7 maclover7 closed this Dec 24, 2017
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
v9.4.0 proposal #18069
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@gibfahn gibfahn added land-on-v6.x labels Jan 24, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
v6.13.0 proposal #18342
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+2 more reviewers
@maclover7 maclover7 maclover7 approved these changes
@hiroppy hiroppy hiroppy approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@maclover7 maclover7

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@sreepurnajasti @danbev @maclover7 @jasnell @lpinca @cjihrig @hiroppy @BridgeAR @gibfahn @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.