🐛 Restore pkg/api/handlers/ops coverage above ratchet floor (main-broken #23762) - #23763
Conversation
PR #23759 added a console_self_upgrade_trigger_total{outcome} sample on every branch of SelfUpgradeHandler.TriggerUpgrade, but most of those branches had no test, so the new statements dropped the ops package to 65.1% — below its 66.0% floor in .github/go-package-coverage-ratchet.txt — and broke `go test ./...` on main (run 36294510853). Forward-fix rather than revert: the metric is useful, the gap was only test coverage. Add tests that walk every non-success TriggerUpgrade outcome (store unavailable, user lookup failed / not found, invalid body, empty tag, not in-cluster, nil k8s client, unknown namespace, client unavailable, deployment not found, RBAC denied, no containers, patch failed) plus RegisterSelfUpgrade, SendAlertNotification and SaveNotificationConfig, which were at 0%. Package coverage goes to 73.2% locally with -race. Root cause / prevention: CI's per-package ratchet only runs on the PR, and #23759's own run passed because the floor is compared against the rounded package figure; adding un-tested statements on many branches is what tipped it. Reviewers should expect tests alongside any change that adds statements to a package sitting near its floor. Fixes #23762 Hive-Run: #23762 Hive-Plan: main-broken-go-tests-forward-fix Hive-Spec: build-sheriff-recovery#coverage-ratchet Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
✅ Deploy Preview for kubestellarconsole canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/L
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
|
🐝 Hi @clubanderson! I'm Trusted users — org members and contributors with write access — can mention Automation may take a moment to start, and follow-up happens through workflow activity rather than chat replies. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues; coverage tests address the reported gap.
Review effort: Lite
Findings: None
What changed in this PR
Adds tests restoring pkg/api/handlers/ops coverage above its ratchet floor.
Changes:
- Covers self-upgrade outcomes and route registration.
- Covers notification send and configuration paths.
| File | Description |
|---|---|
pkg/api/handlers/ops/self_upgrade_outcomes_test.go |
Tests self-upgrade outcomes and routing. |
pkg/api/handlers/ops/notifications_send_config_test.go |
Tests notification send/configuration handlers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Read the new tests against the handlers at this head — this looks correct to me (COMMENT only; a maintainer should approve/merge).
Verified against the tree, not just the diff:
- Every asserted error string exists in
pkg/api/handlers/ops/self_upgrade.go— e.g."user store is not configured"(line 250),"imageTag is required"(295),"could not determine pod namespace"(319),"cluster client unavailable"(331),"deployment not found"(341),"insufficient RBAC permissions"(349),"has no containers"(357). The notifications strings ("Invalid request body","Failed to send notification","Notification configuration validated successfully") matchpkg/api/handlers/ops/notifications.go:100–152. - The tests that hit non-403 paths without setting a user work because
setup_test.go:90–93injectstestAdminUserIDby default and mocks it as admin (setup_test.go:84–87); the viewer test's extraUseoverwrites that local, so the 403 path is real. dummyRestConfig,fiberTestTimeout, andsetupTestEnvare pre-existing package helpers (self_upgrade_test.go:80,setup_test.go:26,48) — no new scaffolding.- Test-only change to an existing package: no ratchet-file entry needed, and the coverage job on this head is green (29/29 completed checks succeeded).
One nit, not a blocker: TestSelfUpgradeHandler_TriggerUpgrade_NamespaceUnknown self-skips when run inside a pod with a mounted service-account namespace, so that branch is only covered on CI/dev machines — acceptable given CI is where the ratchet is enforced.
— hive: agent=reviewer backend=copilot model=claude-fable-5 copilot=1.0.88
hivecommons-hive
Bot
added
agent/scanner
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
⏹️ Post-Merge Verification: cancelledCommit: |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
📌 Fixes
Fixes #23762
📝 Summary of Changes
Go Testsonmainwent red at 87246c2 (#23759, run 36294510853):Triage: real breakage, not flake. #23759 added a
console_self_upgrade_trigger_total{outcome}sample on every branch ofSelfUpgradeHandler.TriggerUpgrade; most of those branches were untested, so the new statements pushed the ops package under its floor.Resolution: forward-fix (tests only, no production code touched). The metric itself is sound and worth keeping, so reverting would lose it for no gain.
Changes Made
pkg/api/handlers/ops/self_upgrade_outcomes_test.go— one test per non-successTriggerUpgradeoutcome: store unavailable, user lookup failed, user not found, invalid body, empty tag, not in-cluster, nil k8s client, unknown namespace, in-cluster client unavailable, deployment not found, RBAC denied, no containers, patch failed; plusRegisterSelfUpgrade(was 0%).pkg/api/handlers/ops/notifications_send_config_test.go—SendAlertNotificationandSaveNotificationConfig(both were 0%), covering admin gate, bad body, empty channels, channel error and success paths.Coverage:
pkg/api/handlers/ops65.1% → 73.2% (go test -race -covermode=atomic ./pkg/api/handlers/ops/).TriggerUpgrade58.3% → 95.2%.Ratchet file deliberately left at 66.0 — locking in the new figure is a separate follow-up ratchet PR per the workflow's own notice.
Post-mortem (per
docs/INCIDENT-RESPONSE.md)Checklist
— hive: backend=copilot model=claude-fable-5.1