8000 Reduce Creation of HTTP Clients in Tests by Bibo-Joshi · Pull Request #4493 · python-telegram-bot/python-telegram-bot · GitHub
[go: up one dir, main page]
Skip to content

Reduce Creation of HTTP Clients in Tests #4493

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 25, 2024
Merged

Reduce Creation of HTTP Clients in Tests #4493

merged 5 commits into from
Sep 25, 2024

Conversation

Bibo-Joshi
Copy link
Member

Apparently the function load_ssl_context_verify that's called upon the creation of httpx.AsyncClient takes quite a bit of time so that creating new HTTPXRequest objects (indirectly via Bot) is a performance bottleneck in the tests.

Hence, this changing make_bot to use offline=True by default and allowing online only in the tests that make requests.

This is just one small step on test performance improvement, but hey it's something :)

@Bibo-Joshi Bibo-Joshi added the ⚙️ tests affected functionality: tests label Sep 21, 2024
@codecov Codecov
Copy link
codecov bot commented Sep 21, 2024 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
5991 1 5990 271
View the top 1 failed tests by shortest run time 
tests.test_bot.TestBotWithRequest test_copy_messages
Stack Traces | 0.48s run time 
self = <tests.test_bot.TestBotWithRequest object at 0x000001E49E2BA340>
bot = PytestExtBot[token=695104088:AAHfzylIOjSIIS-eOnI20y2E20HodHsfz-0]
chat_id = '675666224'

    async def test_copy_messages(self, bot, chat_id):
        tasks = asyncio.gather(
            bot.send_message(chat_id, text="will be copied 1"),
            bot.send_message(chat_id, text="will be copied 2"),
        )
        msg1, msg2 = await tasks
    
        copy_messages = await bot.copy_messages(
            chat_id, from_chat_id=chat_id, message_ids=sorted((msg1.message_id, msg2.message_id))
        )
        assert isinstance(copy_messages, tuple)
    
        tasks = asyncio.gather(
            bot.send_message(chat_id, "temp 1", reply_to_message_id=copy_messages[0].message_id),
            bot.send_message(chat_id, "temp 2", reply_to_message_id=copy_messages[1].message_id),
        )
        temp_msg1, temp_msg2 = await tasks
    
        forward_msg1 = temp_msg1.reply_to_message
        forward_msg2 = temp_msg2.reply_to_message
    
>       assert forward_msg1.text == msg1.text
E       AssertionError: assert 'will be copied 2' == 'will be copied 1'
E         
E         - will be copied 1
E         ?                ^
E         + will be copied 2
E         ?                ^

tests\test_bot.py:3962: AssertionError

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

harshil21
harshil21 reviewed Sep 21, 2024
View reviewed changes
Copy link
Member
@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what % of time was the calling of load_ssl_context_verify taking?

@Bibo-Joshi Bibo-Joshi merged commit 79e589b into master Sep 25, 2024
25 checks passed
@Bibo-Joshi Bibo-Joshi deleted the test-performance branch September 25, 2024 15:17
@Bibo-Joshi Bibo-Joshi mentioned this pull request Sep 25, 2024
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@harshil21 harshil21 harshil21 left review comments

Assignees
No one assigned
Labels
⚙️ tests affected functionality: tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Bibo-Joshi @harshil21
0