nock / nock

HTTP server mocking and expectations library for Node.js
MIT License
12.74k stars 737 forks source link

replyWithError with object does not work since beta-8+ #2789

Open mikicho opened 1 month ago

mikicho commented 1 month ago

Please avoid duplicates

Reproducible test case

it('allows json response', done => {

Nock Version

14.beta-8+

Node Version

20

TypeScript Version

it('allows json response', done => {
    const scope = nock('http://example.test')
      .post('/echo')
      .replyWithError({ message: 'Service not found', code: 'test' })

    const req = http.request({
      host: 'example.test',
      method: 'POST',
      path: '/echo',
      port: 80,
    })

    req.on('error', e => {
      expect(e).to.deep.equal({
        message: 'Service not found',
        code: 'test',
      })
      scope.done()
      done()
    })

    req.end()
  })

What happened?

Copy of this to discuss about a workaround and follow the error from nock POV

Would you be interested in contributing a fix?

mikicho commented 1 month ago

@marikaner If it happens for interceptors with the replyWithError function, you can make sure you throw an Error object.

marikaner commented 1 month ago

Hey @mikicho, sorry for the delay. I am now looking into this again and one thing I noticed is that we almost never use .replyWithError() but rather .reply(500) or similar. Is this not intended?

With that I get the same error as in the original issue:

 TypeError: Reflect.has called on non-object
        at Reflect.has (<anonymous>)
      at getRawFetchHeaders (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/utils/recordRawHeaders.ts:277:16)
      at MockHttpSocket.respondWith (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/MockHttpSocket.ts:321:32)
      at Object.onResponse (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/index.ts:138:16)
      at handleResponse (../../node_modules/@mswjs/interceptors/src/utils/handleRequest.ts:55:21)
      at handleRequest (../../node_modules/@mswjs/interceptors/src/utils/handleRequest.ts:191:16)
      at _ClientRequestInterceptor.onRequest (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/index.ts:132:30)

But this is flaky, sometimes it happens sometimes it doesn't.

mikicho commented 1 month ago

@marikaner Interesting.. can you please send a reproducible code?

marikaner commented 1 month ago

I can't because it is flaky and I don't know which part of the code causes this. But I just found that this is not happening on errors exclusively.

One example:

    nock(baseUrl)
      .post('/SOME_PATH/$batch', new RegExp(body))
      .reply(202, response, {
        'content-type': 'multipart/mixed; boundary=someBoundary'
      });

I'll try to identify which requests this fails for, maybe I can recognize some pattern. At the moment I don't see it.

mikicho commented 1 month ago

Could you provide more details to help me understand the issue better? What do you mean flaky? maybe it's another issue that should be addressed on its own?

marikaner commented 1 month ago

With flaky I mean that it is does not seem to be deterministic, sometimes it fails with the above error, sometimes it doesn't fail at all. I am currently investigating, if this due to other tests interfering somehow. I assume you never stumbled upon that?

marikaner commented 1 month ago

@mikicho I cannot pinpoint what it is, but I found some aspects that I found surprising:

  1. The more tests I run the more likely it seems that the tests will fail with the above error. This includes tests that do not use nock at all.
  2. Our code and tests are written in TypeScript and we use ts-jest with jest to run the tests. I noticed that the error I see seems to be thrown from ../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/utils/recordRawHeaders.ts, while the error you pointed out comes from ../../node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js.
  3. I can almost never reproduce this locally, only when running it through GitHub workflows (but I use the same command and node version).

Unfortunately I think this doesn't help, but maybe sparks ideas.

mikicho commented 1 month ago

@marikaner, maybe it's related to something we fixed on the interceptors side. Can you please try with v14.0.0-beta.15?

marikaner commented 4 weeks ago

Hey @mikicho,

I was already using this version and it still has the same issue. However, I have good news: I was finally able to create a reproducible example for this issue.

Contributing factors to this issue are:

  1. You need a first test file, where you nock a request with a given host.
  2. In a second test file, run against the same host, but this time use an incorrect path.

Expected:

Nock: No match for request

Actual:

TypeError: Reflect.has called on non-object

In the reproducible example with older versions of nock the expected state occurs. In my productive code the request doesn't fail, that is among others why it was very hard to identify.

The test behaves as expected when I:

I configured jest, so that the tests run in alphabetical order, that is what makes this reproducible.

marikaner commented 4 weeks ago

A few more details that I found in the mean time: We had some tests, where we execute POST requests and prior to executing the actual POST we retrieved a CSRF token with HEAD. We used to fetch the token with GET, but changed it to HEAD at some point. In some tests, we forgot to adjust the nock, however:

  1. This still worked with nock < 14.beta-8 (which I guess it shouldn't).
  2. This still works with nock >= 14.beta-8, when run individually or in a "beneficial" order (see my previous comment).
marikaner commented 4 weeks ago

Maybe to point this out as well: Apparently in the older versions of nock, it didn't matter whether a path included a trailing slash or not. Now it seems to matter and the error in that case is the one with Reflect.has....

marikaner commented 4 weeks ago

I hope this doesn't count as spamming, but I found one more weird occurrence of this issue: Before e.g. POST requests we retrieve CSRF tokens with HEAD. In some cases we retry these once with a trailing slash once without. In my tests, when I use the same host for a different preceding test, I get the Reflect.has... error, because we forgot to add a second nock for the retry. However, when I change the host (without adding a second nock), the test passes!

For some of my findings (see above) I wonder how it ever passed, but this seems to be a common pattern throughout the failures that I experienced.

mikicho commented 4 weeks ago

I hope this doesn't count as spamming

Feel free to put in any relevant info you find :)

However, I have good news: I was finally able to create a reproducible example for this issue.

When I run it, I get another error, and I can't reproduce what you get. There is an error in the sequence name (- instead of .). Can you please fix and clean up this example and reduce the irrelevant parts/files?

TypeError: Reflect.has called on non-object

Do you have the stack trace? From what line does this error come?

marikaner commented 3 weeks ago

Hey @mikicho,

I fixed the typo in the file name. The example is already minimal to my knowledge. You need to run the tests sequentially with jest --runInBand to get the error.

I already shared the stack trace, it is always the same:

 TypeError: Reflect.has called on non-object
        at Reflect.has (<anonymous>)
      at getRawFetchHeaders (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/utils/recordRawHeaders.ts:277:16)
      at MockHttpSocket.respondWith (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/MockHttpSocket.ts:321:32)
      at Object.onResponse (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/index.ts:138:16)
      at handleResponse (../../node_modules/@mswjs/interceptors/src/utils/handleRequest.ts:55:21)
      at handleRequest (../../node_modules/@mswjs/interceptors/src/utils/handleRequest.ts:191:16)
      at _ClientRequestInterceptor.onRequest (../../node_modules/@mswjs/interceptors/src/interceptors/ClientRequest/index.ts:132:30)

The only new information now is how to reproduce it ;)