fix(ClientRequest): support ClientRequest constructor

[mikicho]

No description provided.

[kettanaito]

I actually grew to dislike this pattern. Sorry it's still present in the test suite. It leads to a shared state in a shape of the resolver function, and that's really bad.

May I please ask you to add the request listener within the test instead? It will yield a much simpler setup. Thanks!

[mikicho]

Sure! like this?

[kettanaito]

I think so!

And please add interceptor.removeAllListeners() to the afterEach() hook.

[kettanaito]

This looks great. I cannot believe we ignored the constructor itself. Thanks for fixing this, @mikicho!

I left one comment about the test, otherwise good.

[kettanaito]

I'm thinking, will patching ClientRequest lead to double patching when using functions like http.request()? We don't replace their implementation anymore, only inject the MockHttpAgent, which means those functions will construct new http.ClientRequest() when called. Now that we are adding a patch over that class, does that lead to both of them being patched?

I may be wrong about this. Would love to hear your thoughts on this. Also, I think we need tests for this anyway to have clear expectations toward how this should behave.

Since the ClientRequest proxy just drills down the agent, I think we should be fine. To be safe though, perhaps we should check if the options.agent is already an instance of MockHttpgent, and if it is, ignore the proxy. Does that make sense?

if (options.agent instanceof MockHttpAgent) {
  return Reflect.appy(...)
}

const agent = new MockHttpAgent()
return Reflect.apply(...)

[mikicho]

I'm thinking, will patching ClientRequest lead to double patching when using functions like http.request()?

It also came to mind, but when I removed the get/request proxies, Node kept using the original ClientRequest. I think it happens because they destruct the ClientRequest in the http module: https://github.com/nodejs/node/blob/main/lib/http.js#L30

[kettanaito]

That is a great find. I also thought of some cases when we should keep the .get/.request proxies to be safe. I like this approach. I would just

• Check if the mock agent is already set on the ClientRequest level and then ignore everything else.
• Add at least one test that ensures http.request + ClientRequest combo. We may have to assert on implementation details here because proxies are not publicly observable. Something like: if I make http.request request, grab the ClientRequest instance it constructs and make sure that it uses a single and same MockHttpAgent instance.

[mikicho]

Check if the mock agent is already set on the ClientRequest level and then ignore everything else.

In runtime or in a test?

a single and same MockHttpAgent instance

Same as what? We create a new MockAgent instance on each request.
Is this the test you have in mind?

it('..', async () => {
  const req = http.request(..);
  const clientReq = new ClientRequest(..);
  req.agent.name === clientReq.agent.name?
})

I'm not sure what we are trying to check here.