Skip to content

Update l10n test: check .po files for missing entries comments#3594

Merged
arkid15r merged 1 commit into
vacanza:devfrom
KJhellico:upd-l10n-po-test
May 31, 2026
Merged

Update l10n test: check .po files for missing entries comments#3594
arkid15r merged 1 commit into
vacanza:devfrom
KJhellico:upd-l10n-po-test

Conversation

@KJhellico

@KJhellico KJhellico commented May 31, 2026

Copy link
Copy Markdown
Collaborator

Proposed change

Update l10n test: check .po files for missing entries comments.

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

@github-actions github-actions Bot added the test label May 31, 2026
@coderabbitai

coderabbitai Bot commented May 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

Summary by CodeRabbit

  • Tests
    • Enhanced localization testing with improved validation to ensure localization entry quality standards.

Walkthrough

The PR adds a validation assertion to the test_localization test loop in tests/test_l10n.py that verifies every localization PO entry includes a non-empty comment. The failure message includes the entity code, language, PO line number, and message ID for debugging.

Changes

Localization PO Entry Comment Validation

Layer / File(s) Summary
Comment validation in test loop
tests/test_l10n.py		 Asserts that each PO file entry contains a non-empty entry.comment, with diagnostic output including entity code, language, line number, and msgid.

Estimated Code Review Effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly Related PRs

  • vacanza/holidays#3279: Extends test_localization to validate mandatory metadata fields in PO entries, establishing the pattern this PR follows.
  • vacanza/holidays#3169: Modifies test_localization to add new PO entry validation checks in the same test flow.
  • vacanza/holidays#3570: Updates PO generator to treat comment fields as part of equality logic, directly impacted by this test requirement.

Suggested Labels

test

Suggested Reviewers

  • arkid15r
  • PPsyrius
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding a check to the l10n test for missing .po file entry comments.
Description check ✅ Passed The description directly relates to the changeset, explaining the proposed update to the l10n test and confirming the contributor followed guidelines and ran local checks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@sonarqubecloud

sonarqubecloud Bot commented May 31, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@codecov

codecov Bot commented May 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (414f9db) to head (8c0ba50).

Additional details and impacted files 
@@            Coverage Diff            @@
##               dev     #3594   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          315       315           
  Lines        18827     18827           
  Branches      2409      2409           
=========================================
  Hits         18827     18827

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

coderabbitai[bot]
coderabbitai Bot reviewed May 31, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_l10n.py`:
- Around line 94-96: The assertion in tests/test_l10n.py currently only checks
truthiness of entry.comment, allowing whitespace-only strings to pass; change
the check to assert that entry.comment contains non-whitespace characters (e.g.,
use entry.comment.strip() in the assertion) so whitespace-only comments fail,
and keep the existing f"The {entity_code} {language} localization contains
missing comment " message for context; update any references to entry.comment in
the same test method to use .strip() where appropriate.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a6bd3354-c134-4042-86a9-2a7ffe1f697d

📥 Commits

Reviewing files that changed from the base of the PR and between 414f9db and 8c0ba50.

📒 Files selected for processing (1)
  • tests/test_l10n.py

Comment thread tests/test_l10n.py
cubic-dev-ai[bot]
cubic-dev-ai Bot reviewed May 31, 2026
View reviewed changes

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues found across 1 file

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@arkid15r arkid15r enabled auto-merge May 31, 2026 17:42
arkid15r
arkid15r reviewed May 31, 2026
View reviewed changes
Comment thread tests/test_l10n.py
@arkid15r arkid15r added this pull request to the merge queue May 31, 2026
Merged via the queue into vacanza:dev with commit 85511bf May 31, 2026
33 checks passed
@KJhellico KJhellico deleted the upd-l10n-po-test branch May 31, 2026 21:21
@arkid15r arkid15r mentioned this pull request Jun 1, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@cubic-dev-ai cubic-dev-ai[bot] cubic-dev-ai[bot] left review comments

@arkid15r arkid15r arkid15r approved these changes

@PPsyrius PPsyrius Awaiting requested review from PPsyrius PPsyrius is a code owner

Assignees

No one assigned

Labels

test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KJhellico @arkid15r