Extend & Unify Deserialization Testing

オープン
#4,635 コメント 6 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

評価

難易度
5/5
見積もり時間
1週間以上
初心者へのやさしさ
30/100
issue の種類
リファクタリング
明瞭さ
説明が足りない
活発さ
停滞
技術スタック
python
領域
testing

調査の方向性

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.

索引モデルが issue の本文から書いたものです。

説明

⚙️ bot-api ⚙️ tests

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

  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.

主要言語
Python
スター
29.5k
フォーク
6.2k
平均マージ
2日 1時間
マージ済み PR(30日)
14

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

python-telegram-bot/python-telegram-bot のほかの issue

python-telegram-bot/python-telegram-bot の issue をすべて見る

似ている issue

Python の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。