Add translation completeness test for Satellite locales - #22334
Open
pnovotny wants to merge 3 commits into
Open
Conversation
New test `test_positive_check_missing_translations` for untranslated messages and missing template strings. Supported languages: fr, ja, ko, zh_CN
pnovotny
added
the
Stream
Contributor
Reviewer's GuideAdds a parametrized pytest that enforces translation completeness for selected Satellite locales using gettext utilities, introduces a SUPPORTED_LANGUAGES constant for those locales, and wires the new test file into CODEOWNERS, with xfail handling for known open translation issues. Sequence diagram for translation completeness pytestsequenceDiagram
actor Developer
participant Pytest
participant test_translation
participant GettextTools
Developer->>Pytest: run tests with test_positive_check_missing_translations
Pytest->>test_translation: test_positive_check_missing_translations(SUPPORTED_LANGUAGES)
loop for each language in SUPPORTED_LANGUAGES
test_translation->>GettextTools: msgfmt
test_translation->>GettextTools: msgattrib
test_translation->>GettextTools: msgcmp
alt [known translation issue]
test_translation->>Pytest: pytest.xfail
else [no known issue]
Pytest-->>Developer: assert no missing or untranslated strings
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Author
|
trigger: test-robottelo |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
FIND_POTcommand relies on shell brace expansion ({katello,foreman,hammer_cli}), which may not be portable across different default shells; consider expanding this into explicit paths or building the list programmatically to avoid shell-specific behavior. - In the
untranslated_reportsummary, the aggregated error message does not include the.pofilenames, only statistics and messages; add the corresponding file path to each section so failures can be traced back to the specific translation file. - When mapping
.potfiles to related.pofiles,pot_name in pocan overmatch (e.g., when one template name is a substring of another); consider matching against the.pobasename stem instead of a substring to avoid incorrect associations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `FIND_POT` command relies on shell brace expansion (`{katello,foreman,hammer_cli}`), which may not be portable across different default shells; consider expanding this into explicit paths or building the list programmatically to avoid shell-specific behavior.
- In the `untranslated_report` summary, the aggregated error message does not include the `.po` filenames, only statistics and messages; add the corresponding file path to each section so failures can be traced back to the specific translation file.
- When mapping `.pot` files to related `.po` files, `pot_name in po` can overmatch (e.g., when one template name is a substring of another); consider matching against the `.po` basename stem instead of a substring to avoid incorrect associations.
## Individual Comments
### Comment 1
<location path="tests/foreman/cli/test_translation.py" line_range="78-82" />
<code_context>
+ }
+
+ # Step 4: Compare .po files against their .pot templates
+ result = target_sat.execute(FIND_POT)
+ assert result.status == 0, f'Failed to search for .pot files: {result.stderr}'
+ pot_files = result.stdout.strip().splitlines()
+
+ template_failures = {}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that at least one .pot template is found to fully match the documented expected results
The docstring states that all translation files are found on the Satellite, but this test only asserts that `find` succeeds and never verifies that any `.pot` files were actually discovered. If `FIND_POT` matches nothing, the template checks are silently skipped and the test can still pass. To ensure the test enforces its documented behavior and catches packaging/path issues, add an assertion that `pot_files` is non-empty (e.g. `assert pot_files, 'No .pot template files found on the Satellite'`).
```suggestion
result = target_sat.execute(FIND_POT)
assert result.status == 0, f'Failed to search for .pot files: {result.stderr}'
pot_files = result.stdout.strip().splitlines()
assert pot_files, 'No .pot template files found on the Satellite'
template_failures = {}
```
</issue_to_address>
### Comment 2
<location path="tests/foreman/cli/test_translation.py" line_range="66-68" />
<code_context>
+
+ # Step 2 & 3: Check each .po file for untranslated messages
+ untranslated_report = {}
+ for po_file in po_files:
+ stats = target_sat.execute(f'msgfmt -v --statistics -o /dev/null {po_file}')
+ if 'untranslated' in stats.stderr:
+ details = target_sat.execute(f'msgattrib --untranslated --indent --no-wrap {po_file}')
+ untranslated_report[po_file] = {
</code_context>
<issue_to_address>
**issue (testing):** Add assertions on `msgfmt` execution status to avoid silently treating tool failures as translation issues
The loop only checks `stats.stderr` for `untranslated` and ignores `stats.status`, so a failed `msgfmt` run (missing binary, malformed `.po`, permission issues, etc.) could be misread as valid statistics or missed entirely. Please assert `stats.status == 0` before inspecting stderr, or add a separate failure branch that fails the test and surfaces `stats.stderr` when the status is non‑zero.
</issue_to_address>
### Comment 3
<location path="tests/foreman/cli/test_translation.py" line_range="88-90" />
<code_context>
+ pot_name = os.path.splitext(os.path.basename(pot_file))[0]
+
+ related_pos = [po for po in po_files if po.startswith(locale_dir) and pot_name in po]
+ for po_file in related_pos:
+ cmp_result = target_sat.execute(f'msgcmp {po_file} {pot_file}')
+ not_defined = []
+ for line in cmp_result.stderr.splitlines():
+ if f'not defined in {po_file}' in line:
</code_context>
<issue_to_address>
**issue (testing):** Check `msgcmp` return status to distinguish missing translations from tool or environment failures
`msgcmp` stderr is parsed to find missing translations, but `cmp_result.status` is never checked. If `msgcmp` fails (missing binary, bad input, etc.), stderr may not match the expected format and the test could silently misclassify or miss the failure. Please assert on `cmp_result.status` (e.g. `assert cmp_result.status == 0`) or otherwise handle non‑zero exit codes separately to keep failures from the comparison tool distinct and clearly reported.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Passed
Contributor
Author
|
trigger: test-robottelo |
pnovotny
removed
the
PRT-Passed
Collaborator
|
PRT Result |
Satellite-QE
added
the
PRT-Failed
The command returns RC 1 also for missing template strings, which we want to catch.
Contributor
Author
|
trigger: test-robottelo |
Collaborator
|
PRT Result |
Satellite-QE
added
PRT-Passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Statement
There was no automated test to verify that Satellite's translation files (.po) are complete — untranslated messages and missing template strings could ship undetected.
Solution
Related Issues
https://redhat.atlassian.net/browse/SAT-32747
PRT test Cases example
Summary by Sourcery
Add automated CLI test to ensure Satellite translation files are fully translated and complete relative to their templates for selected locales.
Enhancements:
Tests:
Chores: