test(e2e): fail when a bats file is run by no runner by myasnikovdaniil · Pull Request #3778 · cozystack/cozystack

myasnikovdaniil · 2026-08-12T14:40:55Z

Two bats files in hack/e2e-apps/ never ran once. #2826 deleted the test-apps-%: rule that ran hack/e2e-apps/$*.bats together with every file in that directory, and #3176 landed three days later and put two new suites into it. Git gives no conflict when one branch empties directory and another adds files to it, so only signal was directory name that still looked alive.

They could not pass anyway. hack/cozytest.sh has no skip(), so the skip guarding keycloak assertions is command-not-found and exit 127, and kubectl api-resources --api-group=v1.edp.epam.com exits 0 whether that group is served or not. cac07db hit both of these when it ported the kubernetes-oidc twins out of the same race.

Render side is already covered stricter by packages/system/monitoring/tests/oidc_test.yaml (32 cases). It asserts things a live test cannot, spec.public: false for example, because EDP CRD strips that field off the applied object and the deleted bats said so in its own comment.

So what live coverage was carrying and unit coverage is not:

1. api admits spec.oidc - there is no schema validation and no pruning in pkg/registry, rest.go copies app.Spec into HR values as is, so every existing suite already exercises that path and nothing about it is monitoring specific.
2. wrapper passes its values to inner chart - was not covered, this PR adds unittest for it. Checked by mutation, replacing the passthrough with `omit .Values "oidc"` fails exactly that case and leaves the rest of the file green.
3. grafana-operator turns spec.deployment into a Deployment - that is upstream contract, our half of it is pinned in oidc_test.yaml.
4. live swap of oidc mode from inline to secretRef - not covered by anything, and I am not claiming otherwise. Both end states are pinned render side, transition is not. Will open issue.
5. EDP operator accepts rendered KeycloakClient - kubernetes-oidc-system chainsaw suite does that round trip already, same CRDs and same operator.

Second half is a guard so this class does not come back. hack/bats-runner-coverage.bats fails when a bats file under hack/ is reachable by no runner, when the e2e Makefile names a file that is gone, and when a suite is named through make variable the way the deleted rule was. There are exactly two runners and that is a closed list, a new one registers in the guard or the guard reports its files.

It parses the Makefiles instead of asking make. hack/common-envs.mk is included at Makefile:3 and runs git remote add upstream and git fetch upstream --tags inside $(shell ...) at parse time, so make -p or make -n would have a unit test mutate git remotes and go to network to answer a question about filenames. Cost of grepping is that the model can drift from what it models, so a second test pins the modelled lines verbatim, that is the one direction which would otherwise widen the covered set silently.

No allowlist in it. Off switch is the .disabled suffix and the scan stops seeing a parked file by itself.

Makefile comment claimed a new *.bats in hack/ is picked up automatically. That holds only on top level and it is the claim this bug grew in, so it is corrected here too.

Unrelated thing I noticed while reading monitoring, not touched in this PR: around 15 packages hardcode cozystack-<source>-default-<component> in chartRef while artifact name is built as <source>-<variant>-<component>. Not a bug today because the generator emits an artifact per variant and the oidc variant of monitoring-application differs from default only in dependsOn, so content is identical. It goes wrong quietly the day some variant differs in component values.

Summary by CodeRabbit

New Features
- Added Helm coverage verifying tenant-provided OIDC mode and user settings are propagated to child releases.
- Added automated checks to identify BATS suites that are missing from or unreachable through supported test runners.
Documentation
- Clarified test discovery, suite coverage, and e2e capture behavior, including nested suites and naming conventions.
Tests
- Removed two obsolete OIDC end-to-end test suites covering legacy monitoring scenarios.

coderabbitai · 2026-08-12T14:41:25Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fa1e56f9-e8e8-49c7-81a3-a5927feb0e29

📥 Commits

Reviewing files that changed from the base of the PR and between 73f35f3 and 3fbbcd1.

📒 Files selected for processing (1)

Makefile

🚧 Files skipped from review as they are similar to previous changes (1)

Makefile

📝 Walkthrough

Walkthrough

The PR adds a BATS runner coverage audit, updates discovery and capture documentation, removes two monitoring OIDC suites, and adds a Helm test for OIDC value propagation.

Changes

BATS runner coverage

Layer / File(s)	Summary
Runner coverage audit `hack/bats-runner-coverage.bats`	Adds Makefile runner parsing, BATS discovery modeling, orphan and stale suite detection, unresolved variable checks, and fixture-based tests.
Discovery and capture documentation `Makefile`, `packages/core/testing/Makefile`, `hack/bats-no-exit-trap.bats`, `hack/cozytest-capture-gate.bats`, `hack/cozytest.sh`	Documents nested BATS discovery, `.disabled` exclusions, e2e-* capture eligibility, and separate runner coverage validation.

Monitoring OIDC Helm validation

Layer / File(s)	Summary
OIDC values propagation test `packages/extra/monitoring/tests/helmrelease_test.yaml`	Verifies that tenant `oidc.mode` and `oidc.users` values reach the child HelmRelease.
Removed OIDC suites `hack/e2e-apps/monitoring-oidc-customconfig.bats`, `hack/e2e-apps/monitoring-oidc-system.bats`	Removes two monitoring OIDC end-to-end test suites and their helpers.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Merge Risk: ⚪ Minimal · up to 3fbbc

The PR removes two nonfunctional end-to-end suites, adds focused unit coverage, and introduces a guard against unrun test files. No actionable merge-blocking correctness or production risk remains beyond normal checks.

Sequence Diagram(s)

sequenceDiagram
  participant Makefile
  participant CoverageAudit
  participant BatsSuites
  Makefile->>CoverageAudit: provide runner definitions
  CoverageAudit->>BatsSuites: enumerate .bats files
  CoverageAudit->>Makefile: compare suite reachability
  CoverageAudit-->>Makefile: report missing or stale runners

Possibly related PRs

cozystack/cozystack#3520: Deletes the same monitoring OIDC BATS suites.
cozystack/cozystack#3670: Covers BATS suite discovery and unreferenced-suite validation.

Suggested labels: area/ci

Suggested reviewers: ivanhunters, kvaps

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: adding a guard that detects Bats files not executed by any runner.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/bats-runner-coverage

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@hack/bats-runner-coverage.bats`:
- Around line 109-126: Update brc_named_suites to extract the immediate argument
after hack/cozytest.sh without requiring a .bats suffix, while retaining literal
.bats paths and arguments containing Make variables such as $(E2E_SUITE). Add or
update a fixture covering hack/cozytest.sh $(E2E_SUITE) so
brc_unresolvable_suites reports it.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 07f8efef-2123-44d4-8ba5-949c2e24f1ce

📥 Commits

Reviewing files that changed from the base of the PR and between e944619 and 73f35f3.

📒 Files selected for processing (9)

Makefile
hack/bats-no-exit-trap.bats
hack/bats-runner-coverage.bats
hack/cozytest-capture-gate.bats
hack/cozytest.sh
hack/e2e-apps/monitoring-oidc-customconfig.bats
hack/e2e-apps/monitoring-oidc-system.bats
packages/core/testing/Makefile
packages/extra/monitoring/tests/helmrelease_test.yaml

💤 Files with no reviewable changes (2)

hack/e2e-apps/monitoring-oidc-system.bats
hack/e2e-apps/monitoring-oidc-customconfig.bats

coderabbitai · 2026-08-12T14:44:28Z

+brc_named_suites() {
+  brc_code_lines < "$1/$E2E_MAKEFILE_REL" \
+    | grep "^$TAB" \
+    | grep -oE "hack/cozytest\.sh[[:space:]]+[^[:space:]]+\.bats" \
+    | sed "s|.*cozytest\.sh[[:space:]]*||" \
+    | tr -d "\"'" \
+    | sort -u
+}
+
+# Named suites whose path is not a literal -- the shape of the deleted
+# `hack/e2e-apps/$*.bats` pattern rule.
+#
+# Reported rather than expanded. A pattern rule nobody invokes is exactly as dead
+# as no rule at all, so treating one as coverage would reintroduce this bug in a
+# new shape. If a pattern runner is ever wanted back, teach this guard the pattern
+# form on purpose.
+brc_unresolvable_suites() {
+  brc_named_suites "$1" | grep '\$' || true


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Detect Make variables that do not contain a .bats suffix.

Line 112 only extracts an argument that ends in .bats. A runner such as hack/cozytest.sh $(E2E_SUITE) is not extracted. The audit then does not report that unresolved runner.

Extract the immediate argument first. Keep literal .bats paths and arguments containing $. Add a fixture for $(E2E_SUITE).

Proposed fix

- | grep -oE "hack/cozytest\.sh[[:space:]]+[^[:space:]]+\.bats" \ + | grep -oE "hack/cozytest\.sh[[:space:]]+[^[:space:]]+" \ | sed "s|.*cozytest\.sh[[:space:]]*||" \ | tr -d "\"'" \ + | grep -E '\.bats$|\$' \ | sort -u

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

brc_named_suites() {

brc_code_lines < "$1/$E2E_MAKEFILE_REL" \

| grep "^$TAB" \

| grep -oE "hack/cozytest\.sh[[:space:]]+[^[:space:]]+\.bats" \

| sed "s|.*cozytest\.sh[[:space:]]*||" \

| tr -d "\"'" \

| sort -u

}

# Named suites whose path is not a literal -- the shape of the deleted

# `hack/e2e-apps/$*.bats` pattern rule.

#

# Reported rather than expanded. A pattern rule nobody invokes is exactly as dead

# as no rule at all, so treating one as coverage would reintroduce this bug in a

# new shape. If a pattern runner is ever wanted back, teach this guard the pattern

# form on purpose.

brc_unresolvable_suites() {

brc_named_suites "$1" | grep '\$' || true

brc_named_suites() {

brc_code_lines < "$1/$E2E_MAKEFILE_REL" \

| grep "^$TAB" \

| grep -oE "hack/cozytest\.sh[[:space:]]+[^[:space:]]+" \

| sed "s|.*cozytest\.sh[[:space:]]*||" \

| tr -d "\"'" \

| grep -E '\.bats$|\$' \

| sort -u

}

# Named suites whose path is not a literal -- the shape of the deleted

# `hack/e2e-apps/$*.bats` pattern rule.

#

# Reported rather than expanded. A pattern rule nobody invokes is exactly as dead

# as no rule at all, so treating one as coverage would reintroduce this bug in a

# new shape. If a pattern runner is ever wanted back, teach this guard the pattern

# form on purpose.

brc_unresolvable_suites() {

brc_named_suites "$1" | grep '\$' || true

}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/bats-runner-coverage.bats` around lines 109 - 126, Update brc_named_suites to extract the immediate argument after hack/cozytest.sh without requiring a .bats suffix, while retaining literal .bats paths and arguments containing Make variables such as $(E2E_SUITE). Add or update a fixture covering hack/cozytest.sh $(E2E_SUITE) so brc_unresolvable_suites reports it.

hack/e2e-apps/monitoring-oidc-system.bats and its customconfig twin have never executed. #2826 deleted the `test-apps-%:` rule that ran hack/e2e-apps/$*.bats along with every other file in that directory; #3176 landed three days later and added these two into it. Git raises no conflict when one branch empties a directory and another adds files to it, so the only signal was a directory name that still looked live. They could not pass as written either. hack/cozytest.sh defines no skip(), so the `skip` guarding the Keycloak assertions is a command-not-found and exit 127, and `kubectl api-resources --api-group=v1.edp.epam.com` exits 0 whether or not the group is served, so the guard was dead code in both directions. cac07db found and dropped both when it ported the kubernetes-oidc twins out of the same race. The render-side coverage is superseded, strictly, by packages/system/monitoring/tests/oidc_test.yaml -- 32 helm-unittest cases over the same Phase-1 selector, several of them asserting more than a live test can. `spec.public: false` is the example: the EDP CRD strips the field off the applied object, which the deleted bats said so itself. Four comments described hack/e2e-apps/ as a live directory; they now describe the shape rather than the instance, and two of them say what an e2e- prefix does not mean, since arming the cluster captures was never a claim that a runner exists. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

The wrapper hands the whole of .Values to the inner chart verbatim, so that one line is the only path by which a tenant-set spec.oidc reaches the chart which renders it. Nothing pinned that hop: the inner chart's own suite sets its values directly, so a wrapper that dropped or renamed a key would still render a correct inner chart -- from values no tenant can actually set. Verified by mutation: replacing the passthrough with `omit .Values "oidc"` fails exactly this case's three assertions and leaves the other three cases in the file green. This is the one item from the deleted monitoring OIDC bats that live coverage was genuinely carrying and unit coverage was not, and it costs no cluster. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

Two suites sat in hack/e2e-apps/ for three weeks without executing once, and nothing in the tree could notice: the property "something runs this file" was described in prose in three comments and asserted nowhere. This adds the assertion. There are exactly two runners and that is a closed list by design -- the root Makefile's one-level, non-e2e glob, and the literal suite names in packages/core/testing/Makefile. A file matching neither is reported, as is the inverse: a name in the e2e Makefile with no file on disk, which today surfaces forty minutes into the e2e job, or wedges the bootstrap outright. A suite named through a make variable, the shape of the rule #2826 deleted, is reported as unresolvable rather than expanded, because a pattern rule nobody invokes is as dead as no rule. The Makefiles are parsed rather than asked. hack/common-envs.mk is included at Makefile:3 and runs `git remote add upstream` and `git fetch upstream --tags` in $(shell ...) at parse time, so `make -p` or `make -n` would have a unit test mutate git remotes and reach the network to answer a question about filenames. The cost is that the model can drift from what it models, so a second test pins the modelled lines verbatim -- that is the one direction which would otherwise widen the covered set in silence. No allowlist, deliberately: the off-switch is the .disabled suffix, which the scan stops seeing on its own. An in-test list of exempt paths would be the same artefact class as the runner that got deleted while its documentation stayed. That documentation is corrected here too. Makefile's comment claimed a new .bats file in hack/ is picked up automatically, which holds only at the top level and is the false claim this bug grew in. Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

myasnikovdaniil requested review from IvanHunters, Andrei Kvapil (kvaps), Aleksei Sviridkin (lexfrei) and Timofei Larkin (lllamnyp) as code owners August 12, 2026 14:40

coderabbitai Bot reviewed Aug 12, 2026

View reviewed changes

myasnikovdaniil mentioned this pull request Aug 12, 2026

Monitoring OIDC mode swap from inline to secretRef has no test coverage #3785

Open

coderabbitai Bot mentioned this pull request Aug 12, 2026

test(tests): make negated BATS assertions able to fail #3790

Open

11 tasks

github-actions Bot added area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/XL This PR changes 500-999 lines, ignoring generated files labels Aug 12, 2026

coderabbitai Bot mentioned this pull request Aug 15, 2026

fix(tests): let the mariadb waits cover a first boot #3769

Open

1 task

myasnikovdaniil added 3 commits August 15, 2026 17:56

myasnikovdaniil force-pushed the fix/bats-runner-coverage branch from 73f35f3 to 3fbbcd1 Compare August 15, 2026 16:24

Navigation Menu

Uh oh!

test(e2e): fail when a bats file is run by no runner - #3778

test(e2e): fail when a bats file is run by no runner#3778
myasnikovdaniil wants to merge 3 commits into
mainfrom
fix/bats-runner-coverage

myasnikovdaniil commented Aug 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

myasnikovdaniil commented Aug 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Aug 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

myasnikovdaniil commented Aug 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 12, 2026 •

edited

Loading