Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

tests: remove legacy regression test -> double pipe #5387

Closed
wants to merge 1 commit into from
Closed

tests: remove legacy regression test -> double pipe #5387

wants to merge 1 commit into from

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 💯

@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