bito-code-review[bot] commented on code in PR #44395: URL: https://github.com/apache/superset/pull/44395#discussion_r4049501364
########## 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: <div> <div id="suggestion"> <div id="issue"><b>Ordering test targets comment</b></div> <div id="fix"> `text.index("pybabel update")` matches the comment at script line 66 ("`pybabel update` below"), not the real command at line 82. So `msgcat_at < update_at` compares msgcat against a comment and passes even if `pybabel update` were moved before msgcat — the ordering guard this test claims to provide is ineffective. Anchor on the command line, e.g. `text.index("pybabel update \\")`. </div> </div> <small><i>Code Review Run #705dd0</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
