Skip to content

Conversation

@eljefedelrodeodeljefe
Copy link
Contributor

Description of change

as described at #5373

This test was introduced in 07da49b and seems to be obsolete since
96e423a, when child_process was deprecated from c++ land.

Motivation to change this is, that it requires sed and grep on
windows, yet being the only occurences of those. I decided to
propose deletion, since it's difficult to reproduce the breaking
case in the current node implementation.

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

child_process

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Feb 23, 2016
@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@eljefedelrodeodeljefe
Copy link
Contributor Author

@jasnell are there action items after CI? Failures seem to be unrelated.

@eljefedelrodeodeljefe
Copy link
Contributor Author

Should be in the same category as #5750. Afterwards running the full test-suite on windows should be much simpler.

@Trott
Copy link
Member

Trott commented Apr 6, 2016

LGTM.

Tiniest of nits: Strictly speaking, this isn't a deprecation. So I would recommend changing the commit message to use a verb like "remove" or "delete" instead of "deprecate".

I can't imagine how removing an obsolete test could run into trouble in CI, but as always, I'm a fan of the color green, so let's run it again. https://ci.nodejs.org/job/node-test-pull-request/2170/

@eljefedelrodeodeljefe eljefedelrodeodeljefe changed the title tests: deprecate legacy regression test -> double pipe tests: remove legacy regression test -> double pipe Apr 6, 2016
This test was introduced in 07da49b and seems to be obsolete since
96e423a, when child_process was deprecated from c++ land.

Motivation to change this is, that it  requires sed and grep on
windows, yet being the only occurences of those. I decided to
propose deletion, since it's difficult to reproduce the breaking
case in the current node implementation.
@eljefedelrodeodeljefe
Copy link
Contributor Author

@Trott reworded the commit and force pushed. CI seems green 💯

@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@Trott
Copy link
Member

Trott commented May 11, 2016

@eljefedelrodeodeljefe Since you are now the proud owner of a commit bit on this repo, do you want to rebase against master, run CI one last time, and merge this thing?

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

ping @eljefedelrodeodeljefe ... still want this?

@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

Closing given the lack of forward progress on this

@jasnell jasnell closed this Feb 28, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 2, 2017

Seems a pity to close this when the work has all been done, the checking is finished and it's had an LGTM from @Trott , it's just a matter of rebasing and landing.

I rebased here: gibfahn/node:deprecate-pipe-double , I guess we should rerun CI, though I'm not sure what it will show (this PR just removes a test).

@eljefedelrodeodeljefe care to rebase? I can open a new PR with this commit if that'd be easier.

@gibfahn gibfahn self-assigned this Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

child_process Issues and PRs related to the child_process subsystem. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants