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

test: delete obsolete test-sendfd.js #14334

Closed
wants to merge 1 commit into from

Conversation

decareano
Copy link

  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-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 18, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

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

Trott commented Jul 18, 2017

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 )

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Jul 18, 2017
@tniessen tniessen 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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@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/

@tniessen tniessen closed this Jul 21, 2017
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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@addaleax addaleax mentioned this pull request Jul 24, 2017
@addaleax addaleax mentioned this pull request Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants