fix: add "address()" on mock Socket #549

mikicho · 2024-04-13T11:11:48Z

No description provided.

kettanaito · 2024-04-13T18:10:18Z

Sharing our conversation from Discord:

WDYT about something like realSocket property in the MockSocket class? so in the address function we can do something like: if (mocked) {..} else this.realSocket.address()

That's tempting but I would recommend against it. realSocket introduces a state and I'd rather not have any state (or as little as possible state).

Instead, we know the mocked and passthrough contexts because we have methods respondWith() and passthrough(). Those methods can set the address function on the socket instance based on the context:

passthrough() {
  const realSocket = createConnection()
  // Fro actual connections, infer the address function.
  this.address = realSocket.address
}

respondWith(response) {
  this.address = () => addressFromResponse(response)
}

src/interceptors/ClientRequest/MockHttpSocket.ts

kettanaito · 2024-04-14T16:29:12Z

src/interceptors/Socket/MockSocket.ts

+      }
+    }
+
+    return super.address()


This will have incorrect (empty?) value for bypass scenarios because MockHttpSocket has no connection information.

I proposed to infer this from the original socket but now I wonder, socket.address() is generally available throughout the socket's life-cycle, yes? Like, you can grab the address at any point in time after constructing the socket. Hmm.

kettanaito · 2024-04-14T16:32:21Z

test/modules/http/compliance/http.test.ts

+});
+
+
+it.only('returns real socket address', async () => {


Oops, accidental it.only committed! Sorry, don't have an eslint for this.

mikicho · 2024-04-14T19:18:42Z

@kettanaito I fixed this as suggested. I still think we need to separate the socket-related logic from the HTTP-specific one if we want to support more protocols, but that can wait for another PR.

I have a couple of questions before it's ready for merge from my side:

Where should the tests I added be?
What do you think about current fake values? Do you have a better idea?

kettanaito · 2024-04-15T09:29:58Z

src/interceptors/ClientRequest/MockHttpSocket.ts

@@ -124,6 +124,7 @@ export class MockHttpSocket extends MockSocket {
   */
  public passthrough(): void {
    const socket = this.createConnection()
+    this.address = () => socket.address()


Technically, this would be safer:

Suggested change

this.address = () => socket.address()

this.address = socket.address.bind(socket)

In case this in the address() points to the socket instance, which I think it does. Let's preserve the context.

I think both ways preserve the context, but I agree that your solution is more explicit.

kettanaito · 2024-04-15T09:30:35Z

src/interceptors/ClientRequest/MockHttpSocket.ts

@@ -205,6 +206,13 @@ export class MockHttpSocket extends MockSocket {
      return
    }

+    // Return fake address information for the socket.
+    this.address = () => ({ 
+      address: '0.0.0.0',


What do you think about setting 127.0.0.1 as the fake address? Seeing localhost can hint people that this address is, indeed, mocked.

kettanaito · 2024-04-15T09:34:18Z

src/interceptors/ClientRequest/MockHttpSocket.ts

+    // Return fake address information for the socket.
+    this.address = () => ({ 
+      address: '0.0.0.0',
+      family: 'IPv4',


I wonder if there's any info in connectionOptions that we can use to set the family? 🤔

Yeah, there's .family and .host. Do you think we should respect those?

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 MockHttpSocket. You can leave the existing http.* tests as-is, those are useful too!

I think the address info resolved in the socket. I didn't verfiy this, but the connectionOptions includes only the port.
Also, please note the host !== address.

You're right. Let's infer the port only for now then?

kettanaito · 2024-04-15T09:34:51Z

test/modules/http/compliance/http.test.ts

+  const request = http.get('http://example.com')
+  request.once('socket', socket => {
+    socket.once('connect', () => {
+      connectPromise.resolve(socket.address())


It would be great to also have a test for socket.address() before the connect is emitted. I believe it should still be set?

It seems like the lookup happens after the socket event is emitted.

request.once('socket', socket => { console.log(socket.address()); // prints empty object {} });

Yes, this is correct. ClientRequest emits socket as soon as the socket is assigned, then the socket goes through its life-cycle, emitting lookup and connect.

…socket-address

kettanaito · 2024-04-15T21:39:24Z

src/interceptors/ClientRequest/MockHttpSocket.ts

@@ -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)


I think setting this.address should be done in mockConnect() for mocked responses. This will couple the .address() function withe actual address we emit in the lookup even so those two stay consistent (right now, they are not).

const address = { host: '...', family: '...', port: '...' } this.address = () => address this.emit('lookup', null, address.host, address.family, this.connectionOptions.host)

Wonderful! I'm ashamed that I didn't think about it myself.😅

You think of a ton of things that never crossed my mind 🙌 The power of collaboration.

kettanaito

This looks great! I've pushed a minor polishing to the tests, this should be ready for merge after CI passes.

wip

1fb5bf2

mikicho changed the base branch from main to feat/yet-another-socket-interceptor April 13, 2024 11:12

kettanaito reviewed Apr 14, 2024

View reviewed changes

src/interceptors/ClientRequest/MockHttpSocket.ts Outdated Show resolved Hide resolved

kettanaito reviewed Apr 14, 2024

View reviewed changes

mikicho added 2 commits April 14, 2024 22:14

fix

4257577

fix

8b81866

kettanaito changed the title ~~Michael/fake socket address~~ fix: add "address()" on mock Socket Apr 15, 2024

kettanaito reviewed Apr 15, 2024

View reviewed changes

fix

9c7623a

mikicho marked this pull request as ready for review April 15, 2024 18:14

Merge branch 'feat/yet-another-socket-interceptor' into Michael/fake-…

f9168d4

…socket-address

kettanaito reviewed Apr 15, 2024

View reviewed changes

mikicho and others added 3 commits April 16, 2024 17:32

fix

ce1a704

fix(MockHttpSocket): infer family from addressInfo

af55435

test: grab address host from actual url

1a0d5fb

kettanaito approved these changes Apr 16, 2024

View reviewed changes

kettanaito merged commit 66c6046 into feat/yet-another-socket-interceptor Apr 16, 2024
2 checks passed

kettanaito deleted the Michael/fake-socket-address branch April 16, 2024 15:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add "address()" on mock Socket #549

fix: add "address()" on mock Socket #549

mikicho commented Apr 13, 2024

kettanaito commented Apr 13, 2024

kettanaito Apr 14, 2024

kettanaito Apr 14, 2024

mikicho commented Apr 14, 2024

kettanaito Apr 15, 2024

mikicho Apr 15, 2024

kettanaito Apr 15, 2024

kettanaito Apr 15, 2024

mikicho Apr 15, 2024

kettanaito Apr 15, 2024

kettanaito Apr 15, 2024

mikicho Apr 15, 2024 •

edited

Loading

kettanaito Apr 15, 2024

kettanaito Apr 15, 2024 •

edited

Loading

mikicho Apr 16, 2024

kettanaito Apr 16, 2024

kettanaito left a comment

	this.address = () => socket.address()
	this.address = socket.address.bind(socket)

fix: add "address()" on mock Socket #549

fix: add "address()" on mock Socket #549

Conversation

mikicho commented Apr 13, 2024

kettanaito commented Apr 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikicho commented Apr 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikicho Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kettanaito left a comment

Choose a reason for hiding this comment

mikicho Apr 15, 2024 •

edited

Loading

kettanaito Apr 15, 2024 •

edited

Loading