Add codec validators to record parser and builder for all formats. #1447
Conversation
Also removed `decorator` dependency on tests Fixes #1443
|
@asdaraujo FYI, I removed the decorator dependency here. |
There was a problem hiding this comment.
The producer has a check on the application level, forgot to test it on the consumer side.
I'm not very familiar with this code, but from your description there's a check on the producer side, and I wonder if this should be removed since you're adding it at the record parser/builder, so it should apply to both producers and consumer? That way it's not duplicate code to maintain?
| @@ -73,6 +72,7 @@ def kafka_consumer_factory(kafka_broker, topic, request): | |||
| def factory(**kafka_consumer_params): | |||
| params = {} if kafka_consumer_params is None else kafka_consumer_params.copy() | |||
| params.setdefault('client_id', 'consumer_%s' % (request.node.name,)) | |||
| params.setdefault('auto_offset_reset', 'earliest') | |||
There was a problem hiding this comment.
do we really want this? the default in the consumers is normally 'latest' and I'm a little concerned that this sets a default for test purposes that does not match normal behavior.
There was a problem hiding this comment.
It gave me problems, at least. In tests, you set up by sending messages before consuming. So having a latest setting require you to connect before set up, which is not what you want in several places.
There was a problem hiding this comment.
Understood.
For my in-house tests of our internal wrapper, I did the connect-then-send messages and while slightly more verbose, it hasn't been too onerous.
That said, I'm fine with going this route instead.
|
@jeffwidman Valid point actually. I do like it symmetrical, but really quite confusing to have a code, that should raise an error, but some assert is raised instead. |
| @@ -47,6 +51,23 @@ def test_kafka_consumer(simple_client, topic, kafka_consumer_factory): | |||
| kafka_consumer.close() | |||
|
|
|||
|
|
|||
| def test_kafka_consumer_unsupported_encoding( | |||
There was a problem hiding this comment.
please also add a skip if no version is set -- see #1453
|
Looks like this one needs conflicts resolved (sorry!) |
|
Merged manually |
Also removed
decoratordependency on testsFixes #1443