-
-
Notifications
You must be signed in to change notification settings - Fork 133
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: add "address()" on mock Socket #549
Changes from 5 commits
1fb5bf2
4257577
8b81866
9c7623a
f9168d4
ce1a704
af55435
1a0d5fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,7 @@ export class MockHttpSocket extends MockSocket { | |
*/ | ||
public passthrough(): void { | ||
const socket = this.createConnection() | ||
this.address = socket.address.bind(socket) | ||
|
||
// Flush the buffered "socket.write()" calls onto | ||
// the original socket instance (i.e. write request body). | ||
|
@@ -205,6 +206,13 @@ export class MockHttpSocket extends MockSocket { | |
return | ||
} | ||
|
||
// Return fake address information for the socket. | ||
this.address = () => ({ | ||
address: '127.0.0.1', | ||
family: 'IPv4', | ||
port: 0, | ||
}) | ||
|
||
// First, emit all the connection events | ||
// to emulate a successful connection. | ||
this.mockConnect() | ||
|
@@ -298,7 +306,7 @@ export class MockHttpSocket extends MockSocket { | |
} | ||
|
||
private mockConnect(): void { | ||
this.emit('lookup', null, '::1', 6, this.connectionOptions.host) | ||
this.emit('lookup', null, '127.0.0.1', 4, this.connectionOptions.host) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think setting const address = {
host: '...',
family: '...',
port: '...'
}
this.address = () => address
this.emit('lookup', null, address.host, address.family, this.connectionOptions.host) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonderful! I'm ashamed that I didn't think about it myself.😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You think of a ton of things that never crossed my mind 🙌 The power of collaboration. |
||
this.emit('connect') | ||
this.emit('ready') | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,3 +163,41 @@ it('mocks response to a non-existing host', async () => { | |
expect(await text()).toBe('howdy, john') | ||
expect(requestListener).toHaveBeenCalledTimes(1) | ||
}) | ||
|
||
it('returns mocked socket address', async () => { | ||
interceptor.on('request', async ({ request }) => { | ||
request.respondWith(new Response()) | ||
}) | ||
|
||
const connectPromise = new DeferredPromise<object>() | ||
const request = http.get('http://example.com') | ||
request.once('socket', socket => { | ||
socket.once('connect', () => { | ||
connectPromise.resolve(socket.address()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be great to also have a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the lookup happens after the socket event is emitted. request.once('socket', socket => {
console.log(socket.address()); // prints empty object {}
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is correct. ClientRequest emits |
||
}) | ||
}) | ||
|
||
await expect(connectPromise).resolves.toEqual({ | ||
address: '127.0.0.1', | ||
family: 'IPv4', | ||
port: 0, | ||
}) | ||
}); | ||
|
||
|
||
it('returns real socket address', async () => { | ||
const connectPromise = new DeferredPromise<object>() | ||
const request = http.get(httpServer.http.url('/user')) | ||
request.once('socket', socket => { | ||
socket.once('connect', () => { | ||
connectPromise.resolve(socket.address()) | ||
}) | ||
}) | ||
|
||
await waitForClientRequest(request); | ||
await expect(connectPromise).resolves.toEqual({ | ||
address: '127.0.0.1', | ||
family: 'IPv4', | ||
port: expect.any(Number), | ||
}) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's any info in
connectionOptions
that we can use to set thefamily
? 🤔Yeah, there's
.family
and.host
. Do you think we should respect those?We can also add automated tests for the cases when explicit connection options are set. Granted, this is likely better tested in the unit tests on
MockHttpSocket
. You can leave the existinghttp.*
tests as-is, those are useful too!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the address info resolved in the socket. I didn't verfiy this, but the
connectionOptions
includes only theport
.Also, please note the
host
!==address
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Let's infer the
port
only for now then?