Skip to content

Commit d8ec8f0

Browse files
lexfreilexfrei
authored
fix(ci): run every bats unit file after a failure and cover build-*.sh in the cross-compile check (#4633)
## 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-going` make stops scheduling targets after the first failure. CI passes `-k`, so it was not affected. The target now runs the files in a `--keep-going` sub-make. That sub-make still exits non-zero if any file failed, and it shares the caller's `-j` slots and `--output-sync`. The new `hack/bats-unit-keep-going.bats` drives 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 when `MAKEFILES` names another file. I split the exact-line check on this target in `hack/bats-runner-coverage.bats` in 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.sh` as a compile step, and multus runs upstream `./hack/build-go.sh`. The pattern now allows a suffix after `build` but no `/`, and each case has a fixture test. Among the Dockerfiles the check scans under `packages/`, 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.sh` ends a file at its first failing test, is tracked in #4620. Closes #4500 Things I left out: - The multus stage passes because a `curl ... TARGETARCH` step sits in the same stage. The check does not see that `build-go.sh` reads `TARGETPLATFORM`. The test header already documents this per-stage limit. - The sub-make reads only the root Makefile, so an extra `make -f other.mk` given to the parent no longer reaches the bats targets. Command-line variables and `MAKEFILES` still do. Nothing in the repo calls it that way. ### Screenshots Not a UI change. ### Downstream repositories - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: ### Release note ```release-note fix(ci): `make bats-unit-tests` runs every bats file after a failing one and still fails at the end, and the image cross-compile check now covers build scripts such as `build-go.sh`. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Unit tests now continue running across individual test-file failures while still reporting an overall failure if any file fails. * Build-stage detection now recognizes build scripts with names such as `build-go.sh`. * **Tests** * Added coverage for unit-test results and build-script detection. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents cdc4b88 + 71a9432 commit d8ec8f0

6 files changed

Lines changed: 105 additions & 15 deletions

File tree

‎.github/workflows/pull-requests.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -318,8 +318,8 @@ jobs:
318318
run: |
319319
helm plugin list 2>/dev/null | grep -q unittest \
320320
|| helm plugin install https://github.com/helm-unittest/helm-unittest
321-
# One make process owns all four runner CPUs. Each BATS file is a separate
322-
# prerequisite, so the long tail no longer runs as one nine-minute shell
321+
# The make processes share one four-slot jobserver. Each BATS file is a
322+
# separate target, so the long tail no longer runs as one nine-minute shell
323323
# loop; Helm and Go targets share the same bounded pool. Output stays
324324
# grouped by target instead of four test logs interleaving.
325325
#

‎Makefile‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
.PHONY: manifests assets prepare-env prepare-env-container unit-tests helm-unit-tests bats-unit-tests bats-unit-files-check rd-presets-check migrations-target-check test test-controllers preflight
22

3+
# Before any include, the last file make has read is this one: MAKEFILES and
4+
# -f files given before it are earlier in the list, -f files after it are not
5+
# read yet.
6+
ROOT_MAKEFILE := $(lastword $(MAKEFILE_LIST))
7+
38
include hack/common-envs.mk
49

510
build-deps:
@@ -196,17 +201,21 @@ BATS_UNIT_TARGETS := $(patsubst hack/%.bats,bats-unit-%,$(BATS_UNIT_FILES))
196201
# the live stream -- see the COZYTEST_TRACE comment in hack/cozytest.sh.
197202
COZYTEST_TRACE ?= 0
198203

199-
bats-unit-tests: bats-unit-files-check $(BATS_UNIT_TARGETS)
204+
# A sub-make with --keep-going, so one red file does not stop make from
205+
# scheduling the files after it; the sub-make still exits non-zero if any
206+
# file failed (hack/bats-unit-keep-going.bats covers both). It shares the
207+
# caller's -j slots and --output-sync through MAKEFLAGS.
208+
bats-unit-tests: bats-unit-files-check
209+
@$(MAKE) --file=$(ROOT_MAKEFILE) --no-print-directory --keep-going $(BATS_UNIT_TARGETS)
200210

201211
bats-unit-files-check:
202212
@if [ -z "$(BATS_UNIT_FILES)" ]; then \
203213
echo "ERROR: no hack/*.bats unit test files found"; \
204214
exit 1; \
205215
fi
206216

207-
# Each file is its own prerequisite so `make -jN` can schedule them, and the
208-
# trace switch main added for the serial loop rides along per target rather than
209-
# being lost with it.
217+
# Each file is its own target of the sub-make so `make -jN` can schedule them,
218+
# and the trace switch rides along per target.
210219
.PHONY: $(BATS_UNIT_TARGETS)
211220
$(BATS_UNIT_TARGETS): bats-unit-%: hack/%.bats
212221
@echo "--- running $< ---"

‎hack/bats-runner-coverage.bats‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,11 @@ brc_audit() {
154154
printf '%s\n' "$makefile" | grep -qF 'BATS_UNIT_TARGETS := $(patsubst hack/%.bats,bats-unit-%,$(BATS_UNIT_FILES))' \
155155
|| { echo "FAIL: update the unit target model in hack/bats-runner-coverage.bats."; false; }
156156

157-
printf '%s\n' "$makefile" | grep -qF 'bats-unit-tests: bats-unit-files-check $(BATS_UNIT_TARGETS)' \
158-
|| { echo "FAIL: bats-unit-tests no longer depends on every discovered unit file."; false; }
157+
printf '%s\n' "$makefile" | grep -qF 'bats-unit-tests: bats-unit-files-check' \
158+
|| { echo "FAIL: bats-unit-tests no longer checks that unit files were discovered."; false; }
159+
160+
printf '%s\n' "$makefile" | grep -qF -- '--keep-going $(BATS_UNIT_TARGETS)' \
161+
|| { echo "FAIL: bats-unit-tests no longer runs every discovered unit file past a failing one."; false; }
159162

160163
printf '%s\n' "$makefile" | grep -qF '$(BATS_UNIT_TARGETS): bats-unit-%: hack/%.bats' \
161164
|| { echo "FAIL: each unit target must depend on its discovered bats file."; false; }

‎hack/bats-unit-keep-going.bats‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
#!/usr/bin/env bats
2+
# `make bats-unit-tests` must run every bats file even when an early one fails,
3+
# and still exit non-zero. Each file is its own make target, and without
4+
# --keep-going make stops scheduling targets at the first failure, so the files
5+
# sorting after it never run and the output reads like a complete suite.
6+
#
7+
# The tests drive the real root Makefile against a miniature tree: two bats
8+
# files and a stub hack/cozytest.sh that reports which file it was given.
9+
#
10+
# Requires: make.
11+
12+
REPO_ROOT="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")/.." && pwd)"
13+
14+
# kg_run <stub body line> [makefiles]: builds the fixture, runs the target with
15+
# MAKEFILES set to the second argument, and leaves the output in $fixture/out
16+
# and the exit status in $rc.
17+
kg_run() {
18+
fixture="$(mktemp -d)"
19+
mkdir "$fixture/hack"
20+
: >"$fixture/hack/common-envs.mk"
21+
: >"$fixture/hack/a.bats"
22+
: >"$fixture/hack/b.bats"
23+
printf '%s\n' '#!/bin/sh' 'echo "stub ran $1"' "$1" >"$fixture/hack/cozytest.sh"
24+
chmod +x "$fixture/hack/cozytest.sh"
25+
26+
# Under `make unit-tests -k` this suite inherits MAKEFLAGS carrying the
27+
# parent's --keep-going, which would let the old behaviour pass here.
28+
rc=0
29+
env -u MAKEFLAGS -u MFLAGS -u MAKELEVEL MAKEFILES="${2:-}" \
30+
make --file="$REPO_ROOT/Makefile" --directory="$fixture" bats-unit-tests >"$fixture/out" 2>&1 || rc=$?
31+
cat "$fixture/out"
32+
}
33+
34+
@test "bats-unit-tests runs every file after a failure and still fails" {
35+
# Fail whichever file runs first, whatever order make picks, so the exit
36+
# status cannot come from the last file alone.
37+
kg_run 'mkdir "$(dirname "$0")/.failed" 2>/dev/null && exit 1; exit 0'
38+
ran_a=$(grep -c '^stub ran hack/a.bats$' "$fixture/out" || true)
39+
ran_b=$(grep -c '^stub ran hack/b.bats$' "$fixture/out" || true)
40+
rm -rf "$fixture"
41+
42+
[ "$ran_a" = 1 ]
43+
[ "$ran_b" = 1 ]
44+
[ "$rc" -ne 0 ]
45+
}
46+
47+
@test "bats-unit-tests still finds the root Makefile when MAKEFILES reads another first" {
48+
extra="$(mktemp)"
49+
printf '%s\n' 'unrelated:' ' @true' >"$extra"
50+
kg_run 'exit 0' "$extra"
51+
ran_b=$(grep -c '^stub ran hack/b.bats$' "$fixture/out" || true)
52+
rm -rf "$fixture" "$extra"
53+
[ "$ran_b" = 1 ]
54+
[ "$rc" -eq 0 ]
55+
}
56+
57+
@test "bats-unit-tests exits zero when every file passes" {
58+
kg_run 'exit 0'
59+
rm -rf "$fixture"
60+
[ "$rc" -eq 0 ]
61+
}

‎hack/image-builders-cross-compile.bats‎

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,12 @@
99
# of the other architecture pulls it.
1010
#
1111
# A stage compiles when one of its RUN commands is go build/test/install, make,
12-
# gradle, dpkg-buildpackage, a node package manager or a *build.sh script, at
13-
# command position (after RUN and its --flag options, `&&`, `;` or `||`, past
14-
# any VAR=value prefixes). Such a stage must run on the build platform (see
15-
# stages() below) and use TARGETARCH or TARGETPLATFORM on a RUN or ENV line, unless its output
16-
# carries no architecture at all (ARCH_NEUTRAL below). A bare
12+
# gradle, dpkg-buildpackage, a node package manager or a script whose file name
13+
# contains "build" and ends in .sh, at command position (after RUN and its
14+
# --flag options, `&&`, `;` or `||`, past any VAR=value prefixes). Such a stage
15+
# must run on the build platform (see stages() below) and use TARGETARCH or
16+
# TARGETPLATFORM on a RUN or ENV line, unless its output carries no
17+
# architecture at all (ARCH_NEUTRAL below). A bare
1718
# `ARG TARGETARCH` does not count: it declares the variable, and a `go build`
1819
# that never reads it still builds for the host. The check is per stage, not
1920
# per command: a stage with `RUN echo $TARGETARCH` and a bare `go build` would
@@ -60,7 +61,7 @@ stages() {
6061
if (native && name != "-") nativestage[name] = 1
6162
next
6263
}
63-
/^RUN[[:space:]]/ && /(^RUN([[:space:]]+--[^[:space:]]+)*|&&|;|[|][|])[[:space:]]+([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+)*(go[[:space:]]+(build|test|install)|make|gradle|[.]\/gradlew|dpkg-buildpackage|pnpm|npm|yarn|[^[:space:]]*build[.]sh)([[:space:]]|$)/ { compiles = 1 }
64+
/^RUN[[:space:]]/ && /(^RUN([[:space:]]+--[^[:space:]]+)*|&&|;|[|][|])[[:space:]]+([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+)*(go[[:space:]]+(build|test|install)|make|gradle|[.]\/gradlew|dpkg-buildpackage|pnpm|npm|yarn|[^[:space:]]*build[^[:space:]\/]*[.]sh)([[:space:]]|$)/ { compiles = 1 }
6465
/^(RUN|ENV)[[:space:]]/ && /TARGETARCH|TARGETPLATFORM/ { targets = 1 }
6566
END { flush() }
6667
'
@@ -106,6 +107,22 @@ stages() {
106107
[ "$out" = "1 0 0 builder FROM golang AS builder" ]
107108
}
108109

110+
@test "a build script with a suffix after build still counts as a compile" {
111+
fixture="$(mktemp)"
112+
printf '%s\n' 'FROM --platform=$BUILDPLATFORM golang AS build' 'RUN ./hack/build-go.sh' >"$fixture"
113+
out="$(stages "$fixture")"
114+
rm -f "$fixture"
115+
[ "$out" = '1 0 1 build FROM --platform=$BUILDPLATFORM golang AS build' ]
116+
}
117+
118+
@test "a script under a build/ directory is not a build script" {
119+
fixture="$(mktemp)"
120+
printf '%s\n' 'FROM golang AS x' 'RUN ./build/install.sh' >"$fixture"
121+
out="$(stages "$fixture")"
122+
rm -f "$fixture"
123+
[ "$out" = "0 0 0 x FROM golang AS x" ]
124+
}
125+
109126
@test "a stage built FROM an earlier BUILDPLATFORM stage runs on the build platform too" {
110127
fixture="$(mktemp)"
111128
printf '%s\n' 'FROM --platform=$BUILDPLATFORM node AS source' 'RUN true' 'FROM source AS app' 'RUN npm ci' 'FROM node AS emulated' 'RUN npm ci' 'FROM emulated AS child' 'RUN go build .' >"$fixture"

‎hack/unit-test-parallelism.bats‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
#!/usr/bin/env bats
22
# Contract for the bounded parallel unit-test fan-out used by PR CI.
33

4-
@test "every non-E2E BATS file is an independent Make prerequisite" {
4+
@test "every non-E2E BATS file is an independent Make target" {
55
# --output-sync arrived in GNU Make 4.0, and macOS still ships 3.81, where
66
# this invocation dies on an unknown option rather than telling the reader
77
# why. CI is on 4.x, so the contract is still enforced where it gates a

0 commit comments

Comments
 (0)