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

Http Server not closing after tests #178

Closed
martypdx opened this Issue Oct 11, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@martypdx
Copy link
Contributor

martypdx commented Oct 11, 2017

Hey I've been using chai-http for testing for quite some time and I'm 95% sure that it would correctly allow mocha to exist after the tests are run. Now though, it just causes mocha to hang after the tests. If I pass in a server (instead of a listening function) and explicitly close, it terminates correctly.

Below are steps for workaround I had to use to get it to work:

const chaiHttp = require('chai-http');
const http = require('http'); // (1) add a require for http
const assert = chai.assert;
chai.use(chaiHttp);

describe.only('http app', () => {

    // (2) create the server and pass that to chai.request
    const server = http.createServer(app);
    const request = chai.request(server);

    // (3) add an after that closes the server
    after(done => server.close(done));

    // (4) profit by having npm test terminate correctly!
    it('GET /greeting/name responds with greeting', done => {
@liesislukas

This comment has been minimized.

Copy link

liesislukas commented Oct 13, 2017

in test:

  before(async () => {
    process.env.PORT = await portfinder.getPortPromise({port: 10002});
    app = require('./../../../app');
  });

  after(async () => {
    require('./../../../app').stop();
  });

and in app:

...

let port = process.env.PORT || 8080;
let server = app.listen(port, () => {
  console.log('App listening on port %s, in environment %s!', port, _.toUpper(process.env.NODE_ENV || ''));
});

function stop() {
  server.close();
}

module.exports = server;
module.exports.stop = stop;
@cklanac

This comment has been minimized.

Copy link

cklanac commented Oct 14, 2017

@martypdx Mocha 4.0 changed the default behavior.
"By default, Mocha will no longer force the process to exit once all tests complete. This means any test code (or code under test) which would normally prevent node from exiting will do so when run in Mocha. Supply the --exit flag to revert to pre-v4.0.0 behavior"

See mochajs/mocha#2879

So, if you decide to upgrade to Mocha 4.x you'll need to add the --exit flag mocha --exit. Or manually close the server as @liesislukas suggested above.

...
"scripts": {
  "start": "node server.js",
  "test": "mocha --exit"
},
...
@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Oct 15, 2017

Thanks for the issue @martypdx and thanks for the responses @liesislukas and @cklanac.

I think all of the work-arounds seem valid; just depends on which one you'd be happiest with in your test suite.

Would anyone like to file a PR improving our documentation around this? Listing the alternative ideas? I'd be happy to merge such a PR!

@martypdx

This comment has been minimized.

Copy link
Contributor Author

martypdx commented Oct 18, 2017

Thanks for responses, makes sense.

I think one thing that might help from an API perspective is to expose the server as a prop on the request when using the listening function overload, or at least offer close method.

To effectively deal with this issue, you have to run your own http.createServer(app) in order to maintain a handle. I tend to create a single request via a dedicated module (side question: is there any reason not to share the superagent request across tests?), so this isn't that big a deal.

Generally though, having access to the server seems logical:

const chai = require('chai');
const chaiHttp = require('chai-http');
chai.use(chaiHttp);

const app = require('../../lib/app');
const request = chai.request(app);

after(() => request.server.close());
@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Oct 18, 2017

Yeah, actually this makes a lot of sense. I worry though - what of the ergonomics when using a non-express app, what will request.server be then? null?

@barraponto

This comment has been minimized.

Copy link
Contributor

barraponto commented Oct 18, 2017

@martypdx if you want to share the agent, use const agent = chai.request.agent(app). It is useful when you want to keep the cookies between the requests.

@barraponto

This comment has been minimized.

Copy link
Contributor

barraponto commented Oct 18, 2017

@martypdx @keithamus we could have something like request.server(server => server.close()).

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Oct 19, 2017

I see little point in making it a callback - as then we'd simply offer no assurance it got called, and no way to verify at runtime if it was doable. I suppose having request.server == null in other cases can at least be checked.

I'd welcome a PR which adds request.server alongside some tests, and documentation within the README.

@martypdx

This comment has been minimized.

Copy link
Contributor Author

martypdx commented Oct 19, 2017

@keithamus:

I worry though - what of the ergonomics when using a non-express app, what will request.server be then? null?

Do you mean a string hostname like chai.request('http://localhost:3000')? The app param is a handler at the node http server level, nothing express specific.

I did notice that the chai.request.agent(...) method already stores the app argument as this.app, which includes functions converted to httpServer. Perhaps a better approach would be to add .app to the returned request object and the documentation story is that .app is whatever got passed in as an argument, unless it was a function it which case .app is the created httpServer.

Whether .app or adding a specific .server, you know chai http audience better than I do. Just good to make .request() and request.agent() the same.

Happy to help with PR

@barraponto:

want to share the agent, use const agent = chai.request.agent(app). It is useful when you want to keep the cookies between the requests.

Cookies? Aren't those just used by ads, invasive trackers, and tedious oauth calls? 😉

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Oct 19, 2017

Do you mean a string hostname like chai.request('http://localhost:3000')? The app param is a handler at the node http server level, nothing express specific.

Sorry, yes, I meant strings.

@kswope

This comment has been minimized.

Copy link

kswope commented Oct 26, 2017

@martypdx 'workaround' seems to me to be not a workaround but the proper way.

The change to mocha exposed a flaw in this lib's api, although it seems convenient, having request start a listening server by itself and then relying on mocha to murder it when its no longer wanted is a bad plan.

I think just changing the documentation, and maybe deprecating passing a requestListener function has my vote. Somehow exposing the internally started http.Server, otherwise returning a null if an an address had been passed to request smells funny to me.

With those changes, if I understand the code correctly, the only reason the http.Server is being passed in is to build the address string that would have been passed in.

So the docs would read something like:

chai.request() takes a URL

chai.request('http://localhost:3000')

If you are running a express server, as a convenience, you can pass in the server itself and the URL is obtained automatically from server.address().address and server.address().port ( hence doesn't work with koa or hapi )

chai.request(server)

keithamus added a commit that referenced this issue Oct 30, 2017

feat(node-request): close connections after request
This automatically closes server connections after making a request; so
that test runners (like mocha 4) aren't left hanging after the test
execution. If someone really needs to keep the server open,
`.keepOpen()` can be used.

Fixes #178

BREAKING CHANGE:

This change closes servers down after every request. If you want to use
the server for reasons at the same time as your test suite or for some
other reason you dont want the server to automatically be shut-down,
then you'll need to change any `chai.request` callsites to also use the
`keepOpen` comand, like so:

```js
chai.request(server).keepOpen()
```

@keithamus keithamus referenced this issue Oct 30, 2017

Merged

Fix 178 #184

keithamus added a commit that referenced this issue Oct 31, 2017

feat(node-request): close connections after request
This automatically closes server connections after making a request; so
that test runners (like mocha 4) aren't left hanging after the test
execution. If someone really needs to keep the server open,
`.keepOpen()` can be used.

Fixes #178

BREAKING CHANGE:

This change closes servers down after every request. If you want to use
the server for reasons at the same time as your test suite or for some
other reason you dont want the server to automatically be shut-down,
then you'll need to change any `chai.request` callsites to also use the
`keepOpen` comand, like so:

```js
chai.request(server).keepOpen()
```

jridgewell added a commit to ampproject/error-tracker that referenced this issue Nov 1, 2017

jridgewell added a commit to ampproject/error-tracker that referenced this issue Nov 1, 2017

Update dependencies to enable Greenkeeper 🌴 (#27)
* chore(package): update dependencies

* docs(readme): add Greenkeeper badge

* Update lockfile

* Close server after tests

Fixes chaijs/chai-http#178, caused by mochajs/mocha#3044.

ahashem added a commit to ahashem/sern that referenced this issue Jan 20, 2018

stuajnht added a commit to turrone/turrone-server that referenced this issue Jan 22, 2019

Changed `mocha` to `mocha --exit`
When using `chai-http`, `mocha` doesn't seem to exit but instead waits.

There seems to be some workarounds (chaijs/chai-http#178), but adding this argument is the easiest for now until more code is written.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment