Skip to content

Commit e126158

Browse files
lexfreilexfrei
authored
fix(ci): keep the controller Go tests running when a unit test fails first (#4233)
<!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does The `checks` job in `.github/workflows/pull-requests.yaml` runs `make unit-tests test-controllers -j4 --output-sync=target` as a single step. `unit-tests` and `test-controllers` are independent goals on that command line, and without `-k` GNU Make stops handing out new work the instant any earlier-processed goal's prerequisite fails — a goal that had not yet been scheduled never starts. So whenever a BATS file inside `unit-tests` fails before `test-controllers` gets a job slot, the entire Go controller suite (`go test ./internal/...`) silently never runs, and the PR page shows only the unit-test failure with no signal at all about the controllers. This adds `-k` so already-schedulable goals keep running after an earlier one fails; the job still exits non-zero and still fails whenever anything failed, `-k` only stops make from abandoning independent work. This addresses the second half of #3776. The first half of that report — whether the job's comment overclaims that it gates the merge, since `Unit & controller tests` is not currently in `main`'s required status checks — needs a maintainer's call between two mutually exclusive fixes (weaken the comment, or add the job to branch protection), and is deliberately left untouched here; the issue itself frames the two as independent and separately useful. Note: `hack/unit-test-parallelism.bats`, which pins the literal invocation string in a test, was updated to match. ### Screenshots N/A — CI workflow and test-script change only, no UI. ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [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: I walked the trigger map: this PR only touches CI scheduling flags inside this repo's own `.github/workflows/pull-requests.yaml` and a `hack/*.bats` meta-test that asserts against that same file. Nothing in the trigger map names either file, and the change has no effect on any package, API, chart, or contract another repository consumes. ### Validation Reproduced the exact scheduling gap in an isolated Makefile mirroring this repo's real dependency shape (a `unit-tests`-like goal depending on several independent prerequisites, one of which fails immediately, plus a sibling goal with no dependency on it), under the same GNU Make 4.4.1 `-j4 --output-sync=target` invocation used here. Without `-k` the sibling goal never ran, across 5 repeated trials; with `-k` added it ran to completion every time across 3 repeated trials, and the overall exit code stayed non-zero either way. `bats hack/unit-test-parallelism.bats` passes locally against the real files. I also ran `make unit-tests test-controllers -j4 -k --output-sync=target` against this repo directly: local disk space (a limit of my sandbox, unrelated to this change) made `go test ./internal/...` fail to compile with "no space left on device", so I could not confirm a clean end-to-end pass of `test-controllers` itself in this environment — but the run still exercised the fix: after that build failure, unrelated `hack/*.bats` prerequisites kept being scheduled and ran to completion instead of the whole invocation aborting, which is exactly the behavior `-k` adds over the prior command line. I could not run this repo's actual CI runners to confirm disk is not similarly constrained there; nothing in the workflow or runner spec suggests it would be. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(ci): the `Unit & controller tests` PR check no longer silently skips the Go controller test suite when a Helm-unittest or BATS unit test fails first; both suites now always run and report. ``` --- Part of #3776 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated automated test execution so unit and controller test suites continue running when one test target fails, while the job still reports failures. * Updated workflow validation to reflect the revised test command. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents 53829ea + 65b02bf commit e126158

2 files changed

Lines changed: 9 additions & 2 deletions

File tree

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,15 @@ jobs:
322322
# prerequisite, 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.
325+
#
326+
# -k is required, not cosmetic: unit-tests and test-controllers are two
327+
# independent goals on the same command line, and without -k make stops
328+
# handing out new work the moment any unit-tests prerequisite fails —
329+
# test-controllers then never starts and reports nothing, so a red BATS
330+
# file hides the entire Go controller suite. -k lets already-schedulable
331+
# goals keep running; the job still fails overall on any red target.
325332
- name: Run unit and controller tests
326-
run: make unit-tests test-controllers -j4 --output-sync=target
333+
run: make unit-tests test-controllers -j4 -k --output-sync=target
327334

328335
# One image (package) per job, fanned out across the ephemeral pool. Each job
329336
# gets its own buildx (docker-container driver) so the shared-buildkit

‎hack/unit-test-parallelism.bats‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
}
3030

3131
@test "PR workflow shares four slots across unit and controller targets" {
32-
grep -qF 'run: make unit-tests test-controllers -j4 --output-sync=target' \
32+
grep -qF 'run: make unit-tests test-controllers -j4 -k --output-sync=target' \
3333
.github/workflows/pull-requests.yaml || {
3434
echo "PR checks do not use the bounded four-slot make invocation" >&2
3535
exit 1

0 commit comments

Comments
 (0)