-
Notifications
You must be signed in to change notification settings - Fork 71
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
tests, avro: add test on invalid reference schema #966
tests, avro: add test on invalid reference schema #966
Conversation
c597c91
to
646c1bd
Compare
646c1bd
to
b7b29d0
Compare
schema_type=SchemaType.AVRO, | ||
message_type=MessageType.schema, | ||
# expected_exception=CorruptKafkaRecordException, | ||
expected_exception=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosahama I think this is suspicious, the test is passing with no exception raised and no warning logged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I follow correctly, is this a new test or an existing one?
Also the refactoring makes it a bit hard to know what's going on, why are we using the MultipleMessagesHandlingErrorTestCase
for the existing tests, as you have already split out the references test from those existing ones, then the changes here should only concern the references test stub and it should be fine leaving the initial test case as it was: KafkaMessageHandlingErrorTestCase
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine just sending single test cases and probably using different test functions for the grouping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosahama Pushed a test refactored to be easier to follow hopefully :)
The refactored test is test_invalid_protobuf_schema_because_referencing_a_corrupted_schema
.
If it looks good to you then I'll refactor all other tests as well
expected_exception=CorruptKafkaRecordException, | ||
# expected_exception=None, | ||
# expected_warn_message='TODO', | ||
expected_warn_message=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosahama No warnings here, should I add one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice, but depends on if we have an explicit handling of this particular case, i.e. maybe we catch an error and we can log there, else I think the log might need to be generic somehow 🤔
], | ||
), | ||
MultipleMessagesHandlingErrorTestCase( | ||
test_name="Invalid AVRO schema because referencing a corrupted and syntactically invalid JSON", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nosahama Can you also review exceptions raised and warnings logged in this test case too (e.g. InvalidReferences
vs CorruptKafkaRecordException
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within scope of the schema reader, I think InvalidReferences
should end up being raised as CorruptKafkaRecordException
and then we shutdown if the strict mode is on.
MultipleMessagesHandlingErrorTestCase( | ||
test_name="Invalid AVRO schema because referencing a corrupted but syntactically valid JSON", | ||
inner_test_cases=[ | ||
# Given an invalid AVRO schema (corrupted but syntactically valid JSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the comments, takes me back to BDD with behave.
Similar to #962, but for AVRO.
This depends on the following external PR (needs to be merged first): #926
The base branch is avro_references2-rebased , which is the PR branch rebased on latest main.
Note that I have refactored the PROTOBUF test added in #962 to work in a single parametrized test case together with the AVRO ones.
Less code duplication but maybe added complexity :-)