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

Improve Tests, fix connection error timeout, other issues #158

Merged
merged 51 commits into from
May 7, 2014

Conversation

wizzat
Copy link
Collaborator

@wizzat wizzat commented Apr 9, 2014

This is the initial pull request for improving the test suite. There are a few high level things that happened:

  • Removed test support for Python 26 since it's been broken forever.
  • Test runner moved from py.test to nose for coverage support
  • Split out test_unit into component parts
  • Moved away from binary blobs in test towards struct.pack with comments
  • Fixed intermittent failures from test_unit
  • Added python-snappy to tox so that snappy tests actually run
  • Added test util to created shared code in test suite
  • Refactored away _get_conn_for_broker instead of writing a test for it
  • Added timeout to _get_conn
  • Created a new KafkaError instead of just throwing a raw Exception

@wizzat wizzat mentioned this pull request Apr 9, 2014
@wizzat
Copy link
Collaborator Author

wizzat commented Apr 9, 2014

The travis CI build will need updated to not manually run the test suite, but instead to rely on tox to run it for you.

@wizzat
Copy link
Collaborator Author

wizzat commented Apr 9, 2014

Alternatively, you can manually list all of the files in the directory.

@wizzat
Copy link
Collaborator Author

wizzat commented Apr 18, 2014

............S..................................
Name                Stmts   Miss  Cover   Missing
-------------------------------------------------
kafka                  12      0   100%
kafka.client          196     34    83%   164-167, 184, 190-194, 235-236, 314, 342, 359, 366-380, 385-398
kafka.codec            70      4    94%   11-12, 64, 120
kafka.common           45      0   100%
kafka.conn             84      4    95%   79, 89-90, 112
kafka.consumer        316     63    80%   94-96, 99-104, 130-156, 167-168, 172-173, 245, 258, 265, 279-280, 293, 297, 304, 309-310, 335, 344, 393, 404, 446-450, 464-507, 565, 585
kafka.partitioner      22      4    82%   25, 38-39, 44
kafka.producer        115     32    72%   34-75, 165, 199, 208, 234, 246, 257
kafka.protocol        219      0   100%
kafka.util             73     27    63%   24, 28, 32, 40, 84-95, 98-99, 102-108, 111-116
-------------------------------------------------
TOTAL                1152    168    85%
----------------------------------------------------------------------
Ran 88 tests in 147.391s

@wizzat
Copy link
Collaborator Author

wizzat commented Apr 18, 2014

The tests should be stable now, and the three intermittently failing tests should be stable now. The tests should also be significantly faster. Merging this branch in will likely require changing the travis environment. I'd like to move kafka-src to kafka-src-0.8.0 and introduce a new submodule for kafka-src-0.8.1.

@maciejkula
Copy link
Contributor

Could we rename this to indicate that this fixes the connection timeout bug as per #161? That makes it sound more like the critical bug fix that it is. I could then close #161.

@wizzat
Copy link
Collaborator Author

wizzat commented Apr 22, 2014

I can rename the pull request, but the overwhelming bulk of the pull request is about adding and fixing the test suite. As to when it will get merged: not up to me. Probably for the best because I would push ALL THE CHANGES! ;-)

@wizzat wizzat changed the title Add tests Improve Tests, fix connection error timeout, other issues Apr 22, 2014
@wizzat
Copy link
Collaborator Author

wizzat commented May 1, 2014

So, the Travis build works now that I've disabled automatic checks of Pypy and Python2.6. I'll investigate travis-ci + tox + multiple versions of Python.

from functools import partial
from itertools import count
from kafka.common import *
Copy link
Owner

Choose a reason for hiding this comment

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

This looks unnecessary? would like to avoid import *

@dpkp
Copy link
Owner

dpkp commented May 6, 2014

Can you check the Travis CI build failure?

@dpkp
Copy link
Owner

dpkp commented May 6, 2014

This is a great PR. I've only added a few minor comments. Would merge immediately if we can get a passing Travis CI build.

@wizzat
Copy link
Collaborator Author

wizzat commented May 7, 2014

I'll try to have a fix for this in the next day or so. I worked off and on at this while I was working today (KIXEYE is sponsoring the work after all), but I'll have more time to focus on it tonight on the train ride home. Most of the code review comments will be addressed in the next commit.

wizzat added 3 commits May 6, 2014 21:24
…or of in memory logging. Address code review concerns
kafka/client.py contained duplicate copies of same refactor, merged.
Move test/test_integration.py changes into test/test_producer_integration.

Conflicts:
	kafka/client.py
	servers/0.8.0/kafka-src
	test/test_integration.py
@wizzat
Copy link
Collaborator Author

wizzat commented May 7, 2014

Yay!

dpkp added a commit that referenced this pull request May 7, 2014
Improve Tests, fix connection error timeout, other issues
@dpkp dpkp merged commit b47bf78 into dpkp:master May 7, 2014
@wizzat wizzat deleted the add_tests branch May 7, 2014 09:56
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