glaterza commented on code in PR #44395: URL: https://github.com/apache/superset/pull/44395#discussion_r4076916477
########## scripts/translations/babel_update.sh: ########## @@ -49,8 +49,14 @@ pybabel extract \ --project=Superset \ -k _ -k __ -k t -k tn:1,2 -k tct . -# Normalize .pot file -msgcat --sort-by-msgid --no-wrap --no-location superset/translations/messages.pot -o superset/translations/messages.pot +# Normalize the .pot: sort by msgid, keep each message on one line, drop source +# locations. msgcat's sort flag is `--sort-output`; the `--sort-by-msgid` +# spelling used here before is not a msgcat option, and gettext rejects the call +# with `unrecognized option` (verified on 0.23.2 and 1.0). With no `set -e` in +# this script that failure was non-fatal, so the step never ran and the template +# was published unnormalized. `|| exit 1` makes a failed normalization stop the +# script instead. +msgcat --sort-output --no-wrap --no-location superset/translations/messages.pot -o superset/translations/messages.pot || exit 1 Review Comment: Both halves are covered in `tests/unit_tests/scripts/translations/babel_update_test.py`: - `test_normalization_sorts_unwraps_and_drops_locations` runs the script's own msgcat command on a small fixture that is unsorted, width-wrapped and has location comments. The test parses the command out of the script instead of copying it, so it tests the real script. It checks that the output is sorted, unwrapped and has no locations. - `test_a_failing_msgcat_stops_the_script_before_pybabel_update` runs the real script with a stub msgcat that fails the way gettext does on a bad flag. It checks that the script exits non-zero and never calls `pybabel update`. I confirmed that each test fails when the thing it guards is broken: the bad flag restored, `--no-wrap` dropped, `|| exit 1` removed, or `pybabel update` moved above msgcat. Running the whole script end to end during this rebase also turned up a second bug, in the trailing-line strip. It's now fixed and tested in this PR; details in the PR comment. ########## tests/unit_tests/scripts/translations/babel_update_test.py: ########## @@ -0,0 +1,268 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Tests for the .pot normalization step in ``scripts/translations/babel_update.sh``. + +The defect these prove against: the step was written with ``--sort-by-msgid``, +which is not a msgcat option. gettext rejected the call with ``unrecognized +option``, and because the script has no ``set -e`` the failure was non-fatal — so +the normalization silently never ran and ``pybabel update`` continued on an +unnormalized template. Measured on the template at the time of the fix: 1294 +width-wrapped lines, 1 location comment and 7 msgid sort inversions that the +flags were supposed to have removed. + +``check_translation_regression.py`` cannot catch this class: it compares .po +translation *counts*, which are unchanged by losing ``--no-wrap`` or by +continuing past a failed normalization. + +Two guarantees are pinned here, and both are asserted against the real script +rather than a copy of its logic: + +1. **The normalization does what it claims** — sorted, unwrapped, location-free. + The msgcat command is *parsed out of the script* so that a regression in the + script fails this test. A test carrying its own copy of the command would go + on passing while the script broke, which is the exact shape of the original + bug. +2. **A failed normalization stops the script** before ``pybabel update`` can + publish catalogs built from an unnormalized template. +""" + +import re +import shutil +import stat +import subprocess +from pathlib import Path + +import pytest + +_SCRIPT_PATH = ( + Path(__file__).resolve().parents[4] / "scripts" / "translations" / "babel_update.sh" +) + +# Long enough that gettext folds it on width alone, which is what --no-wrap +# governs. Declared once so the fixture and the assertion cannot drift apart. +_LONG_MSGID = ( + "a very long message that gettext would rather fold across several " + "physical lines because it exceeds the default wrapping width of the tool" +) + +# An unsorted, width-wrapped, location-carrying template — every property the +# normalization is meant to fix, in one fixture. +_UGLY_POT = f"""\ +# Translations template for Superset. +msgid "" +msgstr "" +"Content-Type: text/plain; charset=utf-8\\n" + +#: superset/zebra.py:1 +msgid "zebra" +msgstr "" + +#: superset/views/core.py:42 +#: superset/views/dashboard.py:7 +msgid "" +"{_LONG_MSGID[:66]}" +"{_LONG_MSGID[66:]}" +msgstr "" + +#: superset/apple.py:9 +msgid "apple" +msgstr "" +""" + + +def _script_text() -> str: + return _SCRIPT_PATH.read_text(encoding="utf-8") + + +def _msgcat_line() -> str: + """The script's msgcat invocation, as the script actually spells it.""" + calls = [ + stripped + for line in _script_text().splitlines() + if (stripped := line.strip()).startswith("msgcat ") + ] + assert calls, ( + "no msgcat invocation found in babel_update.sh — the .pot normalization " + "step is missing entirely" + ) + return calls[0] + + +def test_the_normalization_step_exists_and_uses_real_msgcat_flags() -> None: + """Guards the original typo and the two flags whose loss is invisible.""" + line = _msgcat_line() + + assert "--sort-output" in line, ( + "the .pot normalization must sort with `--sort-output`. `--sort-by-msgid` " + "is not a msgcat option and gettext rejects the whole call, which is how " + "this step came to be a silent no-op." + ) + assert "--sort-by-msgid" not in line, ( + "`--sort-by-msgid` is not a msgcat option — see `msgcat --help`" + ) + for flag in ("--no-wrap", "--no-location"): + assert flag in line, ( + f"the .pot normalization must pass {flag}; losing it changes the " + "template's shape without changing any translation count, so no " + "other check in the repo would notice" + ) + + +def test_a_failed_normalization_is_fatal() -> None: + """`|| exit 1` — without it, a broken msgcat is a silent no-op.""" + assert re.search(r"\|\|\s*exit\s+1\s*$", _msgcat_line()), ( + "the msgcat call must end with `|| exit 1`. This script has no `set -e`, " + "so an unguarded failure here is ignored and `pybabel update` proceeds " + "on an unnormalized template." + ) + + +def test_normalization_precedes_pybabel_update() -> None: + """Order matters: normalizing after the update pass would not help.""" + text = _script_text() + msgcat_at = text.index(_msgcat_line()) + update_at = text.index("pybabel update") Review Comment: Changed in d75bc8da7d. The test now compares command lines only, so a comment that mentions `pybabel update` can no longer stand in for the command. My earlier reply was right about the case described here: moving the command above `msgcat` did fail the old test. But I missed the opposite case. With a comment mentioning `pybabel update` placed above `msgcat`, the old test failed even though the script was correct. I checked that the old version fails on that script and the new one passes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
