🐛 fix: Go test alignment unblocking nightly release (#8310 #8313 #8314)#8317
Merged
clubanderson merged 1 commit intomainfrom Apr 16, 2026
Merged
🐛 fix: Go test alignment unblocking nightly release (#8310 #8313 #8314)#8317clubanderson merged 1 commit intomainfrom
clubanderson merged 1 commit intomainfrom
Conversation
…8314) The nightly Release workflow has been red since 2026-04-13 because four Go tests fell behind handler changes shipped over the past two weeks. Each test now aligns with the current handler contract: - `pkg/agent/server_test.go:TestServer_SettingsHandlers` — expected the keys endpoint to return 0 providers, but #8248/#8254/#8256 now register nine chat-only HTTP providers (3 OpenAI-compatible gateways + 6 local LLM runners) so the Settings modal can show per-provider base URL overrides. Assert non-empty list and Provider presence. - `pkg/api/handlers/cards_test.go:TestRecordFocus_BadBody_Returns400` — `RecordFocus` now runs `requireEditorOrAdmin` before BodyParser (#7011), which calls `store.GetUser`. The test's custom `recordFocusStore` never registered `GetUser`, so the mock panicked. Add the admin-user expectation. - `pkg/api/handlers/dashboard_test.go:setupDashboardTest` — `CreateDashboard` now calls `store.CountUserDashboards` to enforce the per-user dashboard limit (#7010). Register a default `Return(0, nil).Maybe()` so the three CreateDashboard tests stay under the limit. Tests exercising the limit can override. - `pkg/api/handlers/setup_test.go` — `TestClusterGroupsCRUD` exercises handlers that persist definitions via `SaveClusterGroup` / `DeleteClusterGroup` / `ListClusterGroups` (#7013). Register permissive `.Maybe()` mocks on the shared `setupTestEnv` MockStore. Verification: `go test ./...` — all packages pass locally. Fixes #8310, Fixes #8313, Fixes #8314 Signed-off-by: Andy Anderson <[email protected]>
kubestellar-prow
Bot
added
the
dco-signoff: yes
Contributor
|
[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 |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
Bot
added
the
size/M
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Go unit tests to align with recent handler behavior changes in the API server and kc-agent, unblocking failing nightly release workflows.
Changes:
- Adjusts
kc-agentsettings keys test expectations to reflect the now-populated/settings/keysstatus response. - Extends handler test setup mocks to cover newly-added store calls (dashboard count limiting and cluster-group persistence).
- Fixes
RecordFocustest setup to satisfy the new RBAC role check that callsstore.GetUser.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/api/handlers/setup_test.go | Adds permissive MockStore expectations for cluster-group persistence methods to prevent panics in CRUD tests. |
| pkg/api/handlers/dashboard_test.go | Adds a default CountUserDashboards mock expectation so CreateDashboard tests remain under the new per-user limit. |
| pkg/api/handlers/cards_test.go | Mocks GetUser for RecordFocus to pass the new requireEditorOrAdmin check before body parsing. |
| pkg/agent/server_test.go | Updates TestServer_SettingsHandlers to expect a non-empty keys status list and validate Provider presence. |
Contributor
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
Contributor
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
Contributor
✅ Post-Merge Verification: passedCommit: |
clubanderson
added a commit
that referenced
this pull request
Apr 16, 2026
…nk (#8277) Addresses 2 of the 5 Copilot review comments on merged PR #8267: - pkg/agent/server_operations.go:348 — stop setting BaseURLSource="default" for local LLM runners using the compiled-in default. KeyStatus docs and the frontend type (`'env' | 'config'`) treat empty as the canonical signal for "value is the compiled-in default". The "default" string was unhandled by the UI and broke the documented contract. - web/src/components/agent/APIKeySettings.tsx:248 — handleSaveBaseURL with an empty trimmed input now sends `{ clearBaseURL: true }` instead of `{ baseURL: "" }`, so the backend removes the persisted override and reverts to the compiled-in default. Without this the request hit the missing_field guard once clearBaseURL was introduced. Not addressed here (intentional, kept for follow-up): - The "Configured = URL-present" semantics for local LLMs is contested: the existing comment marks it as intentional (#8259), so treating compiled-defaults as "not configured" would be a behavior change, not a bug fix. - Populating BaseURL via per-provider resolvers for non-local providers (Groq/OpenRouter) so it always shows the effective endpoint — separate PR scope, requires resolver-side changes. - TestServer_SettingsHandlers test mismatch — already fixed in PR #8317. Fixes #8277 Signed-off-by: Andy Anderson <[email protected]>
3 tasks
clubanderson
added a commit
that referenced
this pull request
Apr 16, 2026
…nk (#8277) (#8320) Addresses 2 of the 5 Copilot review comments on merged PR #8267: - pkg/agent/server_operations.go:348 — stop setting BaseURLSource="default" for local LLM runners using the compiled-in default. KeyStatus docs and the frontend type (`'env' | 'config'`) treat empty as the canonical signal for "value is the compiled-in default". The "default" string was unhandled by the UI and broke the documented contract. - web/src/components/agent/APIKeySettings.tsx:248 — handleSaveBaseURL with an empty trimmed input now sends `{ clearBaseURL: true }` instead of `{ baseURL: "" }`, so the backend removes the persisted override and reverts to the compiled-in default. Without this the request hit the missing_field guard once clearBaseURL was introduced. Not addressed here (intentional, kept for follow-up): - The "Configured = URL-present" semantics for local LLMs is contested: the existing comment marks it as intentional (#8259), so treating compiled-defaults as "not configured" would be a behavior change, not a bug fix. - Populating BaseURL via per-provider resolvers for non-local providers (Groq/OpenRouter) so it always shows the effective endpoint — separate PR scope, requires resolver-side changes. - TestServer_SettingsHandlers test mismatch — already fixed in PR #8317. Fixes #8277 Signed-off-by: Andy Anderson <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nightly
Releaseworkflow has been failing every night since 2026-04-13 (runs: 24328017228, 24383107176, 24438572049, 24494366158). Root cause is four Go tests that fell out of sync with handler changes shipped in the last two weeks.TestServer_SettingsHandlersexpected the agent/settings/keysresponse to be empty, but ✨ Register local LLM providers (Ollama, llama.cpp, LocalAI, vLLM, LM Studio, RHAIIS) in agent selector #8248/Add a per-provider Base URL field to APIKeySettings (Advanced) #8254/✨ Settings → API Keys: Advanced Base URL field per local LLM provider (closes #8254) #8256 now register nine chat-only HTTP providers (3 OpenAI-compatible gateways + 6 local LLM runners) so the Settings modal can render a per-provider base URL override field. Asserts non-empty list andProviderpresence instead.TestRecordFocus_BadBody_Returns400panicked becauseRecordFocusnow runsrequireEditorOrAdmin(RecordFocus missing requireEditorOrAdmin check allowing viewer-role users to write to card focus and event log #7011), which callsstore.GetUser— the test's customrecordFocusStorewrapper never registered the expectation. Adds the admin-user mock so the role check passes and the test reaches its intended BodyParser check.setupDashboardTest—CreateDashboardnow enforces a per-user dashboard limit (CreateDashboard has no per-user dashboard limit allowing unlimited storage consumption #7010) viastore.CountUserDashboards. Default helper registersReturn(0, nil).Maybe()so the three existing CreateDashboard tests stay under the limit; tests exercising the limit can override.setupTestEnv—TestClusterGroupsCRUDpanicked because cluster-group handlers persist to the store (In-memory clusterGroups map loses all state on server restart diverging from Kubernetes labels permanently #7013) viaSaveClusterGroup/DeleteClusterGroup/ListClusterGroups. Registers permissive.Maybe()mocks on the shared MockStore.Test plan
go test ./...— all packages pass locallyFixes #8310, Fixes #8313, Fixes #8314