Skip to content

Conversation

@turtlesoupy
Copy link
Contributor

This is an update to #91 that properly adds my new commit on top of the upstream changes (I had accidentally performed a rebase instead of merge that put my branch in a bad state).

@turtlesoupy
Copy link
Contributor Author

@rdiomar - can you merge this in? I'm not a repo collaborator...

@turtlesoupy
Copy link
Contributor Author

Good point - added that exception too

@turtlesoupy
Copy link
Contributor Author

That push should address your comments (I made a method our of setUp() to make clear what it is actually doing).

@rdiomar
Copy link
Collaborator

rdiomar commented Jan 14, 2014

I understand that setUp is making sure we can create topics, but it's still not actually setting up anything. It's testing topic creation, which I believe belongs in a test method. The topic it creates is not used anywhere else, and it has no other side effects.

@turtlesoupy
Copy link
Contributor Author

@rdiomar it is creating a topic of name "self.id()[self.id().rindex(".")+1:]", which matches the name of each test method (e.g. test_multi_process_consumer). The majority of the test methods assume that producers/consumers can immediately access their topic, hence the necessity of warming the topic.

@rdiomar
Copy link
Collaborator

rdiomar commented Jan 14, 2014

Ah I see! Sorry I didn't get that earlier. That's also why you changed the topic names. How about this:

def setUp(self):
    # Set topic to test name
    self.topic = self.id()[self.id().rindex(".")+1:]
    ...

Then in the tests, always use self.topic instead of a string that matches the test name.

@turtlesoupy
Copy link
Contributor Author

Good idea, I've submitted an update (that also adds a bit of randomness to the topic name).

rdiomar added a commit that referenced this pull request Jan 14, 2014
Branch fix: No infinite loops during metadata requests, invalidate metadata more, exception hierarchy
@rdiomar rdiomar merged commit 9644166 into dpkp:master Jan 14, 2014
@rdiomar
Copy link
Collaborator

rdiomar commented Jan 14, 2014

Thanks @cosbynator!

@mrtheb
Copy link
Collaborator

mrtheb commented Jan 15, 2014

thank you both @cosbynator and @rdiomar ! 👍

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.

3 participants