8000 Rename Testing Base Classes by Bibo-Joshi · Pull Request #4453 · python-telegram-bot/python-telegram-bot · GitHub
[go: up one dir, main page]

Skip to content

Rename Testing Base Classes #4453

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

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Rename Testing Base Classes #4453

merged 1 commit into from
Sep 3, 2024

Conversation

Bibo-Joshi
Copy link
Member

addresses part sof #4324

Example console output before and after renaming:

(venv311) ~\PycharmProjects\python-telegram-bot git:[rename-test-classes]
pytest -v -k NoMatchTest
======================================================================= test session starts =======================================================================
platform win32 -- Python 3.11.9, pytest-8.2.2, pluggy-1.5.0 -- C:\Users\hinri\PycharmProjects\python-telegram-bot\venv311\Scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\hinri\PycharmProjects\python-telegram-bot
configfile: pyproject.toml
testpaths: tests
plugins: anyio-4.4.0, flaky-3.8.1, asyncio-0.21.2, socket-0.7.0, xdist-3.6.1, web3-6.18.0
asyncio: mode=Mode.AUTO
collected 6200 items / 6200 deselected / 0 selected                                                      
8000
                                                          

==================================================================== 6200 deselected in 17.72s ====================================================================
(venv311) ~\PycharmProjects\python-telegram-bot git:[rename-test-classes]
pytest -v -k NoMatchTest
======================================================================= test session starts =======================================================================
platform win32 -- Python 3.11.9, pytest-8.2.2, pluggy-1.5.0 -- C:\Users\hinri\PycharmProjects\python-telegram-bot\venv311\Scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\hinri\PycharmProjects\python-telegram-bot
configfile: pyproject.toml
testpaths: tests
plugins: anyio-4.4.0, flaky-3.8.1, asyncio-0.21.2, socket-0.7.0, xdist-3.6.1, web3-6.18.0
asyncio: mode=Mode.AUTO
collected 6200 items / 6200 deselected / 0 selected                                                                                                                

==================================================================== 6200 deselected in 12.71s ====================================================================

Note that the number of collected items did not change. What did change is the elapsed time, however I'm not sure if this is really reproducable. In any case, it doesn't hurt to rename these classes and if on average it does speed up the collection that's a good thing :)

@harshil21
Copy link
Member
harshil21 commented Sep 3, 2024

Thanks! I ran hyperfine with pytest's --collect-only, and the results were not conclusive (compared with master):

(python-telegram-bot) ➜  python-telegram-bot git:(master) ✗ hyperfine "pytest --collect-only"
Benchmark 1: pytest --collect-only
  Time (mean ± σ):      2.997 s ±  0.118 s    [User: 2.897 s, System: 0.094 s]
  Range (min … max):    2.928 s …  3.310 s    10 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
(python-telegram-bot) ➜  python-telegram-bot git:(master) ✗ git checkout rename-test-classes 
Switched to branch 'rename-test-classes'
Your branch is up to date with 'origin/rename-test-classes'.
(python-telegram-bot) ➜  python-telegram-bot git:(rename-test-classes) ✗ hyperfine "pytest --collect-only"
Benchmark 1: pytest --collect-only
  Time (mean ± σ):      3.207 s ±  0.933 s    [User: 3.107 s, System: 0.095 s]
  Range (min … max):    2.888 s …  5.861 s    10 runs
 
  Warning: The first benchmarking run for this command was significantly slower than the rest (5.861 s). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

My guess is that those classes are collected because they are being subclassed..

@Bibo-Joshi
Copy link
Member Author

That means that you would prefer not to merge or that you agree with

In any case, it doesn't hurt to rename these classes and if on average it does speed up the collection that's a good thing :)

? :)

@harshil21
Copy link
Member

I agree we should we merge anyway, but was kinda hoping that there was a little speedup for collection.

@harshil21 harshil21 added the ⚙️ tests affected functionality: tests label Sep 3, 2024
@Bibo-Joshi Bibo-Joshi merged commit b9d2efd into master Sep 3, 2024
26 of 27 checks passed
@Bibo-Joshi Bibo-Joshi deleted the rename-test-classes branch September 3, 2024 03:24
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚙️ tests affected functionality: tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0