GeezerVison is showing github.com — Simplify Handling of Empty Data in `TelegramObject.de_json` and Friends by Bibo-Joshi · Pull Request #4617 · python-telegram-bot/python-telegram-bot
Conversation
When Ready, closes #4614
Currently, I expect that a bunch of tests will fail as I haven't updated them yet
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
codecov
Bot
commented
Dec 29, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
❌ 177 Tests Failed:
| Tests completed |
Failed |
Passed |
Skipped |
| 6375 |
177 |
6198 |
452 |
View the top 3 failed tests by shortest run time
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_forum_topic_icon_stickers]
Stack Traces | 0.002s run time
self = <tests.test_bot.TestBotWithoutRequest object at 0x0000013990675270>
bot_class = <class 'telegram._bot.Bot'>
bot_method_name = 'get_forum_topic_icon_stickers'
bot_method = <bound method ExtBot.get_forum_topic_icon_stickers of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
@bot_methods(ext_bot=False)
async def test_defaults_handling(
self,
bot_class,
bot_method_name: str,
bot_method,
offline_bot: PytestExtBot,
raw_bot: PytestBot,
):
"""
Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two
parts:
1. Check that ExtBot actually inserts the defaults values correctly
2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't
pass any `DefaultValue` instances to Request. See the docstring of
tg.Bot._insert_defaults for details on why we need that
As for most defaults,
we can't really check the effect, we just check if we're passing the correct kwargs to
Request.post. As offline_bot method tests a scattered across the different test files, we
do this here in one place.
The same test is also run for all the shortcuts (Message.reply_text) etc in the
corresponding tests.
Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}
at the appropriate places, as those are the only things we can actually check.
"""
# Mocking get_me within check_defaults_handling messes with the cached values like
# Bot.{bot, username, id, …}` unless we return the expected User object.
return_value = (
offline_bot.bot if bot_method_name.lower().replace("_", "") == "getme" else None
)
# Check that ExtBot does the right thing
bot_method = getattr(offline_bot, bot_method_name)
raw_bot_method = getattr(raw_bot, bot_method_name)
> assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)
tests\test_bot.py:500:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:1913: in get_forum_topic_icon_stickers
return await super().get_forum_topic_icon_stickers(
telegram\_bot.py:8328: in get_forum_topic_icon_stickers
return Sticker.de_list(result, self)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cls = <class 'telegram._files.sticker.Sticker'>, data = None
bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
@classmethod
def de_list(
cls: type[Tele_co], data: list[JSONDict], bot: Optional["Bot"] = None
) -> tuple[Tele_co, ...]:
"""Converts a list of JSON objects to a tuple of Telegram objects.
.. versionchanged:: 20.0
* Returns a tuple instead of a list.
* Filters out any :obj:`None` values.
Args:
data (list[dict[:obj:`str`, ...]]): The JSON data.
bot (:class:`telegram.Bot`, optional): The bot associated with these object. Defaults
to :obj:`None`, in which case shortcut methods will not be available.
.. versionchanged:: 21.4
:paramref:`bot` is now optional and defaults to :obj:`None`
Returns:
A tuple of Telegram objects.
"""
> return tuple(obj for obj in (cls.de_json(d, bot) for d in data))
E TypeError: 'NoneType' object is not iterable
telegram\_telegramobject.py:457: TypeError
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_chat]
Stack Traces | 0.003s run time
self = <tests.test_bot.TestBotWithoutRequest object at 0x0000013990674FF0>
bot_class = <class 'telegram._bot.Bot'>, bot_method_name = 'get_chat'
bot_method = <bound method ExtBot.get_chat of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
@bot_methods(ext_bot=False)
async def test_defaults_handling(
self,
bot_class,
bot_method_name: str,
bot_method,
offline_bot: PytestExtBot,
raw_bot: PytestBot,
):
"""
Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two
parts:
1. Check that ExtBot actually inserts the defaults values correctly
2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't
pass any `DefaultValue` instances to Request. See the docstring of
tg.Bot._insert_defaults for details on why we need that
As for most defaults,
we can't really check the effect, we just check if we're passing the correct kwargs to
Request.post. As offline_bot method tests a scattered across the different test files, we
do this here in one place.
The same test is also run for all the shortcuts (Message.reply_text) etc in the
corresponding tests.
Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}
at the appropriate places, as those are the only things we can actually check.
"""
# Mocking get_me within check_defaults_handling messes with the cached values like
# Bot.{bot, username, id, …}` unless we return the expected User object.
return_value = (
offline_bot.bot if bot_method_name.lower().replace("_", "") == "getme" else None
)
# Check that ExtBot does the right thing
bot_method = getattr(offline_bot, bot_method_name)
raw_bot_method = getattr(raw_bot, bot_method_name)
> assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)
tests\test_bot.py:500:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:889: in get_chat
result = await super().get_chat(
telegram\_bot.py:4732: in get_chat
return ChatFullInfo.de_json(result, self)
telegram\_chatfullinfo.py:517: in de_json
data = cls._parse_data(data)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
data = None
@staticmethod
def _parse_data(data: JSONDict) -> JSONDict:
"""Should be called by subclasses that override de_json to ensure that the input
is not altered. Whoever calls de_json might still want to use the original input
for something else.
"""
> return data.copy()
E AttributeError: 'NoneType' object has no attribute 'copy'
telegram\_telegramobject.py:385: AttributeError
tests.test_bot.TestBotWithoutRequest::test_defaults_handling[Bot.get_star_transactions]
Stack Traces | 0.003s run time
self = <tests.test_bot.TestBotWithoutRequest object at 0x0000013990675590>
bot_class = <class 'telegram._bot.Bot'>
bot_method_name = 'get_star_transactions'
bot_method = <bound method ExtBot.get_star_transactions of PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]>
offline_bot = PytestExtBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
raw_bot = PytestBot[token=5737018356:AAH138SuiKQF0LDCWsfgWeXfjJ5d63kCWLA]
@bot_methods(ext_bot=False)
async def test_defaults_handling(
self,
bot_class,
bot_method_name: str,
bot_method,
offline_bot: PytestExtBot,
raw_bot: PytestBot,
):
"""
Here we check that the offline_bot methods handle tg.ext.Defaults correctly. This has two
parts:
1. Check that ExtBot actually inserts the defaults values correctly
2. Check that tg.Bot just replaces `DefaultValue(obj)` with `obj`, i.e. that it doesn't
pass any `DefaultValue` instances to Request. See the docstring of
tg.Bot._insert_defaults for details on why we need that
As for most defaults,
we can't really check the effect, we just check if we're passing the correct kwargs to
Request.post. As offline_bot method tests a scattered across the different test files, we
do this here in one place.
The same test is also run for all the shortcuts (Message.reply_text) etc in the
corresponding tests.
Finally, there are some tests for Defaults.{parse_mode, quote, allow_sending_without_reply}
at the appropriate places, as those are the only things we can actually check.
"""
# Mocking get_me within check_defaults_handling messes with the cached values like
# Bot.{bot, username, id, …}` unless we return the expected User object.
return_value = (
offline_bot.bot if bot_method_name.lower().replace("_", "") == "getme" else None
)
# Check that ExtBot does the right thing
bot_method = getattr(offline_bot, bot_method_name)
raw_bot_method = getattr(raw_bot, bot_method_name)
> assert await check_defaults_handling(bot_method, offline_bot, return_value=return_value)
tests\test_bot.py:500:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests\auxil\bot_method_checks.py:668: in check_defaults_handling
raise exc
tests\auxil\bot_method_checks.py:636: in check_defaults_handling
assert await method(**kwargs) in expected_return_values
telegram\ext\_extbot.py:4309: in get_star_transactions
return await super().get_star_transactions(
telegram\_bot.py:9405: in get_star_transactions
return StarTransactions.de_json(
telegram\_payment\stars\startransactions.py:159: in de_json
data = cls._parse_data(data)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
data = None
@staticmethod
def _parse_data(data: JSONDict) -> JSONDict:
"""Should be called by subclasses that override de_json to ensure that the input
is not altered. Whoever calls de_json might still want to use the original input
for something else.
"""
> return data.copy()
E AttributeError: 'NoneType' object has no attribute 'copy'
telegram\_telegramobject.py:385: AttributeError
To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Admittedly, this turned out to be a bigger change than expected. Not really the removal of if not data: return None itself, but all that depended on that. However, this shows how much the internal logic of de_json and our tests dependet on this undocumented implementation detail that has no user facing benefit. Clearing that up and making the business logic independent of satisfying minor convenience needs for the tests is a good change IMO.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a couple of questions-
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
# Conflicts:
# telegram/_gifts.py
# tests/auxil/bot_method_checks.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one minor thing to update:
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Poolitzer
Awaiting requested review from Poolitzer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|