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.

Sign up for GitHub

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

Jump to bottom

test: fix test-net-happy-eyeballs without IPv6 support #45856

Merged
merged 1 commit into from Feb 18, 2023
Merged

test: fix test-net-happy-eyeballs without IPv6 support #45856

merged 1 commit into from Feb 18, 2023

Conversation

LiviaMedeiros
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros commented Dec 14, 2022 •
edited

The error observed on linux-6.1.0 with disabled CONFIG_IPV6 is EAFNOSUPPORT.

cc @ShogunPanda in case of any of:

  • EADDRNOTAVAIL is confirmed to be thrown on any other platform: in this case, it would make sense to simply leave the error unchecked. Confirmed, checking both errors.
  • undefined:undefined is not what should appear in error message: in this case, we probably need a fallback to DEFAULT_IPV4_ADDR/DEFAULT_IPV6_ADDR at

    node/lib/net.js

    Line 1040 in cf0a42c

    details = sockname.address + ':' + sockname.port;

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 14, 2022
@ShogunPanda
Copy link
Contributor

@LiviaMedeiros This looks fine to me in general. What happens for other platforms?

@LiviaMedeiros
Copy link
Contributor Author

I didn't test it but from my understanding of connect(2), it must always return EAFNOSUPPORT first on any platform.
EADDRNOTAVAIL should happen if the family is supported but all ports are unavailable (e.g. busy).

If there is any uncertainty, might be better to skip those assertions.

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@LiviaMedeiros
Copy link
Contributor Author

Judging from CI results, I was wrong and the test can produce both EAFNOSUPPORT and EADDRNOTAVAIL.

Unless we really need to check the error, skipping that part.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Dec 17, 2022
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 10, 2023
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 10, 2023
@github-actions
Copy link
Contributor

Failed to start CI
- Validating Jenkins credentials
✖  Jenkins credentials invalid
https://github.com/nodejs/node/actions/runs/4141827344

@Trott

This comment was marked as resolved.

Sign in to view
@Trott

This comment was marked as resolved.

Sign in to view
@LiviaMedeiros LiviaMedeiros added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Feb 13, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 16, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Feb 18, 2023
Open
32 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@LiviaMedeiros LiviaMedeiros added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@nodejs-github-bot nodejs-github-bot merged commit 109545a into nodejs:main Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 109545a

MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
@LiviaMedeiros @MylesBorins
test: fix test-net-autoselectfamily for kernel without IPv6 support
673f76a 
PR-URL: #45856
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
Merged
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
@LiviaMedeiros @MylesBorins
test: fix test-net-autoselectfamily for kernel without IPv6 support
6ad01fd 
PR-URL: #45856
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@danielleadams
Copy link
Member

@LiviaMedeiros this broke tests when pulling into v18.x - do you mind opening a backport PR?

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 5, 2023
LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Apr 6, 2023
@LiviaMedeiros
test: fix test-net-autoselectfamily for kernel without IPv6 support
797e941 
PR-URL: nodejs#45856
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@LiviaMedeiros LiviaMedeiros mentioned this pull request Apr 6, 2023
Closed
@LiviaMedeiros LiviaMedeiros added the backport-open-v18.x Indicate that the PR has an open backport. label Apr 6, 2023
@LiviaMedeiros
Copy link
Contributor Author

@danielleadams opened #47447
Keeping backport-requested-v18.x label as it would require second commit after #45777 (which depends on #45057 to land on v18.x, which has #45994 backport which has conflicts atm) lands on v18.x

LiviaMedeiros added a commit to LiviaMedeiros/node that referenced this pull request Aug 17, 2023
@LiviaMedeiros
test: fix test-net-autoselectfamily for kernel without IPv6 support
15b1c5a 
PR-URL: nodejs#45856
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@LiviaMedeiros
Copy link
Contributor Author

With #49016, this can be landed on v18.x.

@LiviaMedeiros LiviaMedeiros removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. labels Aug 17, 2023
@LiviaMedeiros LiviaMedeiros mentioned this pull request Aug 27, 2023
Merged
ruyadorno pushed a commit that referenced this pull request Aug 29, 2023
@LiviaMedeiros @ruyadorno
test: fix test-net-autoselectfamily for kernel without IPv6 support
c728b8a 
PR-URL: #45856
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@LiviaMedeiros @ShogunPanda @nodejs-github-bot @Trott @danielleadams @jasnell @cjihrig