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

Refactor tests #28

Closed
wants to merge 8 commits into from
Closed

Refactor tests #28

wants to merge 8 commits into from

Conversation

sandello
Copy link
Contributor

@sandello sandello commented Jun 2, 2013

Hi there.

I am looking towards using this library so I decided to look through and (possibly) adapt to my needs. As the first step I decided to run tests (on Mac OS X) and I have encountered some problems (Kafka was not starting, unittest module is not so convenient, etc).

So I have refactored tests a little bit. :)

Here is the list of major changes:

  • Configured tests to spawn a Zookeeper instance. I am not a Java developer, so I don't have Zookeeper running locally. :)
  • Extracted fixture code to the separate file, just not to mix test logic and fixture logic.
  • Updated CLASSPATH settings to reflect kafka-run-class.sh from Kafka distribution.
  • Configured tox and py.test to work with all that stuff.
  • Marked unit tests as "xfail", just to silent them.
  • Added has_gzip() / has_snappy() methods to not to fail on Snappy tests.
  • Did some PEP8 styling by the way.

@mumrah
Copy link
Collaborator

mumrah commented Jun 3, 2013

@sandello, this is a pretty big patch so it will take me some time to review :)

@sandello
Copy link
Contributor Author

sandello commented Jun 3, 2013

No worries.

BTW, why 'kafka-python', and not 'python-kafka'?

@mumrah
Copy link
Collaborator

mumrah commented Jun 3, 2013

There is another python client called pykafka, and I didn't want to cause any confusion.

@mumrah
Copy link
Collaborator

mumrah commented Jun 3, 2013

After a quick glance through the commits a few thoughts:

  • Overall, I like the changes. The tests are one area I know needs improvement, so a lot of this is much needed
  • I don't like introducing tox as a dependency for testing. It's fine to include a tox descriptor that works with our tests, but I always want to be able to run the tests with just "python"
  • The classpath in kafka-run-class.sh is pretty messed up. As you saw, it has dependencies on local ivy cache, and it also includes a bunch of stuff that's not really needed. After the 0.8 release, it will hopefully improve
  • +1 for ZK fixture
  • +1 for splitting out fixtures from tests

More detailed review to follow

@sandello
Copy link
Contributor Author

sandello commented Jun 4, 2013

On (2): Tox is not a dependency, actually. It is just a convenient way to run tests under py26 and py27.
I personally run tests as py.test, because it provides nice filtering facilities and also separates stdout/stderr for different tests. After I am done running the test I do a "pre-push" run with tox to be sure that py26 also works. That's all.

Still, nothing prevents you from doing just python ./test/test_integration.py. It is more a matter of how you write tests (e.g. you don't use pytest magic fixtures and stick with plain ol' unittest library).

On (3): Yeah... I can think only of invoking kafka-run-class.sh directly to not to mess with ivy cache directly.

@mumrah
Copy link
Collaborator

mumrah commented Jun 4, 2013

  1. It seems to be a dependency in that you've integrated it into setup.py as the default test runner
  2. The only other option would be to check-in the Kafka libs into source, which is not appealing. I think your patch is a good solution until Kafka sorts out their build

@sandello
Copy link
Contributor Author

sandello commented Jun 4, 2013

  1. Yeah, but this is a development dependency, right? Not a runtime one.

@mumrah
Copy link
Collaborator

mumrah commented Jun 4, 2013

Sure, but what I'm saying is, I don't want it as a development dependency :)

@sandello
Copy link
Contributor Author

sandello commented Jun 4, 2013

Okay, I don't mind about dropping it. I am not quite familiar on how to use python setup.py test when you don't use neither py.test nor tox. Shall I just drop the extra code and write a few lines in README saying that we are tox & py.test-friendly?

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.

2 participants