Skip to content
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

Extend & Unify Deserialization Testing #4635

Open
Bibo-Joshi opened this issue Jan 2, 2025 · 5 comments
Open

Extend & Unify Deserialization Testing #4635

Bibo-Joshi opened this issue Jan 2, 2025 · 5 comments
Labels
⚙️ bot-api affected functionality: bot-api ⚙️ tests affected functionality: tests

Comments

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 2, 2025

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

  1. deserialization works if optional arguments are missing
  2. deserialization works if additional arguments are passed (backward compatibility for new arguments)
  3. To be discussed¹: deserialization works if required arguments are missing (forward compatibility for TG removing arguments)
  4. 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

  1. 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.
  2. 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.
  3. 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

Of course, as per usual, Telegram doesn't document such things.

@Bibo-Joshi Bibo-Joshi added ⚙️ bot-api affected functionality: bot-api ⚙️ tests affected functionality: tests labels Jan 2, 2025
@Bibo-Joshi
Copy link
Member Author

In light of point 3, I've openend tdlib/telegram-bot-api#686

@Bibo-Joshi
Copy link
Member Author

Looking at the discussion in the tdlib issue, I strongly suggest to do 3 as well

@harshil21
Copy link
Member

Yeah, the abstract base class for tests + meta test sounds pretty robust to me. Could also enforce testing of test_slots, to_dict, equality and friends.

Looking at the discussion in the tdlib issue, I strongly suggest to do 3 as well

by point 3 you mean testing the forward compatibility, right?

@Bibo-Joshi
Copy link
Member Author

deserialization works if required arguments are missing (forward compatibility for TG removing arguments)

yes, this :)

@Bibo-Joshi
Copy link
Member Author

FYI, I'd like to have #4617 merged before starting on this, as that PR is touching a lot of these tests as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚙️ bot-api affected functionality: bot-api ⚙️ tests affected functionality: tests
Projects
None yet
Development

No branches or pull requests

2 participants