Add unit tests for pkg/vmcp/cli serve and validate logic#4898
Add unit tests for pkg/vmcp/cli serve and validate logic#4898yrobla wants to merge 1 commit intoissue-4879from
Conversation
yrobla
requested review from
JAORMX,
amirejaz,
jerm-dro and
jhrozek
as code owners
github-actions
bot
added
size/M
github-actions
bot
added
size/M
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage for pkg/vmcp/cli’s Validate path plus key serve-related helpers (config loading/validation, auth-server sidecar config loading, backend discovery wiring, and session factory HMAC-secret behavior), aligning with the goal of keeping pkg/ business logic well-tested.
Changes:
- Added table-driven tests for
Validatecovering missing path, valid config, missing file, malformed YAML, and semantic validation failure. - Added table-driven tests for
loadAndValidateConfig, plus targeted tests forloadAuthServerConfig,discoverBackends(static path), andcreateSessionFactoryHMAC-secret scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/vmcp/cli/validate_test.go | Adds table-driven unit tests for Validate covering success and key failure modes. |
| pkg/vmcp/cli/serve_test.go | Adds unit tests for serve helper functions: config loading/validation, authserver config lookup, backend discovery (static), and session factory behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Cover loadAndValidateConfig, loadAuthServerConfig, discoverBackends, createSessionFactory, and Validate with table-driven unit tests in pkg/vmcp/cli. Also moves the existing loadAuthServerConfig test from cmd/vmcp/app (where it lived before the thin-wrap refactor) to pkg/vmcp/cli where the function now lives. env var tests use os.Setenv + t.Cleanup via a setEnvForTest helper to remain compatible with t.Parallel(). Part of #4808. Closes #4881.
github-actions
bot
added
size/M
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## issue-4879 #4898 +/- ##
==============================================
+ Coverage 69.07% 69.26% +0.19%
==============================================
Files 533 533
Lines 55195 55195
==============================================
+ Hits 38126 38231 +105
+ Misses 14147 14034 -113
- Partials 2922 2930 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| `, | ||
| wantErr: true, | ||
| errContains: "validation failed", |
There was a problem hiding this comment.
The "fails semantic validation — missing groupRef" case isn’t isolating the missing groupRef failure: this YAML also omits aggregation.conflictResolutionConfig, which is required and will cause validation to fail even if groupRef were present. To make the test actually cover the intended rule (and be more robust), include the required aggregation config in this fixture and assert the wrapped error contains the underlying validation message (e.g. "group reference is required").
| `, | |
| wantErr: true, | |
| errContains: "validation failed", | |
| conflictResolutionConfig: | |
| fieldNamePrefix: | |
| separator: "-" | |
| `, | |
| wantErr: true, | |
| errContains: "group reference is required", |
| name: "config missing required groupRef", | ||
| setup: func(t *testing.T) ValidateConfig { | ||
| t.Helper() | ||
| path := filepath.Join(t.TempDir(), "invalid.yaml") | ||
| // groupRef is required; omitting it must fail validation. | ||
| require.NoError(t, os.WriteFile(path, []byte(` | ||
| name: test-vmcp | ||
| incomingAuth: | ||
| type: anonymous | ||
| outgoingAuth: | ||
| source: inline | ||
| aggregation: | ||
| conflictResolution: prefix | ||
| `), 0o600)) | ||
| return ValidateConfig{ConfigPath: path} | ||
| }, | ||
| wantErr: true, | ||
| errContains: "validation failed", | ||
| }, |
There was a problem hiding this comment.
This test case is named/commented as "missing required groupRef", but the YAML fixture also omits aggregation.conflictResolutionConfig, which is required and may be the reason validation fails. To ensure the test actually verifies the groupRef requirement, include the required aggregation config in the invalid YAML and assert the error contains the underlying validator message ("group reference is required") rather than only the generic wrapper text.
Summary
Cover loadAndValidateConfig, loadAuthServerConfig, discoverBackends, createSessionFactory, and Validate with table-driven unit tests in pkg/vmcp/cli. Also moves the existing loadAuthServerConfig test from cmd/vmcp/app (where it lived before the thin-wrap refactor) to pkg/vmcp/cli where the function now lives.
env var tests use os.Setenv + t.Cleanup via a setEnvForTest helper to remain compatible with t.Parallel().
Fixes #4881
Type of change
Test plan
task test)task test-e2e)task lint-fix)Changes
Does this introduce a user-facing change?
Implementation plan
Approved implementation plan
Special notes for reviewers