Improve local/CI checks: No stage can pass "accidentally" - #42
Merged
Conversation
The coverage section had the same two holes the sanitizer section just closed: the
configure omitted RAPIDPROTO_BUILD_TESTS=ON, so a cached OFF let the build exit 0
doing nothing and grade a stale binary, and llvm-profdata's status was discarded,
so a failed merge left the previous run's profile in place and the floor was graded
against it. Also clear DEBUGINFOD_URLS for the ASan run, print the coverage build
output on failure, and stop the "no Catch2 output" guard firing on a passing summary
("assertions in N test cases" has no colon, so it never matched).
The clang-tidy fan-out discarded xargs' status, and tidy_one reports by writing a log, so a fan-out that linted nothing was indistinguishable from a clean one: JOBS=abc gave ALL GREEN with zero of 48 TUs linted. Its mkdir is checked too -- this file runs without set -e. Also: the previous commit's guard fix silenced the at-exit case it was meant to label, so both test-failure branches now name the three shapes separately; a failed profdata merge no longer prints a percentage read off the stale profile; the debuginfod and "exits 0 doing nothing" comments state the measured mechanism; check_doc_links.py fails on zero files; regen_goldens.sh keeps the suite's status.
regen_goldens.sh captured the suite's status as a separate statement, which under errexit aborts at the assignment: the failure branch could never run, and a crashed regeneration exited with no output at all -- less than before it was "fixed". Use the `|| rc=$?` form. In the tidy stage, a shard selecting zero TUs reported `tidy clean (0 TUs)`, and tidy_one's crash branch returned 0 even when it could not write its report, so an unwritable log dir turned a crashing clang-tidy into a clean run. Both writes now decide the function's status. Drop a superseded comment left above its replacement, and correct two that described mechanisms the code does not have.
The c++20/23 smoke wrote its translation unit unchecked, and that file persists
across runs: a failed write left the previous run's TU in place and the stage
compiled that, passing without including the generated headers it exists to check.
The write is a single printf, not a `{ ... } > file` group -- bash reports a failed
redirect on a group as success when the group is the condition of `if !`, which
left the first version of this guard inert.
corpus_gate lumped "never fetched" together with "checked out at the wrong commit",
so a corpus whose pins were all stale reported the gate's loudest state as a green
self-skip -- while testing it the other way round would have failed CI's deep job,
which legitimately runs with no corpus at all. Now three states, skip only for the
absent one. Also range-check RAPIDPROTO_TIDY_SHARD, which aliased 0/3 onto 3/3.
mkdir -p build and the mktemp that creates $LOG ran unchecked before every stage, so an unwritable build/ surfaced as a cascade of unrelated failures -- an empty $LOG turning every stage's log path into /<name> -- rather than one line naming it. The shard check admitted a leading zero: 08/3 reached [[ -lt ]] and aborted it as an invalid octal digit, and 010/12 was accepted as slice 8, leaving slice 10 unlinted while all twelve jobs passed. Normalize with 10#. Also correct the cxx20 comment's account of the bash behaviour it rests on: a failed redirect on a compound command aborts it with status 1 and DISCARDS a leading `!`, so `if !` takes the else branch -- it does not report success, and the `|| return 1` form elsewhere in this file is unaffected.
The symlink pair three lines below the checks added last commit was still bare: a build/gate-logs that cannot be replaced leaves it pointing at an EARLIER run, so `cat build/gate-logs/<stage>` showed that run's output while this one reported ALL GREEN -- the confusion the per-run directory exists to end. stage_fuzz_seeds treated a seed directory it could not create as its documented "no seeds staged" warning, leaving the tier green with a target fuzzing from nothing; that case is now a failure while the warning keeps its meaning. streamgen_compile_bench.sh's mktemp was unchecked inside the gcc stage. Also: exit-2 messages go to stderr like the others, drop the cxx20 mkdir made dead by the file-scope check, and scope its comment to the default gate (quick and deep exit before it).
job_build_test skipped a missing or non-executable consumer example with a bare `continue`: chmod -x on both binaries gave ALL GREEN with no mention, and the build does not relink them, so a renamed or disabled target read as a clean run. It is the last stage of the default gate that could pass without doing its work. The seed check added last commit described the wrong scenario. A directory that cannot be CREATED was already red -- libFuzzer refuses to start on a missing corpus. The silent one is a stale directory that cannot be REMOVED: mkdir -p succeeds, the copies fail quietly, and the emptiness warning stays quiet because the previous run's seeds are still there. Two conditions, two messages, and the caller now honours the failure instead of launching the target anyway. Also: the three regen_*_goldens.sh reported success on a zero-match find, and the exit-2 paths leaked their log directory past the reaper.
The previous commit claimed the examples were the last stage of the default gate able to pass without doing its work. They were not. The two cmake --preset calls that feed job_build_test never pinned RAPIDPROTO_BUILD_TESTS, so a cached OFF left both presets running a stale binary: an injected failing assertion never compiled and the stage reported the same 9347 assertions, ALL GREEN. build/san and build/cov each pin it with a comment saying exactly this; the two configures at the top of the same file did not. RAPIDPROTO_REGEN_GOLDEN inherited from the environment turned the gate into a rewrite: the test binary regenerated the goldens it was meant to verify, 100 assertions disappeared, and corrupted fixtures were silently overwritten to match the code while the run reported green. The gate now ignores the variable and says so; regeneration stays with tests/regen_*_goldens.sh. Also correct three comments: a renamed or disabled target is caught by the pinned configure and not by the executable check, a missing golden directory already failed through errexit, and the stage table drives less than it claimed.
The guard added last commit was bypassable and half-placed. It tested the value,
while the tests test presence with getenv() != nullptr, so RAPIDPROTO_REGEN_GOLDEN=
went straight past it -- goldens rewritten, ~100 assertions gone, green, and no
warning. And it sat below the fix/quick/deep branches, so quick and deep never
reached it at all. Now `-v`, above the dispatch.
Four more stages could report success without doing their work:
- a renamed example target leaves the previous binary on disk and executable, so
-x passes and a stale binary runs; checked against the preset's target list
- a tests/test_*.cpp never added to CMakeLists.txt is compiled by nothing and run
by nothing, and the assertion count does not move; job_fixtures now requires
every TEST_SRC entry to appear there
- a stage in STAGE_KEYS but in no run list is disabled by omission, and the log
blamed an unset RAPIDPROTO_GATE_STAGES; rejected at startup, message corrected
- the fuzz seed emptiness check could not fire for the parser target (the
synthetic depth seeds are written after the corpus copy), so a moved
tests/corpus dropped it from 28 seeds to 2 in silence; counted, and empty is
now a failure rather than a note
Also: regen_goldens.sh configured without pinning BUILD_TESTS, the same stale-binary
defect, in the script that rewrites the goldens; and N=1 turned the stress compile
into a 1-field no-op. Restore the regen comment this commit's parent replaced with
an invented mechanism.
The previous commit added a stale-binary check to the two example binaries and left rapidproto_tests -- the 9347-assertion binary the stage exists to run -- with none. Renaming it ran the previous build: the injected failure lived in a freshly built rapidproto_tests_v2 while the stage reported the stale binary's 9212 assertions as ALL GREEN. ensure_gcc_binaries generalizes to ensure_targets <build-dir>, which both verifies the target exists and BUILDS it by name; existence alone is not enough, since a target excluded from `all` stays listed and is never rebuilt. Applied to the test binary and both examples on each preset, to build/san and build/cov (a rename there would have graded the sanitizers and the coverage floor against a stale binary), and to regen_goldens.sh, which would have rewritten every golden from one. The TEST_SRC check added last commit was a substring search over the whole file, so a source commented out "while debugging" passed while 135 assertions and 18 test cases vanished. It now matches the comment-stripped add_executable block. Also count the wire/arena/stream seeds instead of inferring emptiness from ls -A, which a leftover payload-seeds directory masked: moved fixtures gave 1 seed instead of 4 and reported clean. Drop a false claim that a BUILD_TESTS=OFF compile database makes clang-tidy lint nothing -- it exits 1 with 58 diagnostics.
All three regen scripts built rapidprotoc unguarded, so a renamed target made `--target rapidprotoc` a silent no-op and every CLI-produced golden was rewritten from the stale binary -- the previous commit guarded only the half of that pipeline driven by rapidproto_tests. quick had the same stale-target hole the full gate just had. ensure_targets moves above the branch dispatch, since quick now calls it. The fixtures block extraction was anchored to a closing paren at exactly two spaces: reindenting CMakeLists.txt ran the range to EOF and degraded the check back into the whole-file substring search it replaced. It is now indentation-agnostic and requires the range to have terminated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check.shhad several stages that passed on the absence of failure text rather than on acommand's exit status, and several that skipped their work in silence. This hardens them, and
adds a coverage check for the nsedge corpus fixtures. Each case below was verified by injecting
the fault and observing the gate fail.
Verdicts now key on exit status
All tests passed. Catch2 printsthat summary before at-exit failures, so a segfault in a static destructor, an
atexitabort or a LeakSanitizer report passed. Both compilers, the ASan/UBSan run and the coverage
run now check the status too, and print what the run actually said.
xargs' status, andtidy_onereports by writing a log —so a fan-out that linted nothing was indistinguishable from a clean one (
JOBS=abc→ALL GREEN, 0 of 48 TUs linted).llvm-profdata mergeleft the previous run's profile on disk and the coverage floorwas graded against it.
Stale binaries can no longer be graded
build/gcc,build/clang,build/sanandbuild/covall pinRAPIDPROTO_BUILD_TESTS=ON,and
ensure_targets <build-dir> <targets…>verifies each target exists and builds it byname. Previously a cached
-DRAPIDPROTO_BUILD_TESTS=OFF, a renamed target, orEXCLUDE_FROM_ALLleft an earlier binary in place: an injected failing assertion nevercompiled and the stage reported the unchanged assertion count.
regen_*_goldens.shscripts check their target before building, so they cannotrewrite goldens from a stale
rapidprotocor test binary.Skips are now failures
RAPIDPROTO_REGEN_GOLDENinherited from the environment made the test binary rewrite thegoldens it verifies — corrupted fixtures were overwritten to match the code and the run went
green. The gate now ignores the variable and says so.
tests/test_*.cppmust appear in theadd_executable(rapidproto_tests …)source list.A file that is unlisted or commented out is compiled and run by nothing, and the assertion
count does not move to give it away.
RAPIDPROTO_TIDY_SHARD, afuzz target with no seeds staged (or seeds left from a previous run), a zero-match golden
regeneration, a
docsrun that finds no files, and a corpus checked out at the wrong pin allfail instead of passing quietly.
STAGE_KEYSbut in no run list is rejected at startup rather than beingskipped silently.
Structure and reporting
rc_*variables; outcomes are recorded beside eachstage log, so an unrecorded stage is visibly absent rather than counted as a pass.
build/gate-logs.XXXXXXand symlinked tobuild/gate-logs; the summary names failing stages and their durations.RAPIDPROTO_GATE_STAGESselects a subset; unknown or empty values exit 2.deep(~163s, 30% of the gate), and thedifferential is parallelised — byte-identical output at 1, 7 and 20 workers.
tests/check_fixture_coverage.shrequires everytests/corpus/nsedge/*.prototo beregenerated by both golden scripts and to have non-empty stream and arena goldens.