-
-
Notifications
You must be signed in to change notification settings - Fork 554
Refactor test_utils.py
#2970
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
Refactor test_utils.py
#2970
Conversation
Summary by CodeRabbit
WalkthroughRefactors and renames tests in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings
📚 Learning: 2025-04-05T04:47:27.213Z
Applied to files:
📚 Learning: 2025-05-06T21:07:11.577Z
Applied to files:
📚 Learning: 2025-04-05T06:49:06.217Z
Applied to files:
📚 Learning: 2025-09-18T03:19:23.722Z
Applied to files:
📚 Learning: 2025-09-14T16:03:13.558Z
Applied to files:
📚 Learning: 2025-04-05T04:29:38.042Z
Applied to files:
📚 Learning: 2025-08-25T04:28:02.061Z
Applied to files:
📚 Learning: 2025-09-14T16:05:55.205Z
Applied to files:
📚 Learning: 2025-09-10T14:35:54.603Z
Applied to files:
📚 Learning: 2025-04-05T04:50:40.752Z
Applied to files:
🧬 Code graph analysis (1)tests/test_utils.py (1)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (21)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1)tests/test_utils.py�[1;31mruff failed�[0m Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2970 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 301 301
Lines 17968 17968
Branches 2321 2321
=========================================
Hits 17968 17968 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Great refactoring! Although this test is not among top-longest for me. 🙄 Do you have an HDD? |
I have an M.2 SSD, but running |
Hmm, strange. I have 2.6s on
|
A little more optimization: countries_count = sum(
1 for path in Path("holidays/countries").glob("*.py") if path.stem != "__init__"
)
self.assertEqual(countries_count, len(supported_countries)) for L204-207 (and similar for L218-221). |
Weird - my vanilla baseline was always pretty fast except for ### Only PR #2881 (TestCase Syntactic Sugar) , with `with_subdiv_categories` flag added ###
===================================== slowest 10 durations ======================================
237.24s call tests/test_utils.py::TestListLocalizedEntities::test_localized_countries
23.09s setup tests/countries/test_united_states.py::TestUnitedStates::test_alaska_day
20.22s setup tests/countries/test_argentina.py::TestArgentina::test_2022
17.92s setup tests/countries/test_canada.py::TestCanada::test_all_holidays_present
15.17s setup tests/countries/test_malaysia.py::TestMalaysia::test_2023
11.35s setup tests/countries/test_chile.py::TestChile::test_2019
10.44s setup tests/countries/test_india.py::TestIndia::test_2018
10.27s setup tests/countries/test_finland.py::TestFinland::test_2018
9.64s setup tests/countries/test_spain.py::TestSpain::test_code
8.99s setup tests/countries/test_france.py::TestFrance::test_2017
==================== 6312 passed, 4 skipped, 2 warnings in 318.72s (0:05:18) ====================
-------------------------------------------------------------------------------------------------
### Both PR #2881 (TestCase Syntactic Sugar) and #2970 (Refactor `test_utils.py`) ###
===================================== slowest 10 durations ======================================
16.67s setup tests/countries/test_united_states.py::TestUnitedStates::test_alaska_day
14.86s setup tests/countries/test_argentina.py::TestArgentina::test_2022
13.28s setup tests/countries/test_canada.py::TestCanada::test_all_holidays_present
10.55s setup tests/countries/test_malaysia.py::TestMalaysia::test_2023
9.38s setup tests/countries/test_chile.py::TestChile::test_2019
9.21s setup tests/countries/test_india.py::TestIndia::test_2018
8.16s setup tests/countries/test_france.py::TestFrance::test_2017
7.87s call tests/test_l10n.py::TestLocalization::test_localization
7.79s call tests/countries/test_united_states.py::TestUnitedStates::test_groundhog_day
7.58s call tests/countries/test_united_states.py::TestUnitedStates::test_election_day
==================== 6315 passed, 4 skipped, 2 warnings in 65.65s (0:01:05) ===================== |
Co-Authored-By: ~Jhellico <KJhellico@users.noreply.github.com>
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! 👍
|
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.
looks good 👍
Proposed change
TestListLocalizedEntities.assertLocalizedEntities
to pre-scan.mo
files only once, drastically reducing test time. Previously, this test took ~240s; now it runs in ~2s and is no longer among the 10 slowest tests, cutting total test runtime from nearly 5 minutes to ~1 minute.TestCountryHolidays
,TestFinancialHolidays
,TestAllInSameYear
,TestListSupportedEntities
to remove duplicated assertions, add misisng test cases, etc.This refactor should probably have been done much earlier, given that slow test times have been one of my biggest pain points for years. At least it’s finally done and out of the way now 🎉
Type of change
holidays
functionality in general)Checklist
make check
locally; all checks and tests passed.