Skip to content
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

Open
NinnOgTonic opened this issue Dec 11, 2018 · 18 comments
Open

Error middleware not properly invoked when using async functions #529

NinnOgTonic opened this issue Dec 11, 2018 · 18 comments

Comments

@NinnOgTonic
Copy link

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.

@knownasilya
Copy link

knownasilya commented Jan 15, 2019

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.

Not sure if #402 would help here. It does not.

@jonathansamines
Copy link

@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.

@NinnOgTonic
Copy link
Author

@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

acifani added a commit to acifani/node-currency-converter that referenced this issue Mar 9, 2019
Can't upgrade supertest to latest major because of
ladjs/supertest#529
@osdiab
Copy link

osdiab commented May 14, 2019

I am experiencing this too... anyone have a workaround?

@jonathansamines
Copy link

jonathansamines commented May 14, 2019

@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 .catch or a try/catch statement to the express next handler.

@jonathansamines
Copy link

@osdiab Can you check, if you are experiencing the same issue as @NinnOgTonic ? If that is not the case, please provide a reproduction example.

@osdiab
Copy link

osdiab commented May 14, 2019

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

@osdiab
Copy link

osdiab commented May 15, 2019

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(), but instead I had a normal function that just called next(). The discrepancy between the server and the test was because the actual function, rather than the mocked one, did invoke next() properly.

Next, the reason I was getting open handles was because, when you use supertest with koa, you need to create a server using yourKoaApp.listen(), but that also means you need to clean it up and close the server.

I think it's safe to say this issue should be closed.

@rogerzxu
Copy link

For anyone else having this issue, make sure that express-async-errors is in scope of where the tests are run. Simply adding require('express-async-errors') to the top of my test was enough to make this work.

@nishnat-rishi
Copy link

Is anyone facing this issue in express 5.x onwards? 5.x seems to have fixed the express-async-errors requirement, but the supertest issue persists (at least in my case).

@jugheadjones10
Copy link

@nishnat-rishi I’m facing the same problem even though my express is 5. Have you fixed the issue?

@isaachinman
Copy link

Also facing this issue with Express v5, and am happy to work on a PR if someone can point me in the right direction.

@jophish
Copy link

jophish commented Aug 17, 2022

I'm also running into this problem... Using Express v4 with the express-async-errors package does not seem to solve the issue.

@ShinJustinHolly3317
Copy link

ShinJustinHolly3317 commented Sep 22, 2022

anyone fix this issue?
or just cannot use express error handler , should use normal res.status(500).json(...) in every handler with if-else condition check?
Q^Q

@tfedawy
Copy link

tfedawy commented Oct 7, 2022

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.

@kelzenberg
Copy link

kelzenberg commented Jan 19, 2023

We solved the issue by adding the setupFilesAfterEnv attribute to the jest config and let it point to a file that includes (but not exclusively) the mentioned module import 'express-async-errors' at the top of the file.
This way, the module is always present in the app with the joined jest environment.

We also experienced that express versions higher than v4.17 might be not compatible with express-async-errors but this is unconfirmed yet.

@caiquecp
Copy link

caiquecp commented Jan 20, 2023

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.

@Elalfy74
Copy link

Elalfy74 commented Feb 8, 2023

Edited
I found an easier way
in index.js

import 'express-async-errors';
const app = express();

if (process.env.NODE_ENV !== 'test') {
  connectToDB().then(() => {
    app.listen(port, () => {
      logger.info(`Running at Port ${port}`);
    });
  });
}
export default app;

in your test

import app from '../../index.ts';

Old way:
The only way I found to fix this is to make two files index.js and server js.

I only tested it for an hour.
I don't know if it has downsides, but it is working fine for me

server.js

import 'express-async-errors';
const app = express();

middlewares(app);
routes(app);

export default app;

index.js

import app from './server.js';
app.listen(8000);

In your test

import app from './server.js'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests