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

Added code coverage and HighLevelProducer tests. #422

Merged
merged 4 commits into from Aug 3, 2016

Conversation

lightswitch05
Copy link
Contributor

@lightswitch05 lightswitch05 commented Aug 1, 2016

I added code coverage using istanbul. My tests were failing on Travis, after some research I found it was due to some changes with NPMv3 and peer dependencies with eslint-config-semistandard. After getting that working, I saw HighLevelProducer was in sore need of some tests, so I added those. They are pretty much the same test as Producer. Testing my changes some more with Travis I saw that proxyquire was not working, I changed the module require to a file to fix it.

I would be happy to add integration with https://coveralls.io/ if there is any interest to have that level of code coverage information with pull requests. It would require the project owner to allow integration with the account.

Summary: Added code coverage. Added unit tests. Fixed tests on travis.

@@ -18,7 +18,7 @@ describe('Client', function () {
before(function () {
zk = new FakeZookeeper();

Client = proxyquire('../lib/Client', {
Client = proxyquire('../lib/client.js', {
Copy link
Collaborator

@hyperlink hyperlink Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be able to omit the .js

Copy link
Contributor Author

@lightswitch05 lightswitch05 Aug 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right @hyperlink - I amended the commit and verified the build is still successful on Travis: https://travis-ci.org/lightswitch05/kafka-node/builds/148936864

@lightswitch05 lightswitch05 force-pushed the feature/add-code-coverage branch from 782222b to 1116e0a Compare Aug 1, 2016
@hyperlink
Copy link
Collaborator

@hyperlink hyperlink commented Aug 1, 2016

Thanks for the PR @lightswitch05!

I was thinking since one of the HLP's key feature is the automatic partitioning of messages maybe we can update the test to verify messages are produced to different partitions. I was thinking we can specify a KAFKA_CREATE_TOPICS in docker-compose.yml to include a topic with three partitions KAFKA_CREATE_TOPICS: "MyTopic:3:1". Then the test can publish messages to those partitions and verify they are consumed. What do you think?

@hyperlink hyperlink merged commit 907d696 into SOHU-Co:master Aug 3, 2016
@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Aug 3, 2016

Thanks for merging! I can definitely look into adding that extra test for HighLevelProducer. Unfortunately I have not had much free time to code this week. Any interest in adding coveralls.io integration?

@austinkelleher
Copy link

@austinkelleher austinkelleher commented Aug 3, 2016

I agree that adding coveralls would be a good idea.

@hyperlink
Copy link
Collaborator

@hyperlink hyperlink commented Aug 3, 2016

Yeah, lets do it! Just need to get @haio to activate it (or make me admin?)

@lightswitch05 lightswitch05 deleted the feature/add-code-coverage branch Aug 8, 2016
@lightswitch05
Copy link
Contributor Author

@lightswitch05 lightswitch05 commented Aug 8, 2016

Added in #432

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.

None yet

3 participants