+
Skip to content

Conversation

KJhellico
Copy link
Collaborator

Proposed change

Fix Islamic holidays tests in several countries.

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 Aug 22, 2025

Summary by CodeRabbit

  • Tests
    • Updated Afghanistan holiday tests to use range-based year intervals for non-holiday checks.
    • Introduced dedicated “no estimated Islamic dates” test calendars for Azerbaijan and Kazakhstan; consolidated assertions for Eid al-Fitr and Eid al-Adha (1990–1992/1993).
    • Added Kazakhstan checks ensuring no Kurban Ait when estimated Islamic dates are disabled (1991–2005).
    • Refined non-observed holiday validations using explicit date strings and shared year variables; passed non-observed years in setup where applicable.
    • No user-facing changes.

Walkthrough

Test updates across Afghanistan, Azerbaijan, and Kazakhstan: adjust year-range arguments, introduce class-level calendars with islamic_show_estimated=False, pass years_non_observed in setup, and refactor assertions to use dedicated calendars or string-date checks for non-observed and non-estimated Islamic holidays.

Changes

Cohort / File(s) Summary of changes
Afghanistan tests
tests/countries/test_afghanistan.py
Switched assertNoHolidayName end-year args from tuples to range() in Soviet Victory Day and Martyrs’ Day tests.
Azerbaijan tests
tests/countries/test_azerbaijan.py
Added class-level no_estimated_holidays (islamic_show_estimated=False); refactored setUpClass to use a local years var; updated Eid tests to assert absence via no_estimated_holidays for 1990–1992.
Kazakhstan tests
tests/countries/test_kazakhstan.py
setUpClass now passes years_non_observed and creates no_estimated_holidays; refactored non-observed checks to string-date assertions; added assertion that Kurban Ait absent in 1991–2005 when estimated holidays disabled.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

test

Suggested reviewers

  • arkid15r
  • PPsyrius

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 36970bc and 68a6146.

📒 Files selected for processing (3)
  • tests/countries/test_afghanistan.py (2 hunks)
  • tests/countries/test_azerbaijan.py (3 hunks)
  • tests/countries/test_kazakhstan.py (4 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: KJhellico
PR: vacanza/holidays#2631
File: tests/countries/test_sint_maarten.py:62-0
Timestamp: 2025-06-14T21:12:07.224Z
Learning: KJhellico prefers to focus on completing and reviewing the main holiday implementation code before doing detailed reviews of the corresponding test files.
Learnt from: PPsyrius
PR: vacanza/holidays#2614
File: holidays/countries/guyana.py:78-90
Timestamp: 2025-06-13T12:18:03.539Z
Learning: The holidays codebase now uses the constructor signature pattern `__init__(self, *args, islamic_show_estimated: bool = True, **kwargs)` across country classes.
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: 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.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: KJhellico
PR: vacanza/holidays#2706
File: holidays/countries/cayman_islands.py:50-55
Timestamp: 2025-07-10T15:55:34.523Z
Learning: The `islamic_show_estimated` parameter in country class constructors is only needed for countries that implement Islamic holidays (inherit from IslamicHolidays or _CustomIslamicHolidays groups). Countries with only Christian and secular holidays do not need this parameter.
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#2537
File: tests/countries/test_finland.py:23-26
Timestamp: 2025-05-09T18:36:09.607Z
Learning: The holidays project prioritizes complete historical coverage in tests, verifying holidays from their first year of observance (e.g., 1853 for Finland) through future projections, rather than using shorter sliding windows.
Learnt from: Wasif-Shahzad
PR: vacanza/holidays#2593
File: holidays/calendars/islamic.py:3993-3994
Timestamp: 2025-06-04T10:09:28.732Z
Learning: In the holidays/calendars/islamic.py file, the Islamic calendar date methods in the _IslamicLunar class (like ali_al_rida_death_dates, ashura_dates, etc.) follow a consistent pattern of being single-line methods that return self._get_holiday(CONSTANT, year) without docstrings. New methods should follow this same pattern for consistency.
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are covered by the test_eid_al_fitr_day() method, which tests both regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).
Learnt from: KJhellico
PR: vacanza/holidays#2465
File: holidays/countries/suriname.py:219-251
Timestamp: 2025-04-13T19:10:31.502Z
Learning: The `_CustomIslamicHolidays` classes in this project contain only exact verified holiday dates from reliable sources, rather than calculated or estimated future dates. This is by design to ensure accuracy, particularly for religious holidays that may follow lunar calendars or depend on local observations.
📚 Learning: 2025-06-13T12:18:03.539Z
Learnt from: PPsyrius
PR: vacanza/holidays#2614
File: holidays/countries/guyana.py:78-90
Timestamp: 2025-06-13T12:18:03.539Z
Learning: The holidays codebase now uses the constructor signature pattern `__init__(self, *args, islamic_show_estimated: bool = True, **kwargs)` across country classes.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.py
📚 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/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.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/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.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/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-04-04T10:52:41.546Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are properly covered by the test_eid_al_fitr_day() method, which tests both the regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.py
📚 Learning: 2025-04-04T10:52:41.546Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: holidays/countries/guinea.py:106-110
Timestamp: 2025-04-04T10:52:41.546Z
Learning: In the Guinea holidays implementation, observed Eid al-Fitr cases are covered by the test_eid_al_fitr_day() method, which tests both regular holiday dates and the observed dates when the holiday falls on a non-working day (for years >= 2023).

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-04-05T04:33:53.254Z
Learnt from: PPsyrius
PR: vacanza/holidays#2416
File: tests/countries/test_turkmenistan.py:52-64
Timestamp: 2025-04-05T04:33:53.254Z
Learning: For Turkmenistan holiday tests, recommend using CommonCountryTests as the base class rather than unittest.TestCase to follow project conventions, be consistent with other country test files, and gain access to common test utilities.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-07-24T15:21:31.632Z
Learnt from: PPsyrius
PR: vacanza/holidays#2750
File: tests/countries/test_germany.py:46-46
Timestamp: 2025-07-24T15:21:31.632Z
Learning: In the holidays project test files, the standard method name for testing the absence of holidays is `test_no_holidays`, not more descriptive names like `test_no_holidays_before_1990`. This is a consistent naming convention across country test files like France and Germany.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.py
📚 Learning: 2025-04-23T14:55:35.504Z
Learnt from: PPsyrius
PR: vacanza/holidays#2489
File: holidays/countries/sao_tome_and_principe.py:22-26
Timestamp: 2025-04-23T14:55:35.504Z
Learning: References in holidays classes should only be included if they're used for test case cross-checks or provide historical context about when holidays were added/removed, not just for the sake of having more references.

Applied to files:

  • tests/countries/test_azerbaijan.py
📚 Learning: 2025-06-18T17:01:58.067Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: tests/countries/test_wallis_and_futuna.py:19-23
Timestamp: 2025-06-18T17:01:58.067Z
Learning: In the vacanza/holidays repository, the standard pattern for test class setUpClass methods is `super().setUpClass(HolidayClass)` or `super().setUpClass(HolidayClass, years=...)` where HolidayClass is passed as the first argument. This is the correct signature that matches the CommonCountryTests.setUpClass method which accepts test_class as the first parameter after cls. Pylint warnings about parameter count mismatch are false positives when comparing against TestCase.setUpClass instead of the immediate parent CommonCountryTests.setUpClass.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.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/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-06-18T17:01:58.067Z
Learnt from: PPsyrius
PR: vacanza/holidays#2642
File: tests/countries/test_wallis_and_futuna.py:19-23
Timestamp: 2025-06-18T17:01:58.067Z
Learning: In the vacanza/holidays repository, the standard pattern for test class setUpClass methods is `super().setUpClass(HolidayClass)` where HolidayClass is passed as the first argument. This is the correct signature used across all country test files.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-06-22T21:36:18.015Z
Learnt from: KJhellico
PR: vacanza/holidays#2671
File: tests/countries/test_libya.py:19-24
Timestamp: 2025-06-22T21:36:18.015Z
Learning: In the vacanza/holidays project, test classes for countries do not use docstrings. All test classes follow the same pattern: class declaration directly followed by classmethod def setUpClass(cls) without any docstrings for the class or methods.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-06-16T15:48:48.680Z
Learnt from: PPsyrius
PR: vacanza/holidays#2615
File: tests/countries/test_anguilla.py:1-12
Timestamp: 2025-06-16T15:48:48.680Z
Learning: Test files in the holidays repository follow a standardized structure without module or class docstrings. All country test files use the same pattern: license header, imports, and class definition (`class Test{Country}(CommonCountryTests, TestCase):`) without docstrings. This is an established codebase convention that should be maintained for consistency.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.py
📚 Learning: 2025-07-09T21:16:35.145Z
Learnt from: KJhellico
PR: vacanza/holidays#2623
File: tests/countries/test_christmas_island.py:136-146
Timestamp: 2025-07-09T21:16:35.145Z
Learning: In Christmas Island's holiday implementation, the test_christmas_day method cannot use assertNoNonObservedHoliday because in some years observed Christmas Day overlaps with Boxing Day when both holidays are moved due to weekend conflicts, causing the standard non-observed holiday check to fail inappropriately.

Applied to files:

  • tests/countries/test_azerbaijan.py
  • tests/countries/test_kazakhstan.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/countries/test_kazakhstan.py
  • tests/countries/test_afghanistan.py
📚 Learning: 2025-06-15T11:52:39.572Z
Learnt from: PPsyrius
PR: vacanza/holidays#2601
File: tests/countries/test_mongolia.py:128-156
Timestamp: 2025-06-15T11:52:39.572Z
Learning: In the vacanza/holidays project tests, when testing holidays that span multiple consecutive days across many years (like Mongolia's National Festival spanning July 11-13), prefer explicit for loops over complex nested generator expressions with unpacking. The explicit loops are more readable, easier to maintain, and better communicate the testing intent even though the Big O complexity is equivalent.

Applied to files:

  • tests/countries/test_afghanistan.py
📚 Learning: 2025-06-14T10:58:43.636Z
Learnt from: PPsyrius
PR: vacanza/holidays#2629
File: tests/countries/test_namibia.py:22-23
Timestamp: 2025-06-14T10:58:43.636Z
Learning: In the vacanza/holidays project, country test files consistently use range(start_year, 2050) which intentionally excludes 2050 and stops at 2049. This is a library-wide implementation pattern, not an off-by-one error.

Applied to files:

  • tests/countries/test_afghanistan.py
📚 Learning: 2025-04-02T18:38:35.164Z
Learnt from: KJhellico
PR: vacanza/holidays#2398
File: tests/countries/test_guinea.py:237-239
Timestamp: 2025-04-02T18:38:35.164Z
Learning: In the vacanza/holidays project, the method assertLocalizedHolidays in country test files should be called with positional parameters rather than named parameters to maintain consistency with the rest of the codebase.

Applied to files:

  • tests/countries/test_afghanistan.py
🧬 Code graph analysis (3)
tests/countries/test_azerbaijan.py (2)
tests/common.py (2)
  • setUpClass (32-53)
  • assertNoHolidayName (273-275)
holidays/countries/azerbaijan.py (1)
  • Azerbaijan (27-197)
tests/countries/test_kazakhstan.py (2)
tests/common.py (3)
  • setUpClass (32-53)
  • assertNoNonObservedHoliday (248-250)
  • assertNoHolidayName (273-275)
holidays/countries/kazakhstan.py (1)
  • Kazakhstan (27-137)
tests/countries/test_afghanistan.py (1)
tests/common.py (1)
  • assertNoHolidayName (273-275)
⏰ 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). (3)
  • GitHub Check: Test build on windows-latest
  • GitHub Check: Test build on macos-latest
  • GitHub Check: Test build on ubuntu-latest
🔇 Additional comments (8)
tests/countries/test_afghanistan.py (2)

73-73: Swapping tuple for range() in assertNoHolidayName is correct.

This aligns with project-wide usage of range(start, 2050) and avoids misinterpreting (start, end) as two separate arguments.


99-99: Same fix here: range() over tuple — good.

Consistent with the repository pattern and the helper’s varargs semantics.

tests/countries/test_kazakhstan.py (3)

22-24: Solid setUpClass: central years, years_non_observed, and no_estimated_holidays.

Leverages CommonCountryTests correctly and prepares datasets for non-observed and non-estimated checks.


221-221: Using assertNoNonObservedHoliday for 2022+ Dec 17 is the right API.

This asserts the absence on the non-observed calendar explicitly and avoids false positives.


248-248: Good: assert absence of Kurban Ait before 2006 with islamic estimates disabled.

Matches Kazakhstan.start_year rules for Islamic holidays and the constructor’s islamic_show_estimated flag.

tests/countries/test_azerbaijan.py (3)

23-25: Clean setup: shared years and a no_estimated_holidays calendar.

Consistent with newer tests; leverages islamic_show_estimated=False correctly.


251-251: Right call: no estimated Eid al-Fitr in 1990–1992.

Asserting against the no_estimated_holidays dataset is the precise intent here.


269-269: Same here for Eid al-Adha — correct absence check for 1990–1992.

Keeps Islamic holiday tests precise without estimated dates.

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the test label Aug 22, 2025
Copy link

Copy link

codecov bot commented Aug 22, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2849   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          286       286           
  Lines        17040     17040           
  Branches      2229      2229           
=========================================
  Hits         17040     17040           

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

Copy link
Collaborator

@PPsyrius PPsyrius left a comment

Choose a reason for hiding this comment

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

LGTM 🛠️

@arkid15r arkid15r added this pull request to the merge queue Aug 22, 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.

👍

Merged via the queue into vacanza:dev with commit d9cd868 Aug 22, 2025
36 checks passed
@KJhellico KJhellico deleted the fix-islamic-tests branch August 22, 2025 16:04
@arkid15r arkid15r mentioned this pull request Sep 1, 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浏览器服务,不要输入任何密码和下载