+
Skip to content

Conversation

PPsyrius
Copy link
Collaborator

@PPsyrius PPsyrius commented Sep 30, 2025

Proposed change

  • Refactor 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.
### Pre `assertLocalizedEntities` Refactor ###
Required test coverage of 100% reached. Total coverage: 100.00%
===================================== slowest 10 durations ======================================
240.21s call     tests/test_utils.py::TestListLocalizedEntities::test_localized_countries
17.64s setup    tests/countries/test_united_states.py::TestUnitedStates::test_alaska_day
6.97s call     tests/countries/test_united_states.py::TestUnitedStates::test_election_day
6.62s call     tests/test_l10n.py::TestLocalization::test_localization
5.24s setup    tests/countries/test_argentina.py::TestArgentina::test_2022
5.05s setup    tests/countries/test_canada.py::TestCanada::test_all_holidays_present
5.04s call     tests/countries/test_india.py::TestIndia::test_ranged_subdiv_holidays
4.95s call     tests/test_utils.py::TestListLocalizedEntities::test_localized_financial
4.71s setup    tests/countries/test_france.py::TestFrance::test_2017
3.36s setup    tests/countries/test_switzerland.py::TestSwitzerland::test_all_holidays_present
========================== 6193 passed, 4 skipped in 293.30s (0:04:53) ==========================

-----------------------------------------------------------------------------------------------------------------------------------------

### with `assertLocalizedEntities` Refactor ###
Required test coverage of 100% reached. Total coverage: 100.00%
===================================== slowest 10 durations ======================================
17.94s setup    tests/countries/test_united_states.py::TestUnitedStates::test_alaska_day
8.63s call     tests/test_l10n.py::TestLocalization::test_localization
6.92s call     tests/countries/test_united_states.py::TestUnitedStates::test_election_day
5.57s setup    tests/countries/test_canada.py::TestCanada::test_all_holidays_present
5.40s setup    tests/countries/test_argentina.py::TestArgentina::test_2022
5.25s call     tests/countries/test_india.py::TestIndia::test_ranged_subdiv_holidays
4.82s setup    tests/countries/test_france.py::TestFrance::test_2017
3.59s setup    tests/countries/test_switzerland.py::TestSwitzerland::test_all_holidays_present
2.77s setup    tests/countries/test_malaysia.py::TestMalaysia::test_2023
2.10s setup    tests/countries/test_australia.py::TestAustralia::test_adelaide_cup_day
=============================== 6196 passed, 4 skipped in 57.52s ================================
  • General refactor for 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

  • New country/market holidays support (thank you!)
  • Supported country/market holidays update (calendar discrepancy fix, localization)
  • Existing code/documentation/test/process quality improvement (best practice, cleanup, refactoring, optimization)
  • Dependency update (version deprecation/pin/upgrade)
  • Bugfix (non-breaking change which fixes an issue)
  • Breaking change (a code change causing existing functionality to break)
  • New feature (new holidays functionality in general)

Checklist

Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

Summary by CodeRabbit

  • Tests
    • Restructured and renamed tests for clearer scope and readability.
    • Split invalid-input checks into dedicated tests for countries and markets.
    • Standardized single-year, multi-year, and range validations using a shared helper.
    • Updated deprecation checks to assert specific warnings and messages.
    • Enhanced localization verification by comparing against a centralized language mapping.
    • Minor wording improvements in test messages and docstrings to match new structure.

Walkthrough

Refactors and renames tests in tests/test_utils.py, adds range/multi-year coverage and explicit invalid-input tests, replaces inline yearly loops with a _check_holidays_years helper, updates localization/language collection assertions, and changes deprecation assertions to use assertWarns. Minor docstring and wording tweaks.

Changes

Cohort / File(s) Summary
Tests: refactor, renames, and new helpers
tests/test_utils.py
Renamed several test methods (country/year/state/province → clearer names), split invalid-input tests into explicit NotImplemented checks for countries/markets and subdivisions, introduced _check_holidays_years to centralize per-year/full-range checks, updated deprecation assertions to use assertWarns(DeprecationWarning), refactored localization assertions to use an entity→languages mapping, and adjusted docstrings and assertion messages.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • arkid15r
  • KJhellico

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Refactor test_utils.py” succinctly identifies the primary focus of the changeset—refactoring the tests in test_utils.py—and is concise, readable, and directly related to the main modifications in the pull request.
Description Check ✅ Passed The description details the performance optimization of TestListLocalizedEntities.assertLocalizedEntities, quantifies the runtime improvements, and notes the broader refactor of test utilities, clearly reflecting the actual changes introduced in the pull request.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99d4728 and 4373137.

📒 Files selected for processing (1)
  • tests/test_utils.py (5 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.
Learnt from: PPsyrius
PR: vacanza/holidays#2386
File: tests/countries/test_nepal.py:499-536
Timestamp: 2025-04-05T06:49:06.217Z
Learning: In the holidays project, test files follow a dual testing approach: individual methods test specific holidays across multiple years, while comprehensive year-specific tests (e.g., `test_2025`) verify all holidays for a specific year in a single assertion. Both approaches serve different testing purposes and complement each other.
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_algeria.py:28-30
Timestamp: 2025-09-18T03:19:23.722Z
Learning: In the vacanza/holidays project, tests now use self.start_year and self.end_year from the TestCase class instead of country-specific aliases (like DZ.start_year) for start_year and end_year references. This approach provides the test framework with better control over test year ranges rather than being tied to specific country start years.
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: tests/countries/test_andorra.py:23-28
Timestamp: 2025-05-06T21:07:11.577Z
Learning: In the holidays project, test methods for country holidays follow a standard form where year ranges are explicitly recreated in each test method rather than being stored as class variables, to maintain consistency across all country tests.
📚 Learning: 2025-04-05T04:47:27.213Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:47:27.213Z
Learning: For holiday tests in the vacanza/holidays project, structure tests by individual holidays rather than by years. Each test method should focus on a specific holiday and test it across multiple years (from start_year through 2050) using helper methods like `assertHolidayName`. For fixed holidays, use generators like `(f"{year}-01-01" for year in range(1991, 2051))`. For movable holidays, specify individual dates for specific years followed by a range check.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-05-06T21:07:11.577Z
Learnt from: KJhellico
PR: vacanza/holidays#2530
File: tests/countries/test_andorra.py:23-28
Timestamp: 2025-05-06T21:07:11.577Z
Learning: In the holidays project, test methods for country holidays follow a standard form where year ranges are explicitly recreated in each test method rather than being stored as class variables, to maintain consistency across all country tests.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-04-05T06:49:06.217Z
Learnt from: PPsyrius
PR: vacanza/holidays#2386
File: tests/countries/test_nepal.py:499-536
Timestamp: 2025-04-05T06:49:06.217Z
Learning: In the holidays project, test files follow a dual testing approach: individual methods test specific holidays across multiple years, while comprehensive year-specific tests (e.g., `test_2025`) verify all holidays for a specific year in a single assertion. Both approaches serve different testing purposes and complement each other.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-09-18T03:19:23.722Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_algeria.py:28-30
Timestamp: 2025-09-18T03:19:23.722Z
Learning: In the vacanza/holidays project, tests now use self.start_year and self.end_year from the TestCase class instead of country-specific aliases (like DZ.start_year) for start_year and end_year references. This approach provides the test framework with better control over test year ranges rather than being tied to specific country start years.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-09-14T16:03:13.558Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_barbados.py:21-23
Timestamp: 2025-09-14T16:03:13.558Z
Learning: In tests/countries/test_barbados.py, using years_non_observed=range(2000, 2024) is intentional because all observed holiday examples fall within 2001-2023, making this range appropriate for limiting testing to years where observed holidays actually exist in the test data.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-04-05T04:29:38.042Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:85-86
Timestamp: 2025-04-05T04:29:38.042Z
Learning: For testing holiday implementations in the vacanza/holidays repository, recommend using `from tests.common import CommonCountryTests` as the base class instead of directly using `unittest.TestCase` to maintain consistency with project conventions and leverage common test utilities.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-08-25T04:28:02.061Z
Learnt from: PPsyrius
PR: vacanza/holidays#2848
File: tests/countries/test_somalia.py:44-127
Timestamp: 2025-08-25T04:28:02.061Z
Learning: In the holidays library, Islamic holiday tests use `self.no_estimated_holidays = Country(years=years, islamic_show_estimated=False)` as the library-wide standard approach to simplify test cases. This pattern is intentional and preferred over testing estimated labels.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-09-14T16:05:55.205Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_iran.py:28-28
Timestamp: 2025-09-14T16:05:55.205Z
Learning: In tests/countries/test_iran.py, using years=(self.start_year - 1, 2102) in the no-holiday test is intentional because Iran uses the Persian Calendar which has specific supported year range constraints, and 2102 represents the upper limit of the Persian Calendar's supported range, not just an arbitrary far-future date.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-09-10T14:35:54.603Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_brazil.py:28-30
Timestamp: 2025-09-10T14:35:54.603Z
Learning: In the holidays project, the test_no_holidays method should test ALL supported_categories (via categories=CountryClass.supported_categories) rather than just the default PUBLIC category, to ensure comprehensive validation that no holidays exist before start_year across any supported category including OPTIONAL and subdivision categories.

Applied to files:

  • tests/test_utils.py
📚 Learning: 2025-04-05T04:50:40.752Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:31-49
Timestamp: 2025-04-05T04:50:40.752Z
Learning: For Turkmenistan holiday tests, use this class structure: `class TestTurkmenistan(CommonCountryTests, TestCase)` with imports `from unittest import TestCase`, `from holidays.countries import Turkmenistan, TM, TKM`, and `from tests.common import CommonCountryTests`. Ensure to call `super().setUp()` in the setUp method.

Applied to files:

  • tests/test_utils.py
🧬 Code graph analysis (1)
tests/test_utils.py (1)
holidays/utils.py (5)
  • country_holidays (32-228)
  • CountryHoliday (316-333)
  • financial_holidays (231-313)
  • list_supported_countries (411-422)
  • list_supported_financial (426-437)
⏰ 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)
  • GitHub Check: Test Python 3.13 on windows-latest
🔇 Additional comments (21)
tests/test_utils.py (21)

15-15: LGTM!

Good addition of defaultdict for the optimized localization check logic.


43-44: LGTM!

Clearer variable naming for the single-year test case.


46-48: LGTM!

The rename from test_country_years to test_country_multiple_years makes the test's intent more explicit.


50-52: LGTM!

The rename from test_country_state to test_country_range_years accurately reflects that it's testing year ranges, not state/subdivision logic.


54-56: LGTM!

The rename from test_country_province to test_country_subdivision better aligns with the current API's subdiv parameter naming.


58-64: LGTM!

Excellent separation of exception tests into distinct methods. This makes test failures clearer and follows the single-assertion-per-test principle.


66-69: LGTM!

Using assertWarns is the proper way to test deprecation warnings. The message assertion on line 69 confirms the warning content.


79-81: LGTM!

Consistent pattern with the country tests. The explicit variable and blank line improve readability.


83-89: LGTM!

Great addition of explicit tests for multiple years and year ranges, maintaining consistency with the country tests.


91-97: LGTM!

Excellent separation of exception tests, mirroring the improvements in TestCountryHolidays.


101-101: LGTM!

The updated docstring is more concise while retaining the key information.


105-125: LGTM!

Excellent refactor! The _check_holidays_years helper eliminates duplication and provides a clean abstraction for testing both countries and financial markets. The use of subTest ensures each entity is tested independently, improving test isolation and failure reporting.


132-133: LGTM!

Clean usage of the new helper method.


135-141: LGTM!

Excellent addition of financial market testing using the same helper, ensuring parity with country tests.


149-156: LGTM! Major performance win.

This pre-scanning optimization is the core improvement that reduces test runtime from ~240s to ~2s. The logic correctly collects locale-to-entity mappings by scanning .mo files once using rglob, extracting the entity code from path.stem and the locale from path.parts[-3]. Using defaultdict(list) is efficient and the subsequent .get() call safely handles missing entities.


162-162: LGTM!

Minor wording improvement for clarity in the error message.


197-198: LGTM!

Consolidating multiple country checks into a single loop reduces duplication.


204-207: LGTM! Clean implementation of the suggested optimization.

Using Path.glob with a generator expression is more concise and efficient than the previous approach. This matches the micro-optimization suggested in the PR comments.


212-213: LGTM!

Consolidating multiple market checks into a single loop reduces duplication.


215-216: LGTM!

Using .get() with a default empty list is safer and more explicit than assuming the key exists.


218-221: LGTM!

Same clean Path.glob optimization applied to financial markets, maintaining consistency with the countries count.

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
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100


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.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We 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:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the test label Sep 30, 2025
Copy link

codecov bot commented Sep 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (ab58ac2) to head (3d1d300).
⚠️ Report is 3 commits behind head on dev.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KJhellico
Copy link
Collaborator

Great refactoring! Although this test is not among top-longest for me. 🙄 Do you have an HDD?

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Sep 30, 2025

I have an M.2 SSD, but running make test / make check command via Windows' WSL probably isn't the most optimised development environment ever - so this definitely helps 🤷

@KJhellico
Copy link
Collaborator

Hmm, strange. I have 2.6s on test_localized_countries. And is's considering that

42.80s setup    tests/countries/test_united_states.py::TestUnitedStates::test_alaska_day
30.52s setup    tests/countries/test_argentina.py::TestArgentina::test_2022
28.31s setup    tests/countries/test_france.py::TestFrance::test_2017
27.60s setup    tests/countries/test_canada.py::TestCanada::test_all_holidays_present
19.61s setup    tests/countries/test_switzerland.py::TestSwitzerland::test_all_holidays_present
13.71s setup    tests/countries/test_malaysia.py::TestMalaysia::test_2023
11.99s setup    tests/countries/test_australia.py::TestAustralia::test_adelaide_cup_day
11.74s setup    tests/countries/test_andorra.py::TestAndorra::test_2023

@KJhellico
Copy link
Collaborator

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

@PPsyrius
Copy link
Collaborator Author

PPsyrius commented Sep 30, 2025

Hmm, strange. I have 2.6s on test_localized_countries. And is's considering that

Weird - my vanilla baseline was always pretty fast except for tests/test_utils.py::TestListLocalizedEntities::test_localized_countries in particular, as seen above, it wasn't until I did the Syntactic Sugar test case as seen in #2881 that I get a similar pattern of spending more time on setup instead of call.

### 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>
Copy link
Collaborator

@KJhellico KJhellico left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link

sonarqubecloud bot commented Oct 4, 2025

@arkid15r arkid15r added this pull request to the merge queue Oct 5, 2025
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

looks good 👍

Merged via the queue into vacanza:dev with commit f6cdab2 Oct 5, 2025
36 checks passed
@PPsyrius PPsyrius deleted the test_util_update branch October 6, 2025 01:20
@arkid15r arkid15r mentioned this pull request Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载