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: add test for internalConnect() when address type is IPv6 #22444

Conversation

yanivfriedensohn
Copy link
Contributor

According to the latest coverage report, internalConnect() is uncovered when address type is IPv6 (https://coverage.nodejs.org/coverage-36696cfe8479f3fd/root/net.js.html#L887). This PR adds a test for internalConnect() when address type is IPv6.

I didn't add a test when address type is neither IPv4 nor IPv6 since this condition isn't reachable.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 21, 2018
@addaleax
Copy link
Member

The Travis CI failure seems related:

not ok 2356 sequential/test-net-connect-local-error
  ---
  duration_ms: 0.266
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:338
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert.ok(expectedErrorCodes.includes(err.code))
    
        at onError (/home/travis/build/nodejs/node/test/sequential/test-net-connect-local-error.js:28:10)
        at Socket.clientIPv6.on.common.mustCall (/home/travis/build/nodejs/node/test/sequential/test-net-connect-local-error.js:25:49)
        at Socket.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:464:15)
        at Socket.emit (events.js:182:13)
        at emitErrorNT (internal/streams/destroy.js:82:8)
        at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
        at process._tickCallback (internal/process/next_tick.js:63:19)
        at Function.Module.runMain (internal/modules/cjs/loader.js:745:11)
        at startup (internal/bootstrap/node.js:257:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:634:3)


client.on('error', common.mustCall(function onError(err) {
const onError = (err, options) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a function declaration?

function onError(err, options) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a named function expression.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a style preference, function declaration are much more common in the test suite than named function expressions.

@yanivfriedensohn
Copy link
Contributor Author

yanivfriedensohn commented Aug 21, 2018

Thanks @addaleax, I added common.hasIPv6 before testing IPv6 and now the checks are passing.

@addaleax
Copy link
Member

addaleax commented Aug 21, 2018

@refack
Copy link
Contributor

refack commented Aug 22, 2018

Hello @yanivfriedensohn and אהלן. Thank you for your contribution 🥇
If you are not familiar with our review and landing process, it's covered in CONTRIBUTING.md. Any PR should stay open for at least 48 hours so that other contributors can review, comment, and familiarize with the change.

P.S. If you have any question you can also feel free to contact me directly.

@yanivfriedensohn
Copy link
Contributor Author

Hi @refack and שלום. Following your comment I changed noError() to a function declaration.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@refack
Copy link
Contributor

refack commented Aug 22, 2018

Thank you for following up.

@refack
Copy link
Contributor

refack commented Aug 22, 2018

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/16686/

@addaleax
Copy link
Member

Landed in e03f9c5, thanks for the PR! 🎉

@addaleax addaleax closed this Aug 27, 2018
addaleax pushed a commit that referenced this pull request Aug 27, 2018
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack refack mentioned this pull request Oct 18, 2018
3 tasks
@yanivfriedensohn yanivfriedensohn deleted the test-net-internal-connect-ipv6 branch June 21, 2019 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants