-
Notifications
You must be signed in to change notification settings - Fork 759
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error middleware not properly invoked when using async functions #529
Comments
Same issue with https://www.npmjs.com/package/co-router Basically if the route doesn't handle the error and the app's error middleware does, that error is never caught by supertest.
|
@NinnOgTonic @knownasilya Do you have a reproduction case to take a look? I just tried to run all tests from https://github.com/davidbanham/express-async-errors and didn't get any errors. |
@jonathansamines Please view the following repository, i have written the source in TS, but also included the transpiled JS for the reproduction: https://github.com/NinnOgTonic/supertest-bug |
Can't upgrade supertest to latest major because of ladjs/supertest#529
I am experiencing this too... anyone have a workaround? |
@NinnOgTonic Sorry for the super-late response. I just checked your example, and the issue is not on supertest, but in express. Express doesn't know how to handle async/await nor promises by default. You have to forward any errors by either using |
@osdiab Can you check, if you are experiencing the same issue as @NinnOgTonic ? If that is not the case, please provide a reproduction example. |
My code is using Koa so sounds like probably not the same as his. I have an error middleware that does roughly the following try { next() } catch (err) { if (err instanceof MyError) { handle it } else throw err } Then I declare my routes with Koa router. Running on http is fine but running in jest with supertest causes thrown errors and 404s instead of the specific error codes it’s supposed to return I know the correct error instances are being thrown even in the test context bc I put in debugger calls where my tests should throw errors and the code hits those error paths and throws. But the result in jest is that the expectations don’t match, and the test doesn’t end even though I awaited on supertest; the jest test ends but handles are left open and I have to kill the process to exit and jest warns me about open handles I tried converting it to the old style supertest.end code but that didn’t change anything Sorry would show you more specific code but on a plane lol. Can make a small example repo to reproduce but maybe that gives you an idea of what might be happening |
Ahhhh I figured it out: The reason it was throwing was because in my test I was mocking out a middleware, but my mock implementation was bad - the Koa middleware needed to be an async function that awaits on Next, the reason I was getting open handles was because, when you use I think it's safe to say this issue should be closed. |
For anyone else having this issue, make sure that |
Is anyone facing this issue in express 5.x onwards? 5.x seems to have fixed the |
@nishnat-rishi I’m facing the same problem even though my express is 5. Have you fixed the issue? |
Also facing this issue with Express v5, and am happy to work on a PR if someone can point me in the right direction. |
I'm also running into this problem... Using Express v4 with the |
anyone fix this issue? |
Anyone found the root cause of this issue? I'm having the same problem and adding require('express-async-errors') to the top of my test didn't work. Using express v4. |
We solved the issue by adding the We also experienced that express versions higher than v4.17 might be not compatible with express-async-errors but this is unconfirmed yet. |
I have plenty of tests that actually triggers error handling with Express and supertest. But there's only one who is failling (with the same issues related here): the last route defined in a router. What is really curious: if I move the definition of this route up above (other route definitions) the error handler is called and the test works. Note: I'm not using express-async-errors, my async routes has try/catch calling next. OK, my problem was solved by answer on similar issue here: #788 (comment) I was using jest.useFakeTimers() in some test, an apparently we need to get it back with jest.useRealTimers() (you can call it on afterEach) or it might mess with event loop. |
Edited
in your test
Old way: I only tested it for an hour. server.js
index.js
In your test
|
When using supertest to test an express app together with the example in the readme from https://github.com/davidbanham/express-async-errors, it seems supertest will not properly invoke the error handling middleware, and thus the test just times out rather than actually responding with the logic defined in the error handler.
This behaviour works when invoking the code via HTTP.
The text was updated successfully, but these errors were encountered: