Skip to content

Isolate skills client discovery in default-client test#5417

Merged
danbarr merged 1 commit into
mainfrom
fix-skills-client-discovery-isolation
Jun 1, 2026
Merged

Isolate skills client discovery in default-client test#5417
danbarr merged 1 commit into
mainfrom
fix-skills-client-discovery-isolation

Conversation

@danbarr
Copy link
Copy Markdown
Collaborator

@danbarr danbarr commented Jun 1, 2026

Summary

TestNewDefaultClient stubbed the TOOLHIVE_API_URL env reader but let the server-discovery step run against real local state. When a thv serve or Desktop server was running, discovery found it and returned its base URL (http://localhost for a Unix socket), so the "falls back to default URL when env is empty" and "applies options" subtests saw the discovered URL instead of http://127.0.0.1:8080 and failed. CI passed only because no server runs there, making this a confusing local-only failure.

newDefaultClientWithEnv already injects an env.Reader so the env-var step is testable; the discovery step just wasn't given the same treatment.

  • Add a discoverFunc parameter to newDefaultClientWithEnv, mirroring the existing env.Reader injection. NewDefaultClient passes the real resolveViaDiscovery; no production behavior change.
  • Stub discovery in the subtests so each resolution step (env var, discovery, default fallback) is tested in isolation, with no dependency on real local state.
  • Add a subtest covering the discovered-server path, which was previously untested.

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Manual: ran go test -count=1 -run TestNewDefaultClient ./pkg/skills/client/; all four subtests pass. The discovery step is now fully stubbed, so the test is robust to a running local server by construction.

Does this introduce a user-facing change?

No. The change adds a test-injection seam; NewDefaultClient behaves identically.

🤖 Generated with Claude Code

TestNewDefaultClient stubbed the TOOLHIVE_API_URL env reader but let
the server-discovery step run against real local state. When a thv or
Desktop server was running, discovery found it and returned its base
URL, so the "falls back to default URL" and "applies options" subtests
saw the discovered URL instead of the default and failed. CI passed
only because no server is running there.

Inject the discovery step into newDefaultClientWithEnv, mirroring the
existing env.Reader injection, and have the subtests stub it. Each
resolution step (env var, discovery, default fallback) is now tested
in isolation, and a new subtest covers the discovered-server path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@danbarr danbarr requested a review from JAORMX as a code owner June 1, 2026 14:43
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Jun 1, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 1, 2026

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.79%. Comparing base (5a8692d) to head (b2c456f).

Files with missing lines Patch % Lines
pkg/skills/client/client.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5417      +/-   ##
==========================================
+ Coverage   68.76%   68.79%   +0.02%     
==========================================
  Files         629      629              
  Lines       63922    63922              
==========================================
+ Hits        43958    43976      +18     
+ Misses      16707    16689      -18     
  Partials     3257     3257              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@danbarr danbarr merged commit 2d59159 into main Jun 1, 2026
45 checks passed
@danbarr danbarr deleted the fix-skills-client-discovery-isolation branch June 1, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants