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

Add API tests #227

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add API tests #227

wants to merge 2 commits into from

Conversation

MeLlamoPablo
Copy link

@MeLlamoPablo MeLlamoPablo commented Oct 29, 2017

This commit adds API tests, and makes some modifications to already existing logic:

  • Add an export called closeConnection to utils.js to allow closing thedatabase connection. This makes the test runner not get stuck, since supertest closes the server on each request, but can't close the database connection.
  • Make app.js and utils.js not log anything to the console if in a test environment in order to keep the test runner output intact.

Closes #220.

This commit adds API tests, and makes some modifications
to already existing logic:

- Add an export called closeConnection to utils.js to allow
closing thedatabase connection. This makes the test runner
not get stuck, since supertest closes the server on each
request, but can't close the database connection.
- Make app.js and utils.js not log anything to the console
if in a test environment in order to keep the test runner
output intact.
@bluzi
Copy link
Owner

bluzi commented Oct 29, 2017

Hey @MeLlamoPablo, thanks!
I need a few days to review your code, and I may want to change a few things, but overall it seems great.

Just one thing pops to my head now: Shouldn't we close the database connection every time the Express app closes? I mean, it has nothing to do with the tests, it should be a regular behavior.
Don't you think?

@MeLlamoPablo
Copy link
Author

Shouldn't we close the database connection every time the Express app closes?

I don't see why not. The important thing here is not the database, IMO, it's listening to SIGINT and/or SIGTERM, and closing the server there, so a running server doesn't stop processing connections after the signal is received.

With vm this is not too much of a big deal, but with platforms like Heroku, they will restart your processes once a day, so probably a good thing to have.

@bluzi
Copy link
Owner

bluzi commented Oct 31, 2017

I agree.

Could you do that and also move the database connection termination part to the app itself?

I'll merge it afterwards.

@MeLlamoPablo
Copy link
Author

I'm sorry, but I'm very busy at the moment. I did this because I wanted to practise API testing (and also wanted the Hacktoberfest shirt, not gonna lie), but I can't really dedicate more time contributing here :(

You'd be better off opening a separate issue and having someone else take over

@bluzi
Copy link
Owner

bluzi commented Nov 1, 2017

No problem @MeLlamoPablo, thank you for your contribution, it's more than enough :)

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

Successfully merging this pull request may close these issues.

Add tests for API routes
2 participants