Skip to content

various fixes to tests, along with groundwork for adding travis and coveralls support #776

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

Merged
merged 7 commits into from
Jul 12, 2015

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jul 11, 2015

  • upgraded nyc to the newest release, it has much nicer test output:

screen shot 2015-07-11 at 4 17 10 pm

- added .travis.yml, so that as soon as soon as (@brycebaril, or @mranney configure Coveralls, and Travis we can add some badges). - added a test for queue.js (which had poor coverage), in the process refactored the tests so that it's easier to split them into multiple files. - made it so tests are run with both the `hiredis` and the `javascript` parser -- this raised coverage significantly. - fixed a major bug with the test suite, once you hit the domains test all tests afterwords did not properly bubble errors.

Doing all this brought test coverage up to 86% not too shabby.

reviewers: @raydog @erinspice

@raydog
Copy link
Contributor

raydog commented Jul 12, 2015

There's a lot of good stuff in here. Do you feel there'd be value in moving node_redis to use a standard test runner, like mocha or similar? I always felt it strange that this library rolls it's own... Who knows what other bugs like the domain one exists in here...

@bcoe
Copy link
Contributor Author

bcoe commented Jul 12, 2015

@raydog I was also thinking it would probably be worth moving to mocha, or maybe node-tap which @isaacs' has been putting a ton of work into.

I'd be happy to take that on as round two of this work, I think it can be done separately though?

@erinishimoticha
Copy link
Contributor

Agree! Let's convert them to use mocha or similar. I'd love to help with the test conversion.

@bcoe
Copy link
Contributor Author

bcoe commented Jul 12, 2015

@erinspice @raydog, I've managed to get stuff running on travis. I'm going to go ahead and rebase/merge this, but let's open an issue to discuss moving things to mocha.

@mranney
Copy link
Contributor

mranney commented Jul 12, 2015

For sure, let's use something standard.

In case you are wondering why things are the way that they are, I had originally found it very hard to realistically mock redis-server, so I wrote my own "test suite" that used a real redis-server.

That was like 5 years ago, and I'm sure there are much better ways to do things now.

bcoe added a commit that referenced this pull request Jul 12, 2015
various fixes to tests, along with groundwork for adding travis and coveralls support
@bcoe bcoe merged commit 131f92b into master Jul 12, 2015
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.

4 participants