msw: Using "onUnhandledRequest": "error" does not fail tests

@sbdchd I have the same problem and at least for my very simple test suite the following hack worked:

onUnhandledRequest: (req) => {
        test(`${req.url} is not handled`, () => {});
      },

test here comes from jest and jest doesn’t allow for tests to be nested or else it will fail with an error message. Coincidentally this will only happen when you have unhandled requests, so in order to know which request failed I pass the url to the test’s name. Could you please let me know if this works for you?

@kettanaito Thanks, it’s just a thought at the moment.

TLDR; the v1.1.0 update release a few days ago resolves the issue for us.

I assume by messed up you’re meaning the stack trace will not point you to the exact line in the test that has a API call with a missing response. Yes this is true however, Jest will report the test that failed. I did a quick test with jest for proof of concept:

import {afterEach, describe, it} from "@jest/globals"
let throwMe

describe('Some tests', () => {
  afterEach(() => {
    if (throwMe) {
      throw new Error('You failed')
    }
  })

  describe('when some condition 1', () => {
    it('will have the expected result', async () => {
      // Test
      throwMe = true
    })
  })

  describe('when some condition 2', () => {
    it('will have the expected result', async () => {
      // Test
      throwMe = false
    })
  })
})

Output from the test

 FAIL  tests/ui/testThrow.spec.js
  Some tests
    when some condition 1
      ✕ will have the expected result
    when some condition 2
      ✓ will have the expected result (1 ms)

  ● Some tests › when some condition 1 › will have the expected result

    You failed

       5 |   afterEach(() => {
       6 |     if (throwMe) {
    >  7 |       throw new Error('You failed')
         |             ^
       8 |     }
       9 |   })
      10 |

      at Object.<anonymous> (tests/ui/testThrow.spec.js:7:13)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        0.765 s, estimated 1 s

This may not be the same for all test frameworks.

That aside, since encountering this issue (about 4 days ago) I noted that an updated for MSW has been released V1.0.0 > V1.1.0 3 days ago. I’ve done the update and I can confirm that the update resolves this issue for my team; the following now causes tests to fail when there are unhandled requests:

beforeAll(() => server.listen({onUnhandledRequest: 'error'}))

Which yields something like

Error sending the query 'myQuery' FetchError {
      message: 'request to http://core.test.lan/app/graphql failed, reason: [MSW] Cannot bypass a request when using the "error" strategy for the "onUnhandledRequest" option.',
      type: 'system',
      errno: undefined,
      code: undefined
    }

The stack trace allows us to dig down into the library calling the API library where the call came from.

Hey, @balzdur. There hasn’t been any additional investigation than the one above.

I did stumble upon what seems the behavior we want when rewriting the ClientRequest interceptor but I don’t have tests to confirm that.

Anyone is welcome to dive into this to see how we can support failing individual tests (or the entire test suite) whenever onUnhandledRequest is set to error.

Note on semantics

I wonder if such test failures are semantic in regards to your assertions. Think of it this way: if your test setup (i.e. the beforeAll hook) throws, it’s not your individual tests that are marked as failed, it’s the entire test run, with the exception originating from the problematic area (i.e. the hook).

Since MSW is, effectively, a party of your testing setup, I’d say that it makes more sense to fail the entire test suite rather than try to figure out how to associate requests with the test code that issued them (spoiler: that’s likely impossible).

That being said, it still poses a challenge due to how the library is architectures. As the first step, I’d try propagating the exception from onUnhandledRequest to server.listen90 scope, which is the direct scope used in your test setup.

Looking into how other libraries deal with this, nock, for example, throws a direct exception within the test. It can afford that because you’re using nock.disableNetConnect() directly in your test file’s scope:

// my.test.js
const nock = require('nock')
nock.disableNetConnect()

That, however, doesn’t fail the test, it exits the process. This is a disruptive action and I’d much prefer for tests to continue running and only the affected tests to be marked as failed.

@kettanaito hmm but how is this different than using the onUnhandledRequest property and passing a callback? I tested it like this:

server.events.on('request:unhandled', () => {
      throw new Error('Must handle request');
    });

and somehow it’s not even executed 🤔
But anyway throwing an error inside the onUnhandledRequest handler doesn’t propagate to the test scope 😢 Isn’t this what this issue is about? Only by dynamically adding another test call in the error handler I can make jest fail.

To give a little more context about why one would like to make tests fail on unhandled requests. Your point here makes a lot of sense to me:

Eventually, the test will fail on your assertions. If it doesn’t, then the mocked request has no effect over the expectations that you’re testing

But… In reality multiple developers work on the same code base and someone might add an unhandled call to some of the components under test. The developer who first wrote the test could not have predicted this and now there might be unexpected changes in the way his code is executed. Even if those changes don’t make the tests fail I see this as a problem because at some point they might. E.g. if someone adds another request which depends on the first unhandled request he has now the burden of mocking both requests. Usually the one who added an unhandled request to the codebase is also the one who knows how to mock it so I think the burden should be on him to do this. I think that’s why you should have the option to make tests fail when this happens. I don’t think it should be the default but having the option would be nice.

I also wonder what would happen if MSW responds to unhandled requests with 500 (or 405 if that makes more sense)

Users might handle those errors in their request libraries in multiple different ways e.g. by showing an error fallback or retrying requests or whatever… So I think if someone wants to have this behaviour then they could just implement this themselves via the onUnhandledRequest callback no?

I also wonder what would happen if MSW responds to unhandled requests with 500 (or 405 if that makes more sense)? That should halt individual tests, as that server error would propagate to the request client and cause an exception. Even if such an error is handled by the client, it still won’t produce the expected state. The only downside here is that false-positive tests are still possible with this approach.

Hello @kettanaito, what is the status of your investigation ?

The behavior you expect is reasonable and I think the library should also behave that way. At the moment I don’t see how we can achieve that, so we must research the options before moving forward. My discoveries above are a great starting point but they don’t spotlight any possible solutions to this issue. We may want to look into how other similar libraries are failing tests, I’m sure they have their analogs of onUnhandledRequest: "error".