test(skillget): registry 401/404/410 edge responses (XDE-92)#4
Conversation
Add httptest-backed tests for SearchSkills, ResolveInstallTarget, and PublishSkill so REGISTRY_CLIENT_CONTRACT status handling stays pinned under go test -short without live registry calls. Co-Authored-By: Paperclip <noreply@paperclip.ing> Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Adds stubbed registry (httptest) unit tests to validate that skillget-manager surfaces helpful error hints for key HTTP edge statuses (401/404/410) without requiring live network access, aligning CLI behavior with the documented registry client contract.
Changes:
- Add
httptest-based tests forSearchSkills401/404 hint strings. - Add tests for
ResolveInstallTarget404 (missing skill) and 410 (yanked version) responses. - Add test for
PublishSkill401 hint string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
|
|
||
| // Stub-registry tests for HTTP status handling (401 / 404 / 410) per REGISTRY_CLIENT_CONTRACT. | ||
| // Always run under go test -short; no live network. |
There was a problem hiding this comment.
The file header comment says these tests “Always run under go test -short”, but there’s no Short() gating here (these tests will also run in non--short runs). Consider rewording to “Safe to run under -short; no live network” or add an explicit skip when !testing.Short() if you truly want them restricted to short mode.
| // Always run under go test -short; no live network. | |
| // Safe to run under -short; no live network. |
| func TestRegistrySearchSkills_404IncludesSkillHint(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodGet || r.URL.Path != "/api/v1/skills" { | ||
| http.NotFound(w, r) | ||
| return | ||
| } | ||
| http.NotFound(w, r) | ||
| })) | ||
| defer srv.Close() | ||
| defer swapRegistryHTTPClient(t, srv.Client())() | ||
|
|
||
| t.Setenv("SKILLGET_REGISTRY_URL", srv.URL+"/api/v1") | ||
|
|
||
| _, err := skillgetmanager.SearchSkills(context.Background(), skillgetmanager.SearchSkillsOptions{Limit: 3}) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| s := err.Error() | ||
| if !strings.Contains(s, "404") { | ||
| t.Fatalf("expected status in error, got: %s", s) | ||
| } | ||
| if !strings.Contains(s, "SKILLGET_REGISTRY_URL") { | ||
| t.Fatalf("expected 404 hint, got: %s", s) | ||
| } |
There was a problem hiding this comment.
Test name says it “IncludesSkillHint”, but the assertion checks for the registry URL env var hint (SKILLGET_REGISTRY_URL). Rename the test to match what’s being asserted (or update the assertion to check for the intended “skill name” wording) to avoid confusion when these fail.
| if !strings.Contains(s, "SKILLGET_REGISTRY_READ_TOKEN") { | ||
| t.Fatalf("expected 401 registry hint, got: %s", s) |
There was a problem hiding this comment.
TestRegistryPublishSkill_401IncludesTokenHint currently asserts the 401 hint contains SKILLGET_REGISTRY_READ_TOKEN, but publish auth uses the write token (SKILLGET_REGISTRY_TOKEN / SKILLGET_TOKEN). To make the test reflect the publish contract and be less brittle if the shared 401 hint text changes, assert for SKILLGET_REGISTRY_TOKEN (and/or SKILLGET_TOKEN) instead.
| if !strings.Contains(s, "SKILLGET_REGISTRY_READ_TOKEN") { | |
| t.Fatalf("expected 401 registry hint, got: %s", s) | |
| if !strings.Contains(s, "SKILLGET_REGISTRY_TOKEN") && !strings.Contains(s, "SKILLGET_TOKEN") { | |
| t.Fatalf("expected 401 registry hint to mention SKILLGET_REGISTRY_TOKEN or SKILLGET_TOKEN, got: %s", s) |
Summary
Stub-registry (
httptest) tests for registry HTTP edge cases aligned with REGISTRY_CLIENT_CONTRACT:Runs under
go test -mod=vendor -short ./...(no live registry; integration tests remain behind-short/SKIP_SKILLGET_REGISTRY_INTEGRATION).Paperclip: XDE-92