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

Slightly refactor & add tests for API config loading & redis setup #151

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

Conversation

rowanhill
Copy link
Contributor

Hi,

I really like I/O Docs, but the PRs I've submitted recently have been made a bit more nail-biting than necessary by the lack of tests. This PR adds some tests for a couple of (relatively trivial) areas of the code, and pulls those bits of code out into their own file.

This is obviously just a small start on a something bigger, but a) there's no reason it all needs to be done at once, and b) I wanted to check you're happy with this as a general approach.

I'd quite like to pull a few more bits out of app.js into their own testable modules (and put a few tests around some of them), and perhaps get some integration tests up and running (perhaps using supertest?) - would you be interested in more PRs along those lines?

Thanks,
Rowan

@phairow
Copy link
Contributor

phairow commented Oct 9, 2014

this looks great although the json loader seems unnecessary. our team has been using chai so unless there is a strong community preference, we should make that change. I'd be happy to merge as is and make those changes. what do you think @rowanhill?

@rowanhill
Copy link
Contributor Author

Hi, thanks for resurrecting this PR! Chai sounds absolutely fine to me, if you're happy to make the switch.

@phairow
Copy link
Contributor

phairow commented Oct 13, 2014

excellent, I'll make the necessary changes and get this merged.

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

Successfully merging this pull request may close these issues.

None yet

3 participants