mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
537 stars 123 forks source link

fix(ClientRequest): support throwing custom errors in request listener #453

Closed mikicho closed 11 months ago

mikicho commented 11 months ago

@kettanaito

  1. I throw a simple object with code property. Do we have a more solid Error object for tests?
  2. I mainly used trial and error to determine that this.afterConnectionPhase = true belongs there. Please confirm if this is correct.

closes #452

mikicho commented 11 months ago

@kettanaito Fixed. PTAL

kettanaito commented 11 months ago

Okay, I've pushed some refactoring to the error suppression logic so it behaves as we expect and account for the support of throwing network connection errors from the request listener. Explaining the changes below...

kettanaito commented 11 months ago

I suspect that the failing CI has to do with the network on GitHub Actions:

 FAIL  modules/http/compliance/http-errors.test.ts > forwards ENETUNREACH error for a bypassed request
AssertionError: expected 'ENETUNREACH' to be 'EHOSTUNREACH' // Object.is equality
 ❯ modules/http/compliance/http-errors.test.ts:131:29
    129|   const requestError = await errorPromise
    130| 
    131|   expect(requestError.code).toBe('EHOSTUNREACH')
       |                             ^
    132|   expect(requestError.syscall).toBe('connect')
    133|   expect(requestError.address).toBe('2607:f0d0:1002:51::4')

  - Expected   "EHOSTUNREACH"
  + Received   "ENETUNREACH"

This same test produces the correct error locally. Will look into it...

Edit: This turns out to be an OS issue. Ubuntu results in ENETUNREACH while MacOS in EHOSTUNREACH when requesting the same unreachable address. I'm setting the CI job to run on MacOS because it's the machine I'm using.

mikicho commented 11 months ago

As long as the state doesn't become too complicated, I think it's a great solution! Thanks!! LGTM

kettanaito commented 11 months ago

As long as the state doesn't become too complicated, I think it's a great solution!

I will keep an eye on this part of the code to make sure it doesn't grow out of hand. I think we should be covered now.

mikicho commented 11 months ago

I'm working with WSL, and I get these errors: image

kettanaito commented 11 months ago

@mikicho, which version of Node.js is that? Also, I think the second error is related to the OS you're using.

I think both of those errors will be gone on Node.js 18.14.x and MacOS. Since they concern this project in particular, I highly recommend either upgrading to those or ignoring those failures since they are system-specific (look at the CI as the source of truth).

mikicho commented 11 months ago

which version of Node.js is that? Also, I think the second error is related to the OS you're using.

20.6.1

Should we attempt to replicate this behavior using a more dependable error code? (e.g. ECONREFUSED)

kettanaito commented 11 months ago

Perhaps we should attempt to replicate this behavior using a more dependable error code? (e.g. ECONREFUSED)

Not sure what you mean here. Can you give an example of how we can achieve this substitute?

I'm not proficient with network error codes but they seem to stand for quite different things. ECONREFUSED means the connection was OK DNS-wise but was refused by the server. It appears to be a different error than EHOSTUNREACH, where the requested host is outside of the current network (but the host itself and the network are OK).

kettanaito commented 11 months ago

I've just ran the test on Node.js v20.6.1 and saw that it groups ECONNREFUSED into an AggregateError that doesn't have the syscall or address properties. I've adjusted those assertions accordingly. All tests are passing locally on that version of Node.

@mikicho, I'd still highly recommend nvm and use v18.14.x because we don't officially support Node.js v20 in Interceptor just yet.

mikicho commented 11 months ago

This is with 18.14.2. The OS acts differently (I have WSL Debian) image

Not sure what you mean here. Can you give an example of how we can achieve this substitute?

I mean, we want to make sure we can throw suppressed errors from the request handler, and capture them before the request is sent. maybe we can use a more reliable error, which acts the same across OS

kettanaito commented 11 months ago

@mikicho, I fixed that assertion to pass on Ubuntu. Also, looks like your branch has to be pulled to resolve the second failing assertion. Let me know how it goes!

mikicho commented 11 months ago

image

kettanaito commented 11 months ago

Released: v0.25.7 🎉

This has been released in v0.25.7!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.