Repository navigation
fix(ci): run every bats unit file after a failure and cover build-*.sh in the cross-compile check - #4633
Conversation
…le check The compile pattern only matched a script named exactly *build.sh, so the multus-cni builder, which compiles through upstream ./hack/build-go.sh, was never classified as compiling and the check skipped the stage while appearing to cover it. Allow a suffix after "build" in the script name, but not a directory separator, so a path through a build/ directory is not mistaken for a build script. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
Each bats file is its own make target, so a plain `make bats-unit-tests` stopped scheduling files at the first red one and never ran the rest, while the output looked like a finished suite. CI passes -k and was not affected; a local run was. The target now runs the files in a --keep-going sub-make, which still exits non-zero when any file fails and shares the caller's job slots and output sync. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/ci
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe Makefile runs per-file BATS unit targets through a keep-going sub-make. New tests check execution and exit behavior. The cross-compile detector now recognizes build scripts by filename, with tests for matching and non-matching examples. ChangesBATS unit-test execution
Cross-compile stage detection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The changes address unit-test continuation and build-script detection, with focused regression coverage. No concrete merge-blocking issue is established; merge after normal checks pass. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Summary
I ran both halves instead of eyeballing them. make bats-unit-tests now runs its files under a sub-make with --keep-going, and the aggregated exit stays non-zero when any file fails, including when the sub-make inherits -k from the parent (which is how CI invokes it: make unit-tests test-controllers -j4 -k --output-sync=target). The widened cross-compile regex is a strict superset of the old one, and the build/-directory exclusion holds.
Checked and correct
- Exit propagation: with a two-file fixture under the real Makefile, forcing the first-scheduled file to fail runs both files and exits 2; an all-pass run exits 0. This holds with and without the parent
-k, so keeping going does not mask a red file. - Coverage fires: the new
build-go.shtest passes and reverting the regex tobuild[.]shreddens it.build-go.shappears only inpackages/system/multus/images/multus-cni/Dockerfile, in abuildstage that already carriesTARGETPLATFORM/TARGETARCH, so the wider match adds no newly-failing stage. build/-directory exclusion:./build/install.shis correctly not treated as a build script, and allowing a slash in the bracket reddens that test.ROOT_MAKEFILE := $(lastword $(MAKEFILE_LIST))resolves to the root Makefile even whenMAKEFILESnames another file first, covered by the new test and confirmed on a run.- No negated-assertion masking: the touched bats files assert in positive
[ ... ]form; the|| trueuses sit in count subshells, not in swallowed assertions. - The renamed
unit-test-parallelism.batstest (prerequisite to target) keeps its body and still enforces per-file scheduling and the no-serial-loop guard.
Left unverified
- Local make is 3.81, so
unit-test-parallelism.batstest 1 self-skips and--output-syncgrouping is not exercised here. The recursivemake -npath was confirmed on 3.81, and the make 4.x path is covered by CI's green unit job on the reviewed head. - awk portability of the new
[^[:space:]\/]bracket was not reproduced across awk variants locally (only BSD awk present); it is settled by the same CI job running the cross-compile bats green under the runner's awk.
Tested against: the branch merges cleanly into current main (per bootstrap), and the unit and controller CI job is green on the reviewed head.
2 participants
What this PR does
Two fixes to the tooling of the unit lane.
A local run of the bats unit tests stopped at the first red file and never ran the files after it, while the output looked like a finished suite. Each bats file is its own make target, and without
--keep-goingmake stops scheduling targets after the first failure. CI passes-k, so it was not affected. The target now runs the files in a--keep-goingsub-make. That sub-make still exits non-zero if any file failed, and it shares the caller's-jslots and--output-sync. The newhack/bats-unit-keep-going.batsdrives the real Makefile against two fixture files. It checks that both run and the target fails when the first one fails, that the target exits 0 when both pass, and that the sub-make still finds the root Makefile whenMAKEFILESnames another file. I split the exact-line check on this target inhack/bats-runner-coverage.batsin two to match the new recipe. A workflow comment and a test name that still called each file a prerequisite now call it a target.The cross-compile check never looked at the multus-cni builder. It treated only a script named exactly
*build.shas a compile step, and multus runs upstream./hack/build-go.sh. The pattern now allows a suffix afterbuildbut no/, and each case has a fixture test. Among the Dockerfiles the check scans underpackages/, this matches exactly one new line, the multus stage, and that stage passes.Closes #3692 for the make-level truncation. The in-file half, where
hack/cozytest.shends a file at its first failing test, is tracked in #4620.Closes #4500
Things I left out:
curl ... TARGETARCHstep sits in the same stage. The check does not see thatbuild-go.shreadsTARGETPLATFORM. The test header already documents this per-stage limit.make -f other.mkgiven to the parent no longer reaches the bats targets. Command-line variables andMAKEFILESstill do. Nothing in the repo calls it that way.Screenshots
Not a UI change.
Downstream repositories
Release note
Summary by CodeRabbit
build-go.sh.