Fix fixture order for consumer tests #898
Conversation
Use worker ids to determine when Kafka servers can be closed. This helps finding out which worker is stuck.
Consumer may get stuck on close if topic is deleted from broker before close. This change should make the stuck consumers on tests to be more rare.
jjaakola-aiven
force-pushed
the
jjaakola-aiven-fix-fixture-order-for-consumer-tests
branch
from
204f13a to
a477c0e
Compare
jjaakola-aiven
changed the title
| def test_partitions_for( | ||
| self, | ||
| new_topic: NewTopic, | ||
| consumer: KafkaConsumer, | ||
| ) -> None: |
There was a problem hiding this comment.
its actually the order of the fixture that causes the problem?
There was a problem hiding this comment.
this is reaaaaally bad, I would expect this to be uninfluent. The order of parameters in a function shouldn't change the semantics of a program.
I wasn't quite liking the idea of fixture but this is much more dangerous.
Shouldn't we create a higher order fixture that gives the parameters and enforces the right order?
IMO if we need a topological sort of the fixture generation we should enforce it by definition
eliax1996
deleted the
jjaakola-aiven-fix-fixture-order-for-consumer-tests
branch
| try: | ||
| yield consumer | ||
| finally: | ||
| consumer.close() |
There was a problem hiding this comment.
Just noting that this shouldn't have any impact, fixtures do not work like a context manager, and the pytest framework will resume the generator on clean-up. So the finally: here doesn't make any difference (but also doesn't hurt).
About this change - What it does
Consumer close can get stuck if the referenced topic is deleted from broker before.
References: #xxxxx
Why this way