Jens Geyer created THRIFT-6113:
----------------------------------
Summary: test_keyword_escape.py regression test silently skips
(thrift compiler not found) in CI and local builds
Key: THRIFT-6113
URL: https://issues.apache.org/jira/browse/THRIFT-6113
Project: Thrift
Issue Type: Bug
Components: Python - Library
Reporter: Jens Geyer
The Python keyword-escaping regression test added in THRIFT-5927
(lib/py/test/test_compiler/test_keyword_escape.py) never actually exercises the
compiler in CI, and can pick up the wrong compiler locally.
Root causes:
1. lib/py/Makefile.am's check-local target invokes the test as a plain
"$(PYTHON) test/test_compiler/test_keyword_escape.py", without passing the
THRIFT make variable through as an environment variable. Every other
Makefile.am in the tree that needs the compiler at test time defines/exports
THRIFT explicitly (e.g. "THRIFT = $(top_builddir)/compiler/cpp/thrift"); this
one doesn't, so the test script's os.environ['THRIFT'] lookup is always empty
when run via "make check".
2. test_keyword_escape.py's find_thrift() fallback then tries two relative-path
guesses to locate the compiler in the build tree, but both are off by one
directory level (3 x "../" instead of 4, from lib/py/test/test_compiler/), so
they resolve to lib/compiler/cpp/thrift and lib/build/compiler/cpp/bin/thrift
instead of the correct compiler/cpp/thrift and build/compiler/cpp/bin/thrift.
With neither the environment variable nor the fallback working, and no
system-wide "thrift" in PATH on a clean CI runner, find_thrift() returns None
and the test prints "WARNING: thrift compiler not found, skipping test" and
returns 0 (success). This has been true on every CI run since the THRIFT-5927
fix was merged (see .github/workflows/build.yml, lib-python job, which
downloads the compiler artifact to exactly compiler/cpp/thrift -- the path the
broken fallback fails to reach), so the regression test currently provides no
actual coverage in CI.
Locally, the same missing-env-var issue means find_thrift() falls through to a
PATH search ('thrift', /usr/local/bin/thrift, /usr/bin/thrift) before ever
trying the build tree; if a different thrift binary happens to be on PATH (e.g.
from a prior "make install" or a distro package), the test silently exercises
that binary instead of the one just built, which can resurface already-fixed
issues under a misleading error.
Fix: pass THRIFT=$(THRIFT) through explicitly on the check-local recipe line,
matching the convention used elsewhere in the tree, and correct the off-by-one
in the relative-path fallback so standalone invocations (without make) still
work too.
Found while investigating a report on THRIFT-5927 that the fix appeared broken
on master; confirmed the generator fix itself is correct -- this is a
test/build-infrastructure gap only.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
