Repository navigation
test(i18n): run the babel_update.sh msgcat tests on CI - #45247
Conversation
The Python-Unit runner has no gettext, so both msgcat tests skip on CI. Where msgcat exists, the trailing-line test fails with `python: command not found`. The script calls `python`, and the test's restricted `PATH` has none. Both end-to-end tests now put `python` and `python3` shims on that `PATH`. Each shim runs the interpreter that runs pytest. A shared helper stages the script, the stamping script and an empty registry. Before this change, the failing-msgcat test passed with the msgcat `|| exit 1` removed. The script stopped one step later, at the missing `python`. Python-Unit and the nightly canary install gettext. On CI, a missing msgcat fails the two tests instead of skipping them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The msgcat tests failed instead of skipping whenever `CI` was set. GitHub, GitLab, CircleCI and Buildkite all set `CI`. A pipeline outside apache/superset without gettext would get two new failures. The tests now call `_require_msgcat()`. It fails only when `SUPERSET_GETTEXT_STRICT` is set, and skips otherwise. Python-Unit and Python-Nightly-Canary set it at job level, next to their gettext step. This copies `SUPERSET_TESTCONTAINERS_STRICT` from apache#43502. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The shims wrapped `sys.executable` in double quotes inside a `/bin/sh` script. A path with a double quote, `$` or a backtick broke the shim. `shlex.quote` covers every path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
CodeAnt PR Risk: Low Risk
Assessed commit: |
hainenber
left a comment
There was a problem hiding this comment.
I don't agree with having an additional arbitrary SUPERSET_ env var only to initiate a test.
|
@hainenber Thanks for the review. What would you prefer here? My proposal is to remove
With gettext installed, both tests run and pass on CI. If a later change removes the gettext step, they skip again without a failure. Does that work for you, or do you have another approach in mind? |
|
Pretty much as you say, except for one thing: make the unit tests failing if they're ran in CI env, i.e. Don't let things skippable in controlled env like CI |
Review asked to drop `SUPERSET_GETTEXT_STRICT`. CI is a controlled environment, so a missing msgcat there must fail the tests, not skip them. `_require_msgcat()` checks `CI` instead. Both workflows drop the variable and keep the gettext step. A local run without gettext still skips. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@hainenber Done in 482e951.
Local results at 482e951: I updated the PR description to match. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45247 +/- ##
==========================================
+ Coverage 83.14% 83.23% +0.09%
==========================================
Files 3029 3037 +8
Lines 196748 197921 +1173
Branches 45967 46128 +161
==========================================
+ Hits 163580 164741 +1161
- Misses 29944 29955 +11
- Partials 3224 3225 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
EnxDev
left a comment
There was a problem hiding this comment.
Read through this one and it looks good to me. The python/python3 shims fix the real reason the trailing-line test blew up on macOS and Debian, and staging the registry in _stage_script means the failing-msgcat test now actually reaches msgcat instead of dying one line later.
The SUPERSET_GETTEXT_STRICT var hainenber objected to is gone and the strict path keys off CI as asked. Python-Unit and the canary are the only workflows that run tests/unit_tests, so nothing else with CI set should start failing on a missing msgcat. CI is green.
No inline notes from me.
hainenber
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your contribution
SUMMARY
Updated after review at
482e951e05: the strict check readsCI, not a newSUPERSET_variable.Fixes #45245.
#44395 added two tests that run the real msgcat step of
scripts/translations/babel_update.sh. They never run on CI, and one of them fails on any machine that has gettext.shutil.which("msgcat")isNoneand both tests skip. The JUnit report of the Python-Unit run on master at7aadf35e7dlists both as skipped (https://github.com/apache/superset/actions/runs/38381761342).PATH(/usr/bin:/binplus stubs). The script callspythonon line 77. macOS, Ubuntu 26.04 and Debian 13 have no/usr/bin/python. The trailing-line test fails withline 77: python: command not found.|| exit 1removed, the script still stopped beforepybabel update. It stopped one line later, at the missingpython.This PR changes three things:
pythonandpython3shims on the restrictedPATH. Each shim runs the interpreter that runs pytest. A shared_stage_scripthelper replaces the setup that two tests repeated.superset-translations.yml, with the same pinned action. Like that step, it leavesexecute_install_scriptsoff. The gettext package needs no post-install script._require_msgcat(). When msgcat is missing, it fails the test ifCIis set and skips it otherwise. CI is a controlled environment, so a missing msgcat there means a misconfigured job.babel_update.shitself does not change.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Local runs at
482e951e05, on master139a06eee1.Before (master's test file, macOS with Homebrew gettext):
After (this branch):
The same runs in Docker, with only
python3,python3-pytestandgettextfrom apt, andCI=true:/usr/bin/pythonubuntu:26.04(the CI runner's release)ubuntu:26.04withpython-is-python3debian:13Each test fails when the behavior it guards breaks. Each test stays green on a harmless change:
babel_update.shor to the test|| exit 1after msgcat|| exit 2pythonshims (macOS)pythoncall topython3Without msgcat on
PATH:Python-Unit on this PR at
482e951e05(https://github.com/apache/superset/actions/runs/38441942670). The JUnit report lists all sixbabel_update_test.pytests as passed, and none as skipped:Python-Nightly-Canary on my fork, with Python 3.13.15, on the first commit of this branch. The workflows at
482e951e05are identical to that commit.All six tests passed, and none skipped. The job then hit its 45-minute timeout, 3 seconds after the last test. The canary on master has the same timeout problem, which #45246 describes.
TESTING INSTRUCTIONS
brew install gettextorapt-get install gettext).pytest tests/unit_tests/scripts/translations/babel_update_test.py. Expect 6 passed. On master, expect the trailing-line test to fail withpython: command not found.PATHand run the same command withCI=true. Expect 2 failed. WithoutCI, expect 2 skipped.junit-results-currentartifact. It lists both msgcat tests as passed, not skipped.All 136 tests in
tests/unit_tests/scripts/translations/pass locally with gettext andCI=true, also with Python-Unit's-n auto --dist loadfile. No other test intests/depends on gettext.check_translation_regression_test.pymocks itsmsgfmtcall.The superset-pr-kit checks skipped three items: toolchain and frontend (no frontend files changed), and catalogs (the translation template did not change).
ADDITIONAL INFORMATION
🤖 Generated with Claude Code