Skip to content

test: add test for internalConnect() when address type is IPv6#22444

Closed
yanivfriedensohn wants to merge 2 commits intonodejs:masterfrom
yanivfriedensohn:test-net-internal-connect-ipv6
Closed

test: add test for internalConnect() when address type is IPv6#22444
yanivfriedensohn wants to merge 2 commits intonodejs:masterfrom
yanivfriedensohn:test-net-internal-connect-ipv6

Conversation

@yanivfriedensohn
Copy link
Contributor

@yanivfriedensohn yanivfriedensohn commented Aug 21, 2018

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

addaleax commented Aug 21, 2018

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)

@yanivfriedensohn yanivfriedensohn force-pushed the test-net-internal-connect-ipv6 branch from 0fef1ee to 9c172e7 Compare August 21, 2018 20:54
@yanivfriedensohn yanivfriedensohn force-pushed the test-net-internal-connect-ipv6 branch from 9c172e7 to ba908ba Compare August 21, 2018 21:29
refack
refack reviewed Aug 21, 2018
View reviewed changes
test/sequential/test-net-connect-local-error.js Outdated
};

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

@refack refack Aug 21, 2018

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

@yanivfriedensohn yanivfriedensohn Aug 21, 2018

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

@refack refack Aug 22, 2018

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 •
edited
Loading

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

@addaleax
Copy link
Member

addaleax commented Aug 21, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/16650/ (edit: ✔️)

@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

yanivfriedensohn commented Aug 22, 2018

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

refack
refack approved these changes Aug 22, 2018
View reviewed changes
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

addaleax commented Aug 27, 2018

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
@yanivfriedensohn @addaleax
test: add test for internalConnect() when address type is IPv6
e03f9c5 
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
@yanivfriedensohn @addaleax
test: add test for internalConnect() when address type is IPv6
99f3d1e 
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@yanivfriedensohn @targos
test: add test for internalConnect() when address type is IPv6
8638999 
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@yanivfriedensohn @targos
test: add test for internalConnect() when address type is IPv6
0ffedd2 
PR-URL: #22444
Reviewed-By: Refael Ackermann <refack@gmail.com>
This was referenced Sep 6, 2018
Closed
Closed
@refack refack mentioned this pull request Oct 18, 2018
Merged
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

Reviewers

1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yanivfriedensohn @addaleax @refack @nodejs-github-bot