Test Suite

[wizzat]

Hey,

So, I think the test suite is pretty widely regarded as being in total shambles. I personally haven't managed to make test_integration work at all, though I have proof-by-existence that the application does in fact work. Additionally, the test suite fails more often than it passes due to dict ordering issues and the snappy tests fail if python-snappy is installed.

Does anyone have any helpful hints about how to make test_integration actually work, or should we just comment it out for now? It seems like most of the problem is in spinning up zookeeper/kafka as external processes, so it might be reasonable to just rely on an existing cluster?

My gut tells me that the right course of action is to disable the integration testing until we have a better solution in place, turn on test coverage reporting, and then start moving tests out of the monolithic test_unit and into their own files. From there we can start looking to improve test coverage. I'll have a pull request going down this path Soon(tm) unless someone objects.

[mumrah]

TravisCI doesn't seem to have any problems with the integration tests (https://travis-ci.org/mumrah/kafka-python). If you follow the same steps as Travis, then hopefully it should work for you too.

The things that are brittle are how kafka-src is built and how the Python fixtures wait for various things to start up. For example, we'll have to switch over to using the Gradle build pretty soon.

The integration tests are very valuable since they actually test the code in a real world configuration. I'd rather not disable them.

As for breaking up the unit tests into separate files +1. Also +1 for adding some code coverage in there.

[mrtheb]

It would be interesting to know the kind of errors you are getting with test_integration. On my end, once the kafka-src thingies are set up, I have had rather good success in running them. The main difference is I am using nosetests instead of pytest as my runner of choice.

Timeouts and flakyness with external dependencies in integration tests are always a hassle.

[wizzat]

I get a combination of timeouts and things like this:

EError: Could not find or load main class org.apache.zookeeper.server.quorum.QuorumPeerMain
Exception in thread Thread-3:
Traceback (most recent call last):
File "/Users/mroberts/.pyenv/versions/2.7.6/lib/python2.7/threading.py", line 810, in __bootstrap_inner
self.run()
File "/Users/mroberts/work/forks/kafka-python/test/fixtures.py", line 121, in run
self.run_with_handles(stdout_handle, stderr_handle)
File "/Users/mroberts/work/forks/kafka-python/test/fixtures.py", line 169, in run_with_handles
raise RuntimeError("Subprocess has died. Aborting.")
RuntimeError: Subprocess has died. Aborting.

[wizzat]

It looks like I got things working via:

./sbt clean update package assembly-package-dependency

Damn the integration tests are slllloooooowwwwwwww. But since they're working (mostly, a few intermittent failures) I'll take a look at them as well.

These are the tests which seem to intermittently fail:

• test_integration.test_async_keyed_producer
• test_unit.test_encode_fetch_request
• test_unit.test_encode_produce_request

The two unit tests are failing due to non-deterministic ordering in dict.items(). I haven't investigated why test_async_keyed_producer is failing.

[dpkp]

A few months back omar hammered on the tests to fix a lot of the major issues. That was merged here: #88 . Since then they have worked fairly well for us.

[wizzat]

One more comment: I'd really like to move away from un-annotated binary strings in TestProtocol. Ideally it could act as some kind of documentation that would allow people who aren't familiar with the protocol to become familiar with it, but more importantly it will allow us to understand what our own changes have changed.

I see two ways of moving away from "\x024\x33\x75L\x81\xd41" != "...":

expected = (
    "\x00\x00\x00\x00\x00" # Offset
    "\x00" # Magic
    "\x00" # Flags
    ...
)

OR

expected = (
    struct.pack(">q", 0) # Offset
    struct.pack(">1s", 0) # Magic
    ....
)

I prefer the second. How about you?

[rdiomar]

Sounds good to me. It might make it easier to modify the tests if the protocol changes in the future. And of course it's neater and more meaningful.

[dpkp]

I vote struct.pack

[wizzat]

I'm moving forward with struct.pack. The branch for this lives here: https://github.com/wizzat/kafka-python/tree/add_tests

[wizzat]

As it turns out, none of the snappy tests are actually being executed because python-snappy isn't in the tox requirements. +/-1 to adding it to the tox requirements for testing? I'm proceeding on the assumption that this is ok unless someone says otherwise.

[mrtheb]

👍 for struct.pack and snappy requirement in tox

slooooowwwwness will be hard to get around, paralellize server startups maybe?

[wizzat]

The (first) pull request is here: #158

[rdiomar]

Selfishly, I would rather not have to deal with snappy as a dependency in my system which I don't plan on using. I think that's why it's always been optional.

[wizzat]

Well, it seems like we have some choices:

• Deal with the snappy dependency
• Only have snappy tests run if you run the test file directly from outside a test harness
• Allow the test harness (tox) to access global site-packages

My vote is for the first, especially for core developers.

[mrtheb]

1st too, snappy is just a tox dependency, it doesn't affect those who aren't using it

[wizzat]

Fixed in #158