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

Infrastructure: Update localhost references for regression tests #2749

Merged
merged 1 commit into from Jul 25, 2023

Conversation

howard-e
Copy link
Contributor

@howard-e howard-e commented Jul 11, 2023

See #2636

Node v17+ deprioritised IPv4 addresses in favour of IPv6 with a change to dns.setDefaultResultOrder. This was done without a happy-eyeballs implementation which meant the loopback address used when running the regression tests resolved to ::1 rather than the IPv4 loopback, 127.0.0.1 which geckodriver expected. So this PR now explicitly uses 127.0.0.1.

Alternatively, we could have added --dns-result-order=ipv4first to the running script, or added dns.setDefaultResultOrder('ipv4first');, but would eventually remove it as the happy-eyeballs implementation is now included in v20.

Note the current environment details for the regression tests when running the node setup:

Environment details
  node: v18.16.1
  npm: 9.5.1

WAI Preview Link (Last built on Tue, 11 Jul 2023 21:24:38 GMT).

@howard-e howard-e added the Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation label Jul 11, 2023
@howard-e howard-e marked this pull request as ready for review July 11, 2023 21:43
@howard-e howard-e requested a review from mcking65 July 11, 2023 21:44
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Infrastructure: Update localhost references for regression tests.

The full IRC log of that discussion <jugglinmike> subtopic: Infrastructure: Update localhost references for regression tests
<jugglinmike> github: https://github.com//pull/2749
<jugglinmike> Matt_King: According to howard-e's research, this will be fixed by Node.js version 20
<jugglinmike> Matt_King: ...but if we merge this, we can fix it in the current release of Node.js
<jugglinmike> howard-e: That's right!
<jugglinmike> howard-e: This is a low-risk patch and easy to revert. I think it should be fine to merge now that you've reviewed, Matt_King.
<jugglinmike> Matt_King: If something went wrong, then the workflows that are running the regression tests would fail, right?
<jugglinmike> howard-e: Yes
<jugglinmike> Matt_King: Okay; I'll merge it later today

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Thank you @howard-e for this insightful research. My only questions is whether we would want to reverse this change after Node 20 is released? Is there any downside to specifying the IP instead of localhost?

@mcking65 mcking65 merged commit 081e874 into main Jul 25, 2023
17 checks passed
@mcking65 mcking65 deleted the fix-2636 branch July 25, 2023 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Related to maintaining task force and repo operations, processes, systems, documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infrastructure: Remove temp pinning to Node 16 to work around tests failing with Node 18 in CI
3 participants