+
Skip to content

Conversation

KJhellico
Copy link
Collaborator

Proposed change

Fix working day test.

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 20, 2025

Summary by CodeRabbit

  • Tests
    • Updated test utilities to use public date-checking APIs for weekend detection, aligning tests with real-world usage and improving input handling robustness.
  • Chores
    • Internal cleanup to reduce reliance on private helpers in test code for better maintainability.
  • Impact
    • No user-facing changes. Existing functionality and behavior remain the same.

Walkthrough

Replaced a weekend check in tests/common.py to use the public is_weekend(dt) instead of the internal _is_weekend(parse(dt)). is_working_day(dt) usage is unchanged. No modifications to public APIs or exported declarations.

Changes

Cohort / File(s) Summary of Changes
Tests
tests/common.py
Switched weekend detection from _is_weekend(parse(dt)) to is_weekend(dt); no other logic altered.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

test

Suggested reviewers

  • arkid15r
  • PPsyrius

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Fix working day test" accurately and concisely reflects the primary intent of the changeset (a test fix in tests/common.py to correct weekend/working-day logic) and is clear enough for a teammate scanning history to understand the main change.
Description Check ✅ Passed The PR description states the intent ("Fix working day test"), labels the change as a bugfix, and notes that local checks passed, so it is clearly related to the changeset and meets the lenient description criteria.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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/common.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

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

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

Copy link

codecov bot commented Sep 20, 2025

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff            @@
##               dev     #2950   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          299       299           
  Lines        17834     17834           
  Branches      2295      2295           
=========================================
  Hits         17834     17834           

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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 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 71f11b7 and b973c4e.

📒 Files selected for processing (1)
  • tests/common.py (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_botswana.py:59-59
Timestamp: 2025-09-14T16:23:46.707Z
Learning: In Botswana's holiday tests, assertNonObservedHoliday(dt) is used to verify that certain holidays (like Easter holidays) stay on their original dates regardless of the observed holiday system, which is different from assertNoNonObservedHoliday that checks for absence of non-observed holidays.
📚 Learning: 2025-09-03T18:29:09.398Z
Learnt from: KJhellico
PR: vacanza/holidays#2820
File: tests/countries/test_saint_helena_ascension_and_tristan_da_cunha.py:76-76
Timestamp: 2025-09-03T18:29:09.398Z
Learning: The assertNoNonObservedHoliday method in tests/common.py accepts a holidays object as its first parameter, followed by the dates to check. Usage like assertNoNonObservedHoliday(self.government_holidays_non_observed, obs_dt) is correct and intended behavior.

Applied to files:

  • tests/common.py
📚 Learning: 2025-09-14T16:23:46.707Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_botswana.py:59-59
Timestamp: 2025-09-14T16:23:46.707Z
Learning: In Botswana's holiday tests, assertNonObservedHoliday(dt) is used to verify that certain holidays (like Easter holidays) stay on their original dates regardless of the observed holiday system, which is different from assertNoNonObservedHoliday that checks for absence of non-observed holidays.

Applied to files:

  • tests/common.py
📚 Learning: 2025-09-14T16:23:46.707Z
Learnt from: PPsyrius
PR: vacanza/holidays#2881
File: tests/countries/test_botswana.py:59-59
Timestamp: 2025-09-14T16:23:46.707Z
Learning: The method assertNonObservedHoliday(dt) is a valid assertion method in the holidays test framework that verifies holidays occur on their original (non-observed) dates, which is different from assertNoNonObservedHoliday that checks for absence of non-observed holidays. It's used to test that certain holidays stay on fixed dates regardless of observed holiday rules.

Applied to files:

  • tests/common.py
🧬 Code graph analysis (1)
tests/common.py (1)
holidays/holiday_base.py (1)
  • is_weekend (1141-1151)
⏰ 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). (2)
  • GitHub Check: Test docs build
  • GitHub Check: Build distribution

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 🛠️

PPsyrius added a commit to PPsyrius/python-holidays that referenced this pull request Sep 20, 2025
@arkid15r arkid15r added this pull request to the merge queue Sep 20, 2025
Merged via the queue into vacanza:dev with commit ff426f3 Sep 20, 2025
36 checks passed
@KJhellico KJhellico deleted the fix-workday-test branch September 21, 2025 09:40
@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浏览器服务,不要输入任何密码和下载