Repository navigation
Extend & Unify Deserialization Testing #4635
Description
This issue is a consequence of #4634 and #4617 (comment).
Currently, for each TO class we manually implement test_de_json and some classes also implement test_de_json_required and test_de_json_localization. Personally I'm okay with the explicit repitition since automating tests across several classes can be error prone itself (see #4593 and also the convoluted logic in https://github.com/python-telegram-bot/python-telegram-bot/blob/v21.9/tests/auxil/bot_method_checks.py).
However, the findings above show that we should try to ensure that we test all uses cases in all classes, that being
- deserialization works if optional arguments are missing
- deserialization works if additional arguments are passed (backward compatibility for new arguments)
To be discussed¹:deserialization works if required arguments are missing (forward compatibility for TG removing arguments)- If required for that class: deserialization correctly localizes datetimes
I can see at least three options for ensuring that a test class tests all required use cases
- Introducing helper methods that do all that and calling them in the test classes. Adantvage: Less repetition. Disatvantage: Still need to ensure that the helper methods are called and also the downside of increased test complexity.
- Introduce abstract base class for TO-Class tests with abstract methods
test_de_json,test_de_json_required,test_de_json_locatization(?) and possibly others. This should force subclasses to implement all relevant tests. Advantage: Enforced unified setup without added test complexity. Disadvantage: Still need to check that the ABC is used. Could be done with a meta-test. - Add a meta-test that checks simply that methods with the corresponding names exist. Advantage: Enforced unified setup without much added test complexity. Disadvantage: Less explicit than 2.
At first glance, I'm in favor of 2.
@harshil21 @Poolitzer do you have any preferences?
¹ So far, Telegram has always made the removal of fields backward compatible (with the exception of thumb back in 2015), see
- https://core.telegram.org/bots/api-changelog (search for "backward")
- Introduce RichText type tdlib/telegram-bot-api#526 (comment) (§2)
Of course, as per usual, Telegram doesn't document such things.
Metadata
Metadata
Assignees
Labels
Type
Fields
Priority
Effort
Activity
Bibo-Joshi commentedon Jan 2, 2025
In light of point 3, I've openend tdlib/telegram-bot-api#686
Bibo-Joshi commentedon Jan 3, 2025
Looking at the discussion in the tdlib issue, I strongly suggest to do 3 as well
harshil21 commentedon Jan 3, 2025
Yeah, the abstract base class for tests + meta test sounds pretty robust to me. Could also enforce testing of
test_slots,to_dict,equalityand friends.by point 3 you mean testing the forward compatibility, right?
Bibo-Joshi commentedon Jan 3, 2025
yes, this :)
Bibo-Joshi commentedon Jan 5, 2025
FYI, I'd like to have #4617 merged before starting on this, as that PR is touching a lot of these tests as well.
de_json#4652github-actions commentedon Oct 15, 2025