Repository navigation
fix(ci): close e2e suite selection gaps in select-e2e and select-install - #4634
Conversation
A path under hack/e2e-chainsaw/<dir>/ put <dir> into the selection without checking that it is a suite, leaving the final intersection to drop it. Alone the path escalated through the empty-selection backstop, but beside any path that contributed a suite the escalation vanished and the run narrowed to that suite with nothing on stderr. Shared material next to _lib/ or a suite nested deeper than discovery looks then went untested in exactly the pull requests that also touch an app. The directory name is now membership-tested where the path is classified, as the hack/e2e-apps/ arm already does, so the decision is a property of the path and names it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
cozystack.seaweedfs-application mapped to a seaweedfs suite that does not exist, so every seaweedfs change reached nothing runnable and paid for the whole Chainsaw run. The suites that can regress from it are bucket, the backup round-trips that write to the tenant-root instance through its in-cluster S3 endpoint, and harbor, whose registry storage is a BucketClaim served by the seaweedfs COSI driver with no graph edge between the two. bucket alone would miss the CA and service-name paths the round-trips depend on. The test derives the expected set from the suites' own yaml, so a new suite that names the endpoint or a BucketClaim there fails it until it is mapped. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
Chainsaw loads chainsaw-test.yml when a directory has no chainsaw-test.yaml, but select-e2e and select-install enumerated the .yaml name only. A suite spelled that way ran whenever the whole directory was handed to chainsaw, yet no escalation listed it and no edit to it could select it, so it silently dropped out of every narrowed run. select-install also sent find's errors to /dev/null inside a pipeline that reported sort's status, so a failed listing validated no suite at all and passed. It is now captured and fails validation. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
The comments explaining the unit/e2e bats split quoted how many files each side held, and the escalation helper how many branches call it. Every figure had gone stale: nothing checks them, and they move each time a bats file or an escalation is added. The Makefile expression quoted beside the bats claim is what defines the set, so the explanation stands without the number. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
examples/backups/<app>/ selects that app's suite, and a directory whose name is no suite selects nothing. A helpers directory shared by every walkthrough, examples/backups/_lib/, falls into the second case: an edit to it alone produces an empty selection, which both lanes read as "skip Chainsaw", so a change reaching every backup round-trip would run none. It now escalates to the full suite like hack/e2e-chainsaw/_lib/. The set of suites that run a walkthrough is spread across the suites' own scripts, and a derived subset that misses one would fail open. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/ci
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe E2E selectors now discover Chainsaw suites defined with either ChangesE2E suite selection
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The changes improve suite selection and validation without an identified merge-blocking issue. Merge after normal checks pass. 🚥 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 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM
Closes three real E2E-selector gaps, each pinned by a test that fails without the fix, verified offline by mutation.
What I checked
I ran the bats suites offline, no cluster, and reverted each new guard in turn: every one reddens exactly the test named for it.
-
chainsaw-test.ymldiscovery. Both selectors now enumeratechainsaw-test.yamlorchainsaw-test.yml, strip either suffix, andsort -uso a directory holding both spellings counts once. A.ymlsuite that previously ran but was never listed (and so never selected by its own per-suite edit) is now reachable.chainsaw-test.yml.disabledstill lands on the.disabledarm and stays inert, andfind -name chainsaw-test.ymldoes not match it. -
The
hack/e2e-chainsaw/*/*arm now membership-tests the derived suite againstall_appson the spot: present selects it, absent escalates with a named reason on stderr. This removes the merge-before-escalate shape (#3330) where a non-suite directory beside a path that contributed a suite narrowed the run silently. Confirmed by mutation: dropping thegrep -Fxqsends the path back through the empty-selection backstop, which the rewritten test catches. -
seaweedfs-applicationpreviously mapped to a suite that does not exist, so every seaweedfs change ran the whole suite. The new explicit list narrows that to the suites that actually use the tenant-root instance. I checked the list against the tree, not just against the test: the suites namingseaweedfs-s3/BucketClaim/objectstorage.k8s.ioin their yaml are exactlybucket clickhouse harbor kafka kafka-metadata mariadb mongodb postgres rabbitmq redis.foundationdbcarries only an empty, disableds3:block andetcdnames the endpoint in a comment while CI gates its round-trip off, so both are correctly excluded. The self-checking test reddens if the hardcoded list and the yaml-derived set ever drift. -
select-install.shdiscover_suitesnow capturesfind's status on its own instead of letting the pipeline reportsort's, so a failed listing fails closed in validate mode rather than validating nothing and passing. This only touches the--validateself-check; the closure (install-selection) path reads its suites from arguments and is unchanged, so selection for existing suites is not affected.
Caveats
One non-blocking note, no action required: the seaweedfs derivation sees only yaml-level references, so a suite that reached the instance exclusively through a sourced script would be invisible to both the list and the test. No such suite exists today and the comment already calls this out; worth a glance the next time a backup round-trip is added. Tested against origin/main: the selection scripts, the hack/e2e-chainsaw/ tree and cozytest.sh are unchanged on main since this branch's base, so the full-suite list the tests derive matches and there is no post-base test on main that this branch would fail.
2 participants
What this PR does
The e2e test selector picked the wrong Chainsaw suites for some diffs. In one case it lost an escalation, in another it ran the full suite where a subset was enough, and in two more it missed a suite or ran nothing. The fixes are in
hack/select-e2e.sh, with the.ymlpart also inhack/select-install.sh.hack/e2e-chainsaw/that holds no suite escalated only when it was the whole diff. Next to any path that selected a suite, the escalation was dropped with nothing on stderr. The directory name is now checked against the suite list right where the path is classified, like thehack/e2e-apps/rule already did. The empty-selection backstop can't be reached anymore. I kept it as a guard, and the doc says its line has no test.cozystack.seaweedfs-applicationmapped to aseaweedfssuite that does not exist. Now it selects the suites that use tenant-root's object storage: bucket, the backup round-trips that write through the in-clusterseaweedfs-s3endpoint, and harbor, which keeps its registry in a BucketClaim served by the seaweedfs COSI driver. The issue says this mapping would break the round-trip test. It doesn't, since that test only walks suite to source to suite, andvm-disk-applicationalready maps tovminstancethe same way. The test takes the expected set from the suites' own yaml.chainsaw-test.ymlwas invisible to both selectors. Chainsaw v0.2.15 loads that name when there is nochainsaw-test.yaml(pkg/discovery/load.go), so such a suite runs but never gets selected. Both scripts list it now.select-install.sh --validatealso passed whenfindfailed, and now it fails.examples/backups/_lib/was read as an app named_liband selected nothing. It escalates now, same ashack/e2e-chainsaw/_lib/.Each fix has a test that fails on main. I also mutated each fix: its own test goes red and no other test does.
Three related gaps outside these lines are tracked in #4630: the
packages/core/testing/Makefilecomment that names onlychainsaw-test.yaml, suites built from numbered step files, and a file placed directly inexamples/backups/.Two things worth knowing before merge:
.disabledone in a parked suite's directory, likehack/e2e-chainsaw/kubernetes-proxmox/tenant.yaml, now always runs the full suite. That is the safe direction. If it gets expensive, those directories could be made inert.examples/backups/_lib/rule and its doc line point at a directory another PR adds. If this PR merges first, the rule just has nothing to match yet.Closes #3816, closes #3710, closes #4283, closes #4120, closes #4205, closes #4626.
Screenshots
Not a UI change.
Downstream repositories
Release note
Summary by CodeRabbit
Bug Fixes
Documentation