Skip to content

Conversation

@decareano
Copy link
Contributor

  1. Test was fixed in Nov/2010 (fa556a1425 Add callback to socket.write(), fix test-sendfds)

  2. Test was disable in Oct/2011 (d2b8037ed0 disable test-sendfd).

  3. Some of the c++ binding(s) are not present anymore (ie: src/node_net.cc). That file loaded the bindings. Now, it does not exist anymore

  4. Variable netBinding has two methods: pipe() is used to create a read and write pair for the FD's and close() closes the pipe(s).

  5. Removal of process.binding() and adding a socket provided a solution to our original error.

  6. It also generated a new error:

     `TypeError: pipeReadStream.open is not a function` 

Since this test has been disabled for so many years and it has two, maybe more, levels of brokenness I request a deletion of the test.

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jul 18, 2017
Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

LGTM

@TrottTrott changed the title src: case for deletion of test-sendfd.jstest: delete obsolete test-sendfd.jsJul 18, 2017
@Trott
Copy link
Member

If it's no trouble, it would be great to fix up the commit message to comply with the guidelines in CONTRIBUTING.md. (If you don't do that, someone else can do it when merging this pull request. But if you want to save them the trouble, awesome.)

I've changed the title of the pull request to something that you can use for the first line of the commit message. The remaining lines of the commit message can be the text you wrote in the pull request description (that is, the 6 numbered points etc.). Just wrap it at 72 chars and it should be good.

Any reason you didn't use the GitHub template that is there in this repo when you open a pull request? (No judgment, genuinely curious, I can think of lots of reasons, want to know what the real one is. :-D )

@mscdexmscdex added the net Issues and PRs related to the net subsystem. label Jul 18, 2017
@tniessentniessen self-assigned this Jul 18, 2017
@MLT-Test
Copy link

Rich, this is a WIP. I was using notepad and just copy and paste.
I am using the template below. does it wrap to 72 chars?
any other edits?

test: delete obsolete test-sendfd.js

Test was fixed in Nov/2010 (fa556a1 Add callback to socket.write(), fix test-sendfds)

Test was disable in Oct/2011 (d2b8037 disable test-sendfd).

Some of the c++ binding(s) are not present anymore (ie: src/node_net.cc). That file loaded the bindings. Now, it does not exist anymore

Variable netBinding has two methods: pipe() is used to create a read and write pair for the FD's and close() closes the pipe(s).

Removal of process.binding() and adding a socket provided a solution to our original error.

It also generated a new error:

TypeError: pipeReadStream.open is not a function

Since this test has been disabled for so many years and it has two, maybe more, levels of brokenness I request a deletion of the test.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jul 21, 2017
This test was disabled in 2011 and is no longer useful without modifications. PR-URL: nodejs#14334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@tniessen
Copy link
Member

Landed in 27343cc, thank you for your first contribution! 🎉

CI on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7475/

addaleax pushed a commit that referenced this pull request Jul 22, 2017
This test was disabled in 2011 and is no longer useful without modifications. PR-URL: #14334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 24, 2017
This test was disabled in 2011 and is no longer useful without modifications. PR-URL: #14334 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
@addaleaxaddaleax mentioned this pull request Jul 24, 2017
@addaleaxaddaleax mentioned this pull request Aug 2, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

netIssues and PRs related to the net subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@decareano@Trott@MLT-Test@tniessen@jasnell@cjihrig@gibfahn@mscdex@MylesBorins@nodejs-github-bot