Skip to content

test(e2e): spell the find negation as -not in select-e2e_test - #4748

Closed
Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
mainfrom
fix/select-e2e-find-not
Closed

Aleksei Sviridkin (lexfrei) wants to merge 1 commit into
mainfrom
fix/select-e2e-find-not

Conversation

@lexfrei

@lexfrei Aleksei Sviridkin (lexfrei) commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

Turns the unit lane on main green again. hack/bats-no-negated-assert.bats fails on main because it reads the continuation line of a multi-line find in hack/select-e2e_test.bats as a bash statement starting with !. That ! is a find operand inside \( ... \), not a negated assertion, so the finding is a false positive, but it fails every PR rebased onto main and skips its E2E.

The two changes met on main without a run in between: #4658 last ran CI before the audit landed, and was merged after it. This PR spells the operand as -not, which is the same find predicate. Teaching the audit to join continued lines is a separate change.

Both hack/bats-no-negated-assert.bats and hack/select-e2e_test.bats pass locally.

Screenshots

Not a UI change.

Downstream repositories

Release note

NONE

Summary by CodeRabbit

  • Bug Fixes
    • Improved compatibility when listing the end-to-end test suite; the directories excluded from the list remain unchanged.

The negated-assertion audit reads a continued line that starts with
`!` as a bash statement, so the `! -type d` operand inside the find
expression on the continuation line turned the unit lane red on main.
`-not` is the same find predicate and is not a statement prefix.

Assisted-by: LLM
Signed-off-by: Aleksei Sviridkin <[email protected]>
@github-actions github-actions Bot added size/XS This PR changes 0-9 lines, ignoring generated files area/testing Issues or PRs related to testing (e2e, bats, unit tests) labels Oct 5, 2026
@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/agents/e2e-testing.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: cozystack/cozystack/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 91c0a7a3-2fbe-4df9-8efd-0b70e3fb677a
📥 Commits

Reviewing files that changed from the base of the PR and between ec4d26d and ceb1afe.

📒 Files selected for processing (1)
  • hack/select-e2e_test.bats

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

full_suite_list now uses -not instead of ! to exclude directories from matching paths. The directory-exclusion predicate and resulting suite enumeration are otherwise unchanged.

Changes

Suite Enumeration

Layer / File(s) Summary
Directory exclusion predicate
hack/select-e2e_test.bats
full_suite_list uses -not -type d instead of ! -type d to exclude directories.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~2 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to ceb1a

The change is intended to preserve suite selection, and no failure is established. The unit-test job exercises the helper, so normal CI should confirm it before merge.

Architecture Summary

Architecture risk: 🔵 Low · up to ceb1a

The change affects 1 system.

Changed systems: hack

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — hack (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in hack/select-e2e_test.bats: full_suite_list uses -not -type d instead of ! -type d to exclude directories from the matching paths.
🚥 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 and concisely describes the change from ! to -not in select-e2e_test.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
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 💡 1
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@IvanHunters IvanHunters left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Verified locally: -not -type d and ! -type d produce byte-identical find output (81 paths), and full_suite_list() returns the same 24 suites. Confirmed the root cause too: on the old spelling hack/bats-no-negated-assert.bats goes red at select-e2e_test.bats:34 (its lexical scan reads the ! after \( as a negated subshell assertion, the audit's own documented over-count), and with -not the audit is green 10/10. select-e2e_test.bats passes on the head. Equivalent predicate, minimal diff, unblocks the unit lane.

@IvanHunters
IvanHunters enabled auto-merge October 5, 2026 12:21
myasnikovdaniil added a commit that referenced this pull request Oct 5, 2026
#4751)

## What this PR does

Unit tests on main are red, and that blocks E2E on every open PR. Three
contracts broke the same way. In each case a check from one PR and a
line from another merged a few minutes apart on 2026-10-05, with no
textual conflict. Any one of them alone keeps the job red, so they are
fixed together.

The first is the pin on how a suite reads HelmRelease history. #4689
added it. It accepts `$(kubectl ...` and, since #3594, the bounded
`$(_cozy_guard_kubectl ...` wrapper. Nothing in front of kubectl is
allowed. The monitoring suite from #4678 bounds that read with
`$(timeout 30 kubectl ...`, and the bound is wanted. So the pin now
accepts an optional `timeout [-k N] N` in front of kubectl. The prefix
is spelled out on purpose. Any text would also let through `$(echo
kubectl ...)`, which names the shared expansion without running it, and
that form is now a refuse case. I checked it with mutations: the new
accept cases fail against the old pattern, and a pin that takes any
prefix fails a refuse case.

The second is the flux install in workflows. #4645 forbids running
fluxcd.io/install.sh and moved every workflow to the SHA-pinned
fluxcd/flux2 action. The isp-hosted lane from #4712 still ran the
script. It now uses the same action and version.

The third is the negated-assertion audit. It reads the find operand in
`\( ! -type d` in select-e2e_test.bats as a shell negation and fails.
The line now says `-not -type d`. This is the same change as #4748, so
this PR supersedes it. The audit bug itself is #4752.

After the Go fix, the full log of the unit job had only these two bats
failures left.

Closes #4750
Closes #4755

### Screenshots

### 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): fix three unit-test contracts that went red on main after adjacent merges.
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* History checks now recognize direct `kubectl` reads and reads through
the supported wrapper, including timeout-bounded commands with an
optional kill-grace setting. Commands that only include `kubectl` as an
argument, use similarly named commands, or place a timeout before the
wrapper are not treated as reads.
* **Chores**
* The hosted end-to-end workflow now uses a pinned Flux version, and
suite selection handles directory exclusions consistently.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@lexfrei

Copy link
Copy Markdown
Contributor Author

Closing, #4751 landed the same -not -type d change along with the other fixes main needed.

auto-merge was automatically disabled October 5, 2026 20:48

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/XS This PR changes 0-9 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants