fix(ClientRequest): pass custom agent options in passthrough #701
Conversation
| }) | ||
| request.end() | ||
|
|
||
| await expect(waitForClientRequest(request)).resolves.not.toThrow() |
There was a problem hiding this comment.
My only concern is that this test doesn't make it clear what's being asserted. Is the expectation for the request to complete without errors? Did it not do that before? We need to define the expectations here clearer, I think.
There was a problem hiding this comment.
I see your point.
If the options are not propagated correctly, the request will fail due to its HTTPS nature, as we did not provide a certificate.
There was a problem hiding this comment.
I think I will remove this test. The option forwarding is implies in the normalize* function tests, which you've added.
|
This looks good! I've pushed little changes and also added this forwarding for HTTP agent as well. I think tests could do a bit better, let me know what you think. |
|
|
||
| const socket = new MockHttpSocket({ | ||
| connectionOptions: options, | ||
| createConnection: createConnection.bind( | ||
| this.customAgent || this, | ||
| options, | ||
| createConnectionOptions, |
There was a problem hiding this comment.
We don't really have any direct tests for this. Is this a reason for us to add agents.test.ts and test these things on the unit level?
There was a problem hiding this comment.
I saw your merged it, but I'm wondering if we need here a unit test to make sure we merge the options correctly.
I tested the normalize function, but I'm not sure if it's enough
There was a problem hiding this comment.
Ideally, we can have a follow up PR that unit-tests the agents. I think that's the right surface to do it. Does this makes sense to you?
There was a problem hiding this comment.
Yes. I opened an issue for this, so we won't forget about this:
#703
Released: v0.37.6 🎉This has been released in v0.37.6! Make sure to always update to the latest version ( Predictable release automation by @ossjs/release. |
resolves: nock/nock#2828