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

fix: use 127.0.0.1 in test to explicitly listen on IPv4 interface #215

Closed

Conversation

lionralfs
Copy link

Hey,

tests in node >= 17.0.0 are not passing due to the way in which localhost gets resolved. From (nodejs/node#40537 (comment)):

Node.js now returns IP addresses in the order they are returned from the name resolver/DNS. There's a --dns-result-order command line option you can set to control the behaviour -- the difference in Node.js 17 is the default changed.

This change was introduced in nodejs/node#39987 (node v17.0.0), causing this test to fail:

test('emits the ECONNREFUSED error connecting to an inactive server given no mocked response', (done) => {
const request = new NodeClientRequest(
normalizeClientRequestArgs('http:', 'http://localhost:12345'),
{
observer: new EventEmitter(),
resolver() {},
}
)
request.on('error', (error: ErrorConnectionRefused) => {
expect(error.code).toEqual('ECONNREFUSED')
expect(error.syscall).toEqual('connect')
expect(error.address).toEqual('127.0.0.1')
expect(error.port).toEqual(12345)
done()
})
request.end()
})

localhost resolves to the IPv6 address ::1 which means that the line expect(error.address).toEqual('127.0.0.1') fails.

This PR changes the test to explicitly listen on the IPv4 equivalent.

@kettanaito
Copy link
Member

Hey, @lionralfs. Thanks for submitting this pull request!

So, does this mean you can no longer construct a ClientRequest with a localhost hostname?

@lionralfs
Copy link
Author

lionralfs commented Feb 27, 2022

This is just a change for the test case, so that tests don't break in newer node versions.

Also, you should still be able to use localhost in the NodeClientRequest but you might run into issues where if you pass localhost, it resolves to ::1 (IPv6) instead of 127.0.0.1 (IPv4). That is out of scope for this library though.

More info (from nodejs/node#39987):

Switch the default from false (reorder the result so that IPv4 addresses
come before IPv6 addresses) to true (return them exactly as the resolver
sent them to us.)

@kettanaito
Copy link
Member

Hey, @lionralfs. Thanks for opening this!

We don't currently have official support for Node 17. While the library should work there, our test suite is meant to be run on Node 14/16. I'm putting this change on hold until we adopt Node 17 as the official development version.

kettanaito added a commit that referenced this pull request Feb 20, 2023
- Closes #215

Migrating away from < Node 18 for happier life.
@lionralfs lionralfs deleted the fix/node17-localhost-dns-resolve branch February 20, 2023 18:22
@kettanaito
Copy link
Member

Released: v0.21.1 🎉

This has been released in v0.21.1!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants