test: make negative bats assertions fail under cozytest - #3132
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a subtle issue in the test suite where negative assertions were failing to trigger test failures due to how Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAcross six Bats tests, negated pipeline assertions were replaced with explicit conditional failures so the checks fail reliably under ChangesBats negation assertion fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
There was a problem hiding this comment.
Code Review
This pull request updates several BATS test files to replace vacuous negative assertions (using !) with explicit if checks and false exits, ensuring that regressions are properly caught under set -e execution. The reviewer suggests using printf instead of echo when printing variables containing arbitrary command or log output to prevent potential parsing and portability issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if echo "$out" | grep -q '"packages/core/talos"'; then echo "FAIL: packages/core/talos must be excluded from the parallel matrix"; false; fi | ||
| if echo "$out" | grep -q '"packages/core/installer"'; then echo "FAIL: packages/core/installer must be excluded from the parallel matrix"; false; fi |
There was a problem hiding this comment.
Using echo to print variables containing arbitrary command output (like $out) can lead to portability and parsing issues if the output contains backslashes or starts with hyphens (interpreting them as options). It is safer and more robust to use printf '%s\n' instead, which is also consistent with the pattern used in hack/capture-dataplane.bats.
if printf '%s\n' "$out" | grep -q '"packages/core/talos"'; then echo "FAIL: packages/core/talos must be excluded from the parallel matrix"; false; fi
if printf '%s\n' "$out" | grep -q '"packages/core/installer"'; then echo "FAIL: packages/core/installer must be excluded from the parallel matrix"; false; fi
| # would not fail the test; assert the absence via `if ... grep; then ...; false`. | ||
| echo "${pub_logs}" | grep -F "${pub_host}" >/dev/null | ||
| ! echo "${pub_logs}" | grep -F "${int_host}" >/dev/null | ||
| if echo "${pub_logs}" | grep -F "${int_host}" >/dev/null; then echo "FAIL: public-prefix probe must NOT see the internal hostname ${int_host}"; false; fi |
There was a problem hiding this comment.
Using echo to print variables containing arbitrary log output (like ${pub_logs}) can lead to portability and parsing issues if the logs contain backslashes or leading hyphens. It is safer and more robust to use printf '%s\n' instead.
if printf '%s\n' "${pub_logs}" | grep -F "${int_host}" >/dev/null; then echo "FAIL: public-prefix probe must NOT see the internal hostname ${int_host}"; false; fi
| # And the inverse for the internal-prefix probe. | ||
| echo "${int_logs}" | grep -F "${int_host}" >/dev/null | ||
| ! echo "${int_logs}" | grep -F "${pub_host}" >/dev/null | ||
| if echo "${int_logs}" | grep -F "${pub_host}" >/dev/null; then echo "FAIL: internal-prefix probe must NOT see the public hostname ${pub_host}"; false; fi |
There was a problem hiding this comment.
Using echo to print variables containing arbitrary log output (like ${int_logs}) can lead to portability and parsing issues if the logs contain backslashes or leading hyphens. It is safer and more robust to use printf '%s\n' instead.
if printf '%s\n' "${int_logs}" | grep -F "${pub_host}" >/dev/null; then echo "FAIL: internal-prefix probe must NOT see the public hostname ${pub_host}"; false; fi
| # into a "warn" variant, the server could still accept the object. A bare | ||
| # `! echo | grep` is vacuous under cozytest's `set -e` (suppressed for a `!` | ||
| # pipeline), so the regression would slip through; assert via `if ...; false`. | ||
| if echo "$output" | grep -qi "created"; then echo "FAIL: kubectl reported the tenant as created — validation must reject it, not warn"; false; fi |
There was a problem hiding this comment.
Using echo to print variables containing arbitrary command output (like $output) can lead to portability and parsing issues if the output contains backslashes or leading hyphens. It is safer and more robust to use printf '%s\n' instead.
if printf '%s\n' "$output" | grep -qi "created"; then echo "FAIL: kubectl reported the tenant as created — validation must reject it, not warn"; false; fi
myasnikovdaniil
left a comment
There was a problem hiding this comment.
The fix is correct and complete within its stated scope. Under cozytest.sh, each @test runs with set -e, but POSIX exempts ! pipeline from errexit — making all ten bare ! cmd assertions silently vacuous. Replacing them with if cmd; then echo "FAIL: …"; false; fi is the right inverse of the || { …; false; } pattern already used in the suite for the must-succeed direction. All affected files are covered; the etcd.bats carve-out is clearly justified and noted in the PR body.
Two optional suggestions inline: multi-line if form for readability, and adding a kubectl describe to the failure branch per the project convention of dumping scoped diagnostics on failure. Neither is a blocker.
| # is vacuous under cozytest's `set -e` (suppressed for a `!` pipeline), so if | ||
| # the child wrongly got its own HelmRelease the test would still pass; assert | ||
| # via `if kubectl get; then ...; false`. | ||
| if kubectl -n tenant-test-gwparent-gwchild get helmrelease gateway 2>/dev/null; then echo "FAIL: child tenant must NOT have its own gateway HelmRelease (gateway inheritance broken)"; false; fi |
There was a problem hiding this comment.
Nit: the single-line form is 150+ characters and hard to extend. The e2e conventions recommend dumping scoped diagnostics on failure — if the child unexpectedly has its own HelmRelease, a describe in the failure branch shows why without needing to reproduce:
if kubectl -n tenant-test-gwparent-gwchild get helmrelease gateway 2>/dev/null; then
echo "FAIL: child tenant must NOT have its own gateway HelmRelease (gateway inheritance broken)"
kubectl -n tenant-test-gwparent-gwchild describe helmrelease gateway || true
false
fi| # A bare `! kubectl get` is vacuous under cozytest's `set -e` (errexit is | ||
| # suppressed for a `!` pipeline), so a delete that silently failed would not | ||
| # fail the test; assert the absence via `if kubectl get; then ...; false`. | ||
| if kubectl get configmap cozystack-version -n cozy-system 2>/dev/null; then echo "FAIL: cozystack-version configmap must be gone after delete with the no-delete label removed"; false; fi |
There was a problem hiding this comment.
Same style nit: multi-line form + a describe on failure would capture why the ConfigMap survived the delete (e.g. a re-protection race from the admission webhook or a finalizer):
if kubectl get configmap cozystack-version -n cozy-system 2>/dev/null; then
echo "FAIL: cozystack-version configmap must be gone after delete with the no-delete label removed"
kubectl describe configmap cozystack-version -n cozy-system || true
false
fif17a31d to
0123b20
Compare
These bats files are all run by hack/cozytest.sh, which executes each @test under `set -e`. POSIX errexit is suppressed for a pipeline that begins with `!`, so a bare `! cmd` must-not-happen assertion never fails the test: when the forbidden condition actually occurs (cmd succeeds) the negated pipeline returns non-zero but errexit ignores it and the test passes anyway. These assertions were silently vacuous. Replace each with `if cmd; then echo "<reason>"; false; fi`, the idiom already used and documented elsewhere in these files — the if-condition is exempt from errexit and the `false` in the taken branch fails the test loudly. The forbidden condition now fails the test as intended: - bucket: read-only user must not be able to upload (access control) - gateway: child tenant must not have its own gateway HelmRelease - kuberture: external-dns public/internal hostname isolation - e2e-install: tenant-name validation must reject (not warn); the version ConfigMap must be gone after delete - build-matrix: talos/installer excluded from the parallel matrix - capture-dataplane: kube-dns and pending rows are filtered out Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Aleksei Sviridkin <f@lex.la>
0123b20 to
12fe325
Compare
What this PR does
Every
*.batsfile in the repo runs throughhack/cozytest.sh, not real bats — the e2e files viapackages/core/testing/Makefileand thehack/*.batsunit tests viamake bats-unit-tests. cozytest runs each@testunderset -e, and POSIX errexit is suppressed for a pipeline beginning with!. So a bare! cmdmust-not-happen assertion never fails the test: when the forbidden condition actually occurs (the command succeeds), the negated pipeline returns non-zero but errexit ignores it and the test passes anyway. Ten such assertions across six files were silently vacuous.This converts each to
if cmd; then echo "<reason>"; false; fi. Theifcondition is exempt from errexit (so the probe itself never aborts the test), and thefalsein the taken branch is a plain command underset -e, so it aborts and fails the test with a legible message when the forbidden condition holds. This is the must-not-happen inverse of thecmd || { echo …; false; }pattern already used in the suite (e.g. harbor.bats, per docs/agents/e2e-testing.md). Original redirections and quoting are preserved, so happy-path behavior is unchanged — only the failure path now actually fails.Converted assertions:
bucket: a read-only user must not be able to upload (access-control regression guard)gateway: a child tenant must not have its own gateway HelmRelease (inheritance)kuberture: external-dns public/internal hostname isolatione2e-install-cozystack: tenant-name validation must reject rather than warn; the version ConfigMap must be gone after deletebuild-matrix(unit test): talos/installer must be excluded from the parallel matrixcapture-dataplane(unit test): kube-dns and pending rows must be filtered outA separate in-flight etcd test change owns two more such assertions in
etcd.bats; they are left out of this PR to avoid a conflicting edit.Release note
Summary by CodeRabbit