From 3c8aa171a15a39af9cc65cbec7c1e4ef8d6c6a47 Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Thu, 3 Sep 2026 02:51:22 +0200 Subject: [PATCH] Census the sqllogictest skips a run's own summary cannot report WITNESS: MobilityDuck main 238040ab2 carries 14 `mode skip` directives across its test files, and the summary its CI writes reports 102 test cases and no skip at all. The directive silences every statement after it to the end of the file or to a `mode unskip`, while the file still counts as one PASSING test case -- so no log-reading check can see it, whatever dialect it reads. A SECOND FINDING THE FIXTURE EXPOSED: `.test` belongs to no suffix set, so `is_text` refuses a sqllogictest file and NO rule of this check reads one. The first draft of the pattern therefore found nothing over a fixture carrying the directive outright, and the suffix is what makes the census possible at all. MEASURED: the rule reads 14 findings over that tree, one per directive, which is what an anchored grep of the same tree counts. MEOS-API's own tree stays at 0 -- its two prose mentions of the directive sit inside comments, and the pattern anchors to the start of a line because that is where a directive stands. A fixture carrying the directive fails while a fixture naming it in a comment passes, so the census discriminates the two rather than counting the word. The finding message becomes per-pattern: a build flag publishes an artefact from a suite that never ran, while this directive leaves the suite running and silent, and one sentence cannot state both. WHY: the log guard and this census answer different questions, and only the pair covers a sqllogictest consumer. A guard reading the run's summary alone answers `0 skipped` over all 14, which is a green tick placed exactly where the coverage left. --- .github/workflows/tree-hygiene.yml | 7 +++++ tools/check-tree-hygiene.py | 42 +++++++++++++++++++++++------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/.github/workflows/tree-hygiene.yml b/.github/workflows/tree-hygiene.yml index 880c704..4d0953e 100644 --- a/.github/workflows/tree-hygiene.yml +++ b/.github/workflows/tree-hygiene.yml @@ -48,6 +48,13 @@ jobs: > "$FIX/Dockerfile.fixture" echo "mvn package $SKIP" > "$FIX/build.sh" echo "nothing references the fixture image" > "$FIX/README.md" + # A sqllogictest file silenced from its directive onward. The runner + # still reports it as one passing test case, so this census is the + # only thing that sees it. + MODE="mode ""skip" + mkdir -p "$FIX/test/sql" + printf 'statement ok\nSELECT 1;\n\n%s\n\nquery I\nSELECT 2;\n----\n2\n' \ + "$MODE" > "$FIX/test/sql/fixture.test" git -C "$FIX" add -A if python3 "$GITHUB_WORKSPACE/tools/check-tree-hygiene.py" --root "$FIX"; then echo "::error::the check passed a tree carrying build output, a stale pin, a skipped suite and an orphan image" diff --git a/tools/check-tree-hygiene.py b/tools/check-tree-hygiene.py index 9b8a44f..8f9d5a6 100755 --- a/tools/check-tree-hygiene.py +++ b/tools/check-tree-hygiene.py @@ -13,7 +13,10 @@ # The tracked copy answered for fewer families than the catalog # the generator reads, so the two runs bound different surfaces. # skipped-tests -DskipTests and -Dmaven.test.skip in build commands, a jar -# published from a suite that never ran. +# published from a suite that never ran; and a sqllogictest +# `mode skip`, which silences the statements after it while the +# file still reports as a passing test case, so no summary and +# therefore no log-reading check can see it. # stale-pin a Dockerfile cloning MobilityDB at a release branch and a # personal fork, while the jar in the same image is generated # from the catalog of master. @@ -49,13 +52,30 @@ "dependency-reduced-pom.xml", "*/dependency-reduced-pom.xml", ) -# A suite that does not run cannot witness the surface the jar carries. +# The consequence each skip construct carries, named once and shared by the +# entries that earn it. +JAR = ("a jar whose tests did not run witnesses no surface") +SILENT = ("the file still reports as one passing test case, so the run's own " + "summary reads no skips over it") + +# A suite that does not run cannot witness the surface it is there to cover. +# Each entry carries the consequence it earns, because the two dialects fail +# differently: a build flag publishes an artefact from a suite that never ran, +# while a sqllogictest directive leaves the suite running and silent. SKIP_PATTERNS = ( - (re.compile(r"-DskipTests\b"), "-DskipTests"), - (re.compile(r"-Dmaven\.test\.skip\b"), "-Dmaven.test.skip"), - (re.compile(r""), ""), - (re.compile(r""), ""), - (re.compile(r"\bskipITs\b"), "skipITs"), + (re.compile(r"-DskipTests\b"), "-DskipTests", JAR), + (re.compile(r"-Dmaven\.test\.skip\b"), "-Dmaven.test.skip", JAR), + (re.compile(r""), "", JAR), + (re.compile(r""), "", JAR), + (re.compile(r"\bskipITs\b"), "skipITs", JAR), + # sqllogictest, which MobilityDuck's suite is written in. The directive + # silences every statement after it to the end of the file or to a + # `mode unskip`, and NOTHING REPORTS IT: the file still counts as one + # passing test case, so the runner's own summary reads `0 skipped` over it + # and only a census of the sources can see it. Anchored to the start of a + # line because that is where the directive stands; a prose mention of it + # inside a comment is not one. + (re.compile(r"^\s*mode\s+skip\b"), "mode skip", SILENT), ) # The chain derives every artifact from one MobilityDB commit on master. A clone @@ -85,6 +105,9 @@ TEXT_SUFFIXES = { ".sh", ".bash", ".yml", ".yaml", ".xml", ".md", ".py", ".java", ".sql", ".txt", ".cfg", ".toml", ".properties", ".json", ".conf", "", + # sqllogictest, which carries a suite as text and would otherwise be read + # by no rule at all. + ".test", } @@ -166,12 +189,11 @@ def check_skipped_tests(root, files, allow): if path == ALLOW_FILE or path.endswith("check-tree-hygiene.py"): continue for n, line in enumerate(read_lines(root, path), 1): - for pattern, name in SKIP_PATTERNS: + for pattern, name, why in SKIP_PATTERNS: if pattern.search(line): found.append(Finding( "skipped-tests", path, n, - f"`{name}` keeps the suite from running; a jar whose " - f"tests did not run witnesses no surface")) + f"`{name}` keeps the suite from running; {why}")) return found