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: test various calling patterns of net.connect() related functions #11847

Closed
wants to merge 5 commits into from

Conversation

@joyeecheung
Copy link
Member

commented Mar 14, 2017

Related: #11761
This PR adds coverage to more calling patterns of net.connect:

  • Cover the calling pattern net.connect(), net.createConnection(), new Socket().connect() with/without callback, with port passed in plain argument/in option
  • Refactor test-net-create-connection.js and rename it to test-net-connect-options-port.js
  • Add tests for connect(path)
  • Refactor test-net-connect-options.js, add tests for more new Socket() options.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, net

@joyeecheung

This comment has been minimized.

@joyeecheung joyeecheung force-pushed the joyeecheung:net-connect-test branch Mar 14, 2017

@joyeecheung joyeecheung force-pushed the joyeecheung:net-connect-test branch to 8d6ee2c Mar 15, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2017

Error messages vary on different machines..loosen up the message validation and try again. CI: https://ci.nodejs.org/job/node-test-pull-request/6850/

@joyeecheung joyeecheung changed the title test: test various calling patterns of net.connect() related functions [WIP]test: test various calling patterns of net.connect() related functions Mar 15, 2017

@joyeecheung joyeecheung force-pushed the joyeecheung:net-connect-test branch 2 times, most recently to 969e285 Mar 15, 2017

@joyeecheung joyeecheung force-pushed the joyeecheung:net-connect-test branch 10 times, most recently to 40e984d Mar 15, 2017

@joyeecheung joyeecheung changed the title [WIP]test: test various calling patterns of net.connect() related functions test: test various calling patterns of net.connect() related functions Mar 16, 2017

@joyeecheung

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2017

Something is wrong with net.connect({fd: TCPfd})(keep getting ENOTCONN or flaky libuv assertion failures)..probably has something to do with the state management. I'll remove it from this PR for now and investigate later. Pipe fd works fine though.

@jasnell PTAL, thanks.

CI is green now: https://ci.nodejs.org/job/node-test-pull-request/6867/

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Still LGTM!

jasnell added a commit that referenced this pull request Mar 16, 2017
test: add more and refactor test cases to net.connect
PR-URL: #11847
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Landed in 7b830f4

@jasnell jasnell closed this Mar 16, 2017

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017
test: add more and refactor test cases to net.connect
PR-URL: nodejs#11847
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

This PR need backport to v7

@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

@joyeecheung Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

If it is backported, the PR should include #12601.

@gibfahn gibfahn referenced this pull request Jun 18, 2017
3 of 3 tasks complete

@joyeecheung joyeecheung self-assigned this Oct 17, 2017

@joyeecheung joyeecheung removed their assignment Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.