-
-
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 3 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() | ||
|
||
// 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: '0.0.0.0', | ||
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. What do you think about setting |
||
family: 'IPv4', | ||
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 wonder if there's any info in Yeah, there's this.address = () => {
return {
address: this.connectionOptions?.host || '127.0.0.1',
family: this.connectionOptions?.family || 'IPv6',
port: this.connectionOptions?.port || 0
}
} 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 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 the address info resolved in the socket. I didn't verfiy this, but the 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're right. Let's infer the |
||
port: 0, | ||
}) | ||
|
||
// First, emit all the connection events | ||
// to emulate a successful connection. | ||
this.mockConnect() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,3 +163,40 @@ 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: '0.0.0.0', | ||
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 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.
Technically, this would be safer:
In case
this
in theaddress()
points to the socket instance, which I think it does. Let's preserve the context.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 both ways preserve the context, but I agree that your solution is more explicit.