Skip to content

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

Closed
lionralfs wants to merge 3 commits intomswjs:mainfrom
lionralfs:fix/node17-localhost-dns-resolve
Closed

fix: use 127.0.0.1 in test to explicitly listen on IPv4 interface#215
lionralfs wants to merge 3 commits intomswjs:mainfrom
lionralfs:fix/node17-localhost-dns-resolve

Conversation

@lionralfs
Copy link
Copy Markdown

@lionralfs lionralfs commented Feb 26, 2022

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:

interceptors/src/interceptors/ClientRequest/NodeClientRequest.test.ts

Lines 151 to 170 in 4563ce7

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.

@lionralfs lionralfs mentioned this pull request Feb 26, 2022
Closed
@kettanaito
Copy link
Copy Markdown
Member

kettanaito commented Feb 27, 2022

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
Copy Markdown
Author

lionralfs commented Feb 27, 2022 •
edited
Loading

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
Copy Markdown
Member

kettanaito commented May 2, 2022

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 kettanaito mentioned this pull request Aug 27, 2022
Closed
@milesrichardson milesrichardson mentioned this pull request Aug 29, 2022
Closed
@kettanaito kettanaito mentioned this pull request Feb 20, 2023
Merged
kettanaito added a commit that referenced this pull request Feb 20, 2023
@kettanaito
chore: use node 18 for development (#342)
2c5ca74 
- 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
Copy Markdown
Member

kettanaito commented Feb 20, 2023

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lionralfs @kettanaito