Files
firmware/qa/static/test_shell_syntax.py
T
Oleksandr ShyshatskyiandClaude Opus 5 16d00be525 case-services.sh is 360 lines and one exit code. That contract is 44 tests now.
test/ has two test frameworks. run-tests.py + ffsim is 1,657 lines
reimplementing what pytest already does -- skip accounting, gating,
reporting, then parsing pytest's own JUnit XML back out -- and it calls
pytest as gate #5 of about twenty. Underneath it, 6,280 lines of
case-*.sh execute inside the qemu chroot and report 13 booleans between
them; case-moonraker.sh is 1,059 lines that collapse to one pass/fail.
Nothing runs in parallel and nothing can be selected: each replica gate
is its own `docker run --privileged`, re-registering binfmt and
re-assembling the mount layout, thirteen of them in sequence.

All three follow from one decision -- THE REPLICA CONTAINER IS TREATED
AS A TEST RUNNER INSTEAD OF AS A FIXTURE. That is what pushes assertions
into ash, which yields one bit per container, which then needs a bespoke
framework above pytest to aggregate the bits.

qa/ is the replacement, built beside test/ rather than inside it, with
two lanes and one framework. Both suites run in CI; test/ is still the
release gate; nothing has been deleted. docs/qa-migration.md has the
order the rest moves in, and the rule: a case script is deleted only
once its replacement here has been green, so there is never a window
where coverage drops.

THE MACHINE IS UNCHANGED. qa/ reuses test/integration/printer/'s
Dockerfile, entrypoint.sh, assemble.sh, binfmt.sh and seed-prog.sh as
they are, and its `docker run` argv is deliberately ffsim's, minus --rm
and plus -d. While both suites run they have to be testing the same
machine or neither proves anything about the other. The only new trick
is how the container is held open: entrypoint.sh ends by running the
case script it was given, and the one it is given here touches a marker
and sleeps, so setup completes exactly as it always has and the
container simply does not exit. The marker is also the readiness
signal, because setup ranges from under a second to over a minute and a
constant would either waste the fast case or fail the slow one.

TESTS RUN AGAINST A MACHINE THE REAL INSTALLER PRODUCED. The `printer`
fixture is a replica with the package installed by the printer's own
app_startup.sh, off a genuine FAT filesystem on /dev/sda1. The first
version of this tree did what case-services.sh does -- copy payload/*.sh
and payload/init.d/S* into $MODDIR by hand -- and two things were wrong
with it. It is a second implementation of the install, so everything
downstream asserted against a layout the harness built rather than one
the installer produced, and the real installer could have broken without
a single test noticing. And it can only place what is in payload/, so
everything the BUILD produces was missing: s6 is a build output that
bin/patch.sh stages into the package, so a hand-placed payload has no
supervisor at all, which is why the tests had to carry a stand-in
scanner and gate one assertion on whether it was in use.

Installing for real fixes both and deletes the stand-in. Hand-placed,
/usr/data/anvil held anvil-env.sh, anvil-service.sh, anvil.conf, init.d
and etc/s6. Really installed it holds VERSION, backup, bin, config,
config-installed, helixscreen, lib, libexec, moonraker, nginx and www as
well -- bin/lib/libexec being the cross-built s6 and CPython 3.13. The
file is SHORTER than the case script it replaces while reporting 44
results instead of one. case-install.sh's header records this same
lesson from the first time somebody learned it: "An earlier version of
this file replayed app_startup.sh by hand -- which meant a bug in our
reading of it could never be caught."

The install is the machine's own shell under qemu, so it is baked into
an image once per package and cached under a tag derived from the
package's md5 -- build-printer-image.sh's trick for the stock baseline.
Rebuild the package and the tag changes, so the next run bakes again
instead of testing yesterday's build; deliberately not a hand-written
staleness check, because those are the ones that eventually get it
wrong.

A MISSING TOOL OR RESOURCE FAILS, and there is no flag. This tree
briefly had --strict-skip=<lane|all>, which was ALLOW_SKIP inverted --
and ALLOW_SKIP's defect was never the direction it named things in, it
was that the decision lived in the CI invocation. An accept-list has to
be edited whenever a gate moves; a strict-list has to be remembered
whenever a job is added; both degrade silently when the command line is
wrong, which is the same disease as the printer-sim job that was gated
on an unset secret and never ran once in its existence. So the verdict
lives with the knowledge: no shellcheck, no pyflakes, no docker, no
daemon, no replica image, no built package -- each fails where it is
needed, with the fix in the message. There are no skips in this suite.

MEASURED, on monstrofil/creator5-printer:latest, both suites agreeing on
the same machine: 131 tests in 40s, all green; qa/static alone 1.3s with
no docker; one assertion selected by node id 2.9s, against the whole
360-line case being the smallest unit test/ can run. Granularity checked
by injecting the regression case-services.sh exists to catch -- deleting
SVC_EXTRA_VERBS="|force-start" from S70klipper -- which failed one named
test, printed the usage line that came back, left the other 41
reporting, and took 4s to re-run alone.

Two bugs the port found in its own harness, both only visible by
running it. bake.sh shipped without its executable bit, so the lane died
at container init with 43 errors all saying "could not install the
package" -- a file mode wearing an installer failure's clothes;
test_action_scripts_are_executable now guards it, the same check the
payload's own s6 run scripts already have. And an ELF magic assertion
used `od -An`, which the printer's busybox od does not have -- the
replica catching host-vs-printer drift in the test code itself, which is
the entire reason it exists.

Also here: qa/static/test_probes.py, which has no counterpart in test/.
Printer.ps() and Printer.listening() are parsers, and a parser is where
a harness goes quietly wrong -- a listening() that never matches makes
every readiness assertion pass by the absence of contradiction. Neither
failure looks like a failure; they look like a green suite. So they are
tested against captured output, in the static lane, with no docker
involved.

Two things found while surveying, neither fixed here.
case-pyext.sh is 567 lines with no gate in gates.py, no Makefile target
and no CI step: it has never run. And case-mcu-bringup.sh:4 says the
handshake is "tested elsewhere against a pty" -- there is no pty
anywhere in the repo, and every documented past bug in
ff_mcu_bringup.py lived in the code that comment excuses from testing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-28 09:35:14 +03:00

248 lines
11 KiB
Python

"""Every script parses, and none of them is secretly bash.
Ported from test/run-tests.py's check_shell_syntax and check_no_bashisms. The
logic is theirs; what changes is the granularity. There, one gate covered ~25
files and reported one bit, so a syntax error in S62moonraker and a syntax
error in patch.sh were the same red line. Here each file is its own test, named
after itself, and `pytest -k S62moonraker` runs exactly one.
The original's docstring made a deliberate choice worth preserving:
One line per check, not one per file: 25 green lines saying "syntax ok"
hide the two that matter.
That reasoning was about output noise, and it was right. pytest solves it a
different way -- passes collapse to dots, and only failures print -- so the
granularity and the quiet are no longer in tension.
WHY THE PAYLOAD IS CHECKED TWICE, WITH DIFFERENT DIALECTS
`bash -n` proves a file parses at all. shellcheck's dash dialect proves it uses
nothing bash-only, which is what matters for anything under payload/: the
printer runs busybox ash, not bash, and a bash-only construct parses perfectly
here and fails there. dash rather than sh as the dialect because busybox ash,
like dash, supports `local`, which the payload uses.
"""
import importlib.util
import os
import shutil
import subprocess
import sys
import pytest
from lib.paths import ROOT
pytestmark = pytest.mark.static
# ------------------------------------------------------------------ the sets
# Everything that must at least parse. bin/ and test/ run under bash on the
# build image; payload/ runs under the printer's ash but bash parses it too,
# and a file that does not parse under either is broken beyond dialect.
SYNTAX_GLOBS = ("bin/*.sh", "payload/*.sh", "payload/init.d/S*",
"payload/firmwareExe", "test/integration/printer/*.sh",
"qa/replica/actions/*.sh")
# The subset executed by the printer's busybox ash. bin/ and test/ are
# deliberately absent: they run on the build image, where bash is the shell and
# a bashism is not a defect.
#
# qa/replica/actions/ IS here, and that is not a formality. Those scripts are
# handed to entrypoint.sh and run inside the chroot exactly as a case script
# is, so they are as exposed to ash as anything under payload/. A bashism in
# hold.sh would fail at container start and read as "the replica is broken on
# this machine" -- a harness failure wearing a machine failure's clothes, which
# is the hardest kind to diagnose.
ASH_GLOBS = ("payload/*.sh", "payload/init.d/S*", "payload/firmwareExe",
"qa/replica/actions/*.sh")
PY_GLOBS = ("bin/*.py", "payload/*.py", "payload/bin/*.py",
"payload/klipper/extras/*.py", "test/*.py", "test/ffsim/*.py",
"test/integration/*.py", "qa/*.py", "qa/lib/*.py",
"qa/static/*.py", "qa/replica/*.py")
def _files(globs):
found = []
for pattern in globs:
found += sorted(p for p in ROOT.glob(pattern) if p.is_file())
return found
def _ids(paths):
return [str(p.relative_to(ROOT)) for p in paths]
SHELL_FILES = _files(SYNTAX_GLOBS)
ASH_FILES = _files(ASH_GLOBS)
PY_FILES = _files(PY_GLOBS)
# --------------------------------------------------------- the globs are live
#
# Each lane below asserts its own inputs exist. This is not defensive noise: it
# is the exact bug the original guarded against, quoted from check_no_bashisms:
#
# With no targets shellcheck writes usage to stderr and exits 1, leaving
# `hits` empty -- a green gate that examined nothing. check_shell_syntax
# guards this; this one did not, so moving payload/ would have retired the
# ash-compatibility check silently.
#
# Parametrized tests fail the same way, and worse: zero parameters is zero
# tests, which is not even a red line. It is an empty run that reads as clean.
def test_shell_globs_match_something():
assert SHELL_FILES, "no scripts to parse -- the globs in SYNTAX_GLOBS rotted"
def test_ash_globs_match_something():
assert ASH_FILES, "no payload scripts -- has payload/ moved?"
def test_python_globs_match_something():
assert PY_FILES, "no python files -- have the globs in PY_GLOBS rotted?"
ACTIONS = sorted((ROOT / "qa" / "replica" / "actions").glob("*.sh"))
@pytest.mark.parametrize("script", ACTIONS, ids=_ids(ACTIONS))
def test_action_scripts_are_executable(script):
"""qa/replica/actions/*.sh are handed to docker, not sourced.
bake.sh is a `--entrypoint` and the others are mounted in and run, so a
missing executable bit is not a style problem -- the container dies at
init with `exec: permission denied` before a single test runs, and the
43 errors that follow all say "could not install the package", which
points at the installer rather than at a file mode.
Caught here because it happened: bake.sh was written without the bit and
the whole lane failed in 1.6 seconds with an error about the mod. It is
the same failure the payload's own s6 `run` scripts have a check for --
"s6 reports a non-executable run in its own log and nowhere else" -- and
it deserved the same treatment here.
"""
assert os.access(str(script), os.X_OK), (
"%s is not executable, so docker cannot run it. `chmod +x` it and "
"commit the mode." % script.name)
# ------------------------------------------------------------------- it parses
@pytest.mark.parametrize("script", SHELL_FILES, ids=_ids(SHELL_FILES))
def test_parses(script):
parsed = subprocess.run(["bash", "-n", str(script)],
capture_output=True, text=True)
assert parsed.returncode == 0, parsed.stderr.strip()
# ------------------------------------------------------ it is not secretly bash
@pytest.fixture(scope="session")
def bashisms():
"""shellcheck over every payload script at once, indexed by file.
One process, not one per file. shellcheck spends most of a run starting
up, so per-file invocation cost more than everything else in this lane
combined -- but per-file REPORTING is the whole point of the port. The
gcc output format is `path:line:col: level: message [SCxxxx]`, so the
findings can be split back apart by path afterwards and each test can ask
only about itself.
"""
# A failure, not a skip. There is no configuration in which running this
# suite without shellcheck is a complete run, so calling it "skipped" would
# describe a machine that is not set up as though it were a decision
# somebody made. See qa/conftest.py.
#
# pytest.fail rather than a bare assert on which(): `assert None` is what
# an assert would lead with, and the sentence explaining how to fix it
# would be the part that got truncated in the summary.
if not shutil.which("shellcheck"):
pytest.fail(
"shellcheck is not installed, so nothing checked that the payload "
"is free of bash-only constructs -- the printer runs busybox ash. "
"The build image has it: run `make qa`, or install it locally "
"(dnf install ShellCheck / apt install shellcheck).")
checked = subprocess.run(
["shellcheck", "-s", "dash", "-f", "gcc"]
+ [str(p) for p in ASH_FILES],
capture_output=True, text=True, cwd=str(ROOT))
# SC3xxx is shellcheck's "not supported in POSIX sh" family; SC2039 is its
# older spelling of the same thing. Everything else it says here is style,
# and a gate that goes red for style gets switched off.
found = {}
for line in (checked.stdout or "").splitlines():
if "SC3" not in line and "SC2039" not in line:
continue
path = line.split(":", 1)[0]
found.setdefault(path, []).append(line)
# shellcheck exits 1 for findings, which `found` covers, but also for a
# usage or file error -- which would otherwise pass unnoticed as "no
# findings". Raised here rather than returned so it fails every test in
# the lane instead of hiding behind whichever file happens to be clean.
if not found and checked.returncode not in (0, 1):
raise AssertionError(
"shellcheck failed to run (exit %d): %s"
% (checked.returncode, (checked.stderr or "").strip()[:200]))
return found
@pytest.mark.parametrize("script", ASH_FILES, ids=_ids(ASH_FILES))
def test_no_bash_only_constructs(script, bashisms):
# shellcheck echoes back the path it was given. It was given absolute
# paths, but match on both spellings so a future cwd-relative call site
# does not silently stop matching and report every file clean.
rel = str(script.relative_to(ROOT))
hits = bashisms.get(str(script), []) + bashisms.get(rel, [])
assert not hits, "bash-only constructs:\n " + "\n ".join(hits)
# ------------------------------------------------------- every name is bound
# One test, not one per file: pyflakes resolves names within a module, so the
# per-file split buys nothing here and costs an interpreter start each. The
# failure message names the files itself.
def test_no_undefined_names():
"""A name that does not exist anywhere it is read.
py_compile catches typos in syntax; nothing caught typos in names, so a
rename that renamed an assignment and missed one of its uses stayed green
through both lanes and waited for the line to execute on the printer. That
is how `_plate_check` came to reach for `z_trigger` when what it had bound
was `station_z` -- on the one path that runs BEFORE a nozzle is allowed to
descend.
"""
# The debian package ships the module without putting a pyflakes
# executable on PATH, so shutil.which() -- the shape the shellcheck test
# above uses -- reports it missing on the very image that has it.
#
# A failure, not a skip, for the same reason as shellcheck above.
if importlib.util.find_spec("pyflakes") is None:
pytest.fail(
"pyflakes is not installed, so nothing checked that every name is "
"bound -- which is the gate that catches a rename missing one of "
"its uses. The build image has it: run `make qa`, or install it "
"locally (apt install python3-pyflakes / pip install pyflakes).")
checked = subprocess.run(
[sys.executable, "-m", "pyflakes"] + [str(p) for p in PY_FILES],
capture_output=True, text=True, cwd=str(ROOT))
# Only the fatal findings count. pyflakes also reports unused imports and
# the like, which are untidy but cannot fail at runtime.
fatal = ("undefined name", "referenced before assignment",
"syntax error", "invalid syntax")
hits = [line for line in (checked.stdout or "").splitlines()
if any(marker in line.lower() for marker in fatal)]
assert hits or checked.returncode in (0, 1), (
"pyflakes failed to run (exit %d): %s"
% (checked.returncode, (checked.stderr or "").strip()[:200]))
assert not hits, "names used but never bound:\n " + "\n ".join(hits)