-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
tests: remove legacy regression test -> double pipe #5387
Conversation
LGTM |
@jasnell are there action items after CI? Failures seem to be unrelated. |
Should be in the same category as #5750. Afterwards running the full test-suite on windows should be much simpler. |
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/ |
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.
@Trott reworded the commit and force pushed. CI seems green 💯 |
7da4fd4
to
c7066fb
Compare
@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? |
c133999
to
83c7a88
Compare
ping @eljefedelrodeodeljefe ... still want this? |
Closing given the lack of forward progress on this |
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. |
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:
make -j8 test
(UNIX) orvcbuild test nosign
(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
child_process