GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Extend & Unify Deserialization Testing · Issue #4635 · python-telegram-bot/python-telegram-bot

Extend & Unify Deserialization Testing #4635

New issue
Copy link
New issue
Copy link
Open
🧹 chore
1 / 11 of 1 issue completed
Open
Extend & Unify Deserialization Testing#4635
🧹 chore
1 / 11 of 1 issue completed
Copy link
Labels
⚙️ bot-apiaffected functionality: bot-apiaffected functionality: bot-api⚙️ testsaffected functionality: testsaffected functionality: tests
Milestone
v23

Description

@Bibo-Joshi
Bibo-Joshi
opened on Jan 2, 2025
Issue body actions

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

  • 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.

Activity

  1. Bibo-Joshi
    added
    ⚙️ bot-apiaffected functionality: bot-apiaffected functionality: bot-api
    ⚙️ testsaffected functionality: testsaffected functionality: tests
    on Jan 2, 2025
  2. Bibo-Joshi
    added a parent issue on Jan 2, 2025
    • Test suite maintainence and new ideas #4324
  3. Bibo-Joshi commented on Jan 2, 2025

    @Bibo-Joshi
    Bibo-Joshi
    on Jan 2, 2025
    MemberAuthor
    More actions

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

  4. Bibo-Joshi commented on Jan 3, 2025

    @Bibo-Joshi
    Bibo-Joshi
    on Jan 3, 2025
    MemberAuthor
    More actions

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

  5. harshil21 commented on Jan 3, 2025

    @harshil21
    harshil21
    on Jan 3, 2025
    Member
    More actions

    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?

    Reacted by Bibo-Joshi
  6. Bibo-Joshi commented on Jan 3, 2025

    @Bibo-Joshi
    Bibo-Joshi
    on Jan 3, 2025
    MemberAuthor
    More actions

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

    yes, this :)

  7. Bibo-Joshi commented on Jan 5, 2025

    @Bibo-Joshi
    Bibo-Joshi
    on Jan 5, 2025
    MemberAuthor
    More actions

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

  8. Bibo-Joshi
    added a sub-issue on Jan 15, 2025
    • Split Tests for Classes with Subclass-de_json #4652
  9. github-actions
    added
    📋 stalework status: stalework status: stale
    on Oct 6, 2025
  10. harshil21
    removed
    📋 stalework status: stalework status: stale
    on Oct 8, 2025
  11. github-actions
    added
    📋 stalework status: stalework status: stale
    on Oct 12, 2025
  12. github-actions commented on Oct 15, 2025

    @github-actions
    github-actionsBot
    on Oct 15, 2025 – with GitHub Actions · Hidden as resolved
    show commentMore actions
  13. github-actions
    closed this as not plannedon Oct 15, 2025
  14. harshil21
    reopened this on Oct 15, 2025
  15. Bibo-Joshi
    removed
    📋 stalework status: stalework status: stale
    on Oct 15, 2025
  16. harshil21
    updated Priority Medium and Effort Medium on May 20, 2026
  17. harshil21
    added this to the v23 milestone on Jun 16, 2026
  18. 2ale2
    added a commit that references this issue on Nov 10, 2026

    Exclude input-only classes from de_json meta-test (python-telegram-bo…

    c710482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ⚙️ bot-apiaffected functionality: bot-apiaffected functionality: bot-api⚙️ testsaffected functionality: testsaffected functionality: tests

    Type

    🧹 chore

    Fields

    Priority

    Medium

    Effort

    Medium

    Projects

    No projects

      Milestone

      • v23

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions

      You are visitor 01402503 — GeezerVison does not store the pages you look up.
      Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.