-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Extend & Unify Deserialization Testing #4635
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
Comments
In light of point 3, I've openend tdlib/telegram-bot-api#686 |
Looking at the discussion in the tdlib issue, I strongly suggest to do 3 as well |
Yeah, the abstract base class for tests + meta test sounds pretty robust to me. Could also enforce testing of
by point 3 you mean testing the forward compatibility, right? |
yes, this :) |
FYI, I'd like to have #4617 merged before starting on this, as that PR is touching a lot of these tests as well. |
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 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 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
To be discussed¹:deserialization works if required arguments are missing (forward compatibility for TG removing arguments)I can see at least three options for ensuring that a test class tests all required use cases
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.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), seeOf course, as per usual, Telegram doesn't document such things.
The text was updated successfully, but these errors were encountered: