python-telegram-bot / python-telegram-bot/python-telegram-bot
Extend & Unify Deserialization Testing
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 29.5k
- Forks
- 6.2k
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 14
Description
This issue is a consequence of #4634 and https://github.com/python-telegram-bot/python-telegram-bot/pull/4617#discussion_r1900476722.
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")
- https://github.com/tdlib/telegram-bot-api/issues/526#issuecomment-1938506997 (§2)
Of course, as per usual, Telegram doesn't document such things.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by reviewing the existing TO-class deserialization tests and tests/auxil/bot_method_checks.py, then compare which cases each test class currently covers. Decide how the proposed enforcement should work and ensure every relevant class covers optional arguments, additional arguments, and any agreed required-argument or localization cases.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 30/100