GeezerVison

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

GeezerVison is showing github.com — test: add test for exec() known issue by Trott · Pull Request #7375 · nodejs/node

Conversation

Trott commented Jun 22, 2016

Copy link

Copy Markdown

Member

Checklist

make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

test child_process

Description of change

Refs: #7342

Sorry, something went wrong.

Uh oh!

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

All reactions


          test: add test for exec() known issue

ad5b897

Refs: nodejs#7342

Trott added child_process

Issues and PRs related to the child_process subsystem.

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

Jun 22, 2016

Trott mentioned this pull request

Jun 22, 2016

Encoding of child_process.exec.stdout changed from String to Buffer in v6.2.1 #7342

Closed

Trott commented Jun 22, 2016

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott mentioned this pull request

Jun 22, 2016

Revert "child_process: measure buffer length in bytes" #7377

Closed

2 tasks done

cjihrig reviewed

Jun 22, 2016

View reviewed changes

Comment thread

test/known_issues/test-child-process-exec-stdout-data-string.js Outdated

		const assert = require('assert');
		const exec = require('child_process').exec;

		const keepAlive = setInterval(() => {}, 9999);

cjihrig Jun 22, 2016

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.

Won't the child process keep the event loop open? Works for me locally without the timer.

Sorry, something went wrong.

Uh oh!

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

All reactions

cjihrig commented Jun 22, 2016

Copy link

Copy Markdown

Contributor

LGTM with a couple comments.

All reactions

Sorry, something went wrong.

Uh oh!

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


          squash: address nits, add test for stderr

d46fe32

Trott commented Jun 23, 2016

Copy link

Copy Markdown

Member Author

@cjihrig Nits addressed. Also added a test for stderr. PTAL

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott added 2 commits

June 22, 2016 22:21


          squash: fix goofs

8485d17


          squash: lint

8141cfa

Trott commented Jun 23, 2016

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

cjihrig commented Jun 23, 2016

Copy link

Copy Markdown

Contributor

Changes LGTM

All reactions

Sorry, something went wrong.

Uh oh!

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

santigimeno commented Jun 23, 2016

Copy link

Copy Markdown

Member

LGTM

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott added a commit to Trott/io.js that referenced this pull request

Jun 24, 2016


          test: add test for exec() known issue

c964574

PR-URL: nodejs#7375
Refs: nodejs#7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

Trott commented Jun 24, 2016

Copy link

Copy Markdown

Member Author

Landed in c964574

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott closed this

Jun 24, 2016

Fishrock123 pushed a commit that referenced this pull request

Jun 27, 2016


          test: add test for exec() known issue

044a0be

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

Fishrock123 mentioned this pull request

Jun 27, 2016

Propose v6.3.0 #7441

Merged

Fishrock123 pushed a commit that referenced this pull request

Jul 5, 2016


          test: add test for exec() known issue

16aff79

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

Fishrock123 mentioned this pull request

Jul 5, 2016

Propose v6.3.0 (v2) #7550

Merged

MylesBorins mentioned this pull request

Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label

Jul 11, 2016

MylesBorins commented Jul 11, 2016

Copy link

Copy Markdown

Contributor

@Trott lts?

All reactions

Sorry, something went wrong.

Uh oh!

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

Trott commented Jul 12, 2016

Copy link

Copy Markdown

Member Author

@thealphanerd If it lands and CI's cleanly, yes.

All reactions

Sorry, something went wrong.

Uh oh!

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

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels

Jul 12, 2016

MylesBorins pushed a commit that referenced this pull request

Jul 12, 2016


          test: add test for exec() known issue

91001cc

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Jul 12, 2016


          test: add test for exec() known issue

5735a3d

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

MylesBorins mentioned this pull request

Jul 12, 2016

v4.5.0 proposal #7688

Merged

MylesBorins pushed a commit that referenced this pull request

Jul 14, 2016


          test: add test for exec() known issue

d7d9990

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

MylesBorins pushed a commit that referenced this pull request

Jul 14, 2016


          test: add test for exec() known issue

a31d316

PR-URL: #7375
Refs: #7342
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>

Trott deleted the known-exec branch

January 13, 2022 22:43

Reviewers

No reviews

Uh oh!

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

Assignees

No one assigned

Labels

child_process

Issues and PRs related to the child_process subsystem.

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.

4 participants

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.