Improve code coverage for serialization #51419
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR increases test coverage across various serialization modules by adding new unit tests for schema validation, callback request types, asset decoding, data intervals, timezone encoding, and multiple serializer implementations.
- Added schema validation tests for
BaseSerialization.validate_schema - Expanded serialization/deserialization tests for callback requests, assets, and datetime-related types
- Broadened serializer tests (bignum, numpy, pandas, deltalake, kubernetes, timezone, JSON schema loader)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/unit/serialization/test_serialized_objects.py | Added tests for schema validation, callback requests, data intervals, hash, asset decoding, and timezone encoding |
| tests/unit/serialization/serializers/test_serializers.py | Added serializer tests for bignum, numpy, pandas, deltalake, kubernetes, timezone, and JSON schema loader |
Comments suppressed due to low confidence (2)
airflow-core/tests/unit/serialization/test_serialized_objects.py:329
- The new DeadlineAlert case uses
Nonefor the expected type and comparator, so it isn’t actually verifying serialization/deserialization behavior. Add the appropriateDAT.DEADLINE_ALERTconstant and a comparator (e.g., checkreference,interval, andcallback) to fully test this path.
( DeadlineAlert( ... ), None, None, ),
airflow-core/tests/unit/serialization/serializers/test_serializers.py:498
- The phrase "does not exists" is grammatically incorrect. Update the error message to "Schema file schema.json does not exist" and adjust the test accordingly.
assert "Schema file schema.json does not exists" in str(ctx.value)
airflow-core/tests/unit/serialization/test_serialized_objects.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Well done! Increasing Airflow's coverage is indeed quite important.
As a general rule of thumb, when there's no dependency between assertions - I think that it's better to seperate assertions to multiple tests (which could also be parametrized) - it will make it easier to detect exactly what assertions went wrong. Otherwise, when failing upon the first assertion within a test - the assertions below won't be checked.
|
Thanks for the review, good point about splitting the assertions. |
|
Hi @shahar1, I’ve refactored the tests per your suggestion, thanks! Let me know what you think 🙏 |
airflow-core/tests/unit/serialization/serializers/test_serializers.py
Outdated
Show resolved
Hide resolved
airflow-core/tests/unit/serialization/serializers/test_serializers.py
Outdated
Show resolved
Hide resolved
|
Fixed, thanks for reviewing! If there's anything that needs improvement, please let me know 💪 |
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Great job! Thanks for your contribution :)
3 participants
Related: #35127
I'm trying to improve the code coverage for serialization. If someone could take a look, I’d really appreciate it. :)
Btw, I found that the tests for
iceberg.pyonly work whenpyiceberg >= 2.0.0.However, the
pyicebergversion in my Breeze shell is0.9.1, which is why the coverage is low. Does anyone know why the minimum version is set to2.0.0?