- Notifications
You must be signed in to change notification settings - Fork5.7k
Description
This issue is a consequence of#4634 and#4617 (comment).
Currently, for each TO class we manually implementtest_de_json
and some classes also implementtest_de_json_required
andtest_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 inhttps://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 ofthumb
back in2015), 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.