[test] Add tests for config.expandMapInPlace#9084
Merged
Merged
Conversation
expandMapInPlace has zero direct test coverage despite having three distinct code paths: 1. Empty/nil map fast path - returns nil without calling expandEnvVariables 2. Happy path - expands variables in map values and updates *m in place 3. Error path - wraps underlying error with server name and field description using the format: server %q: failed to expand %s: %w Tests cover: - nil map (pointer to nil - empty fast path) - empty map (empty fast path) - all variables defined (in-place mutation verified by caller) - literal values pass through unchanged - undefined variable produces wrapped error mentioning server name - error includes fieldDesc from call site (env vars vs HTTP headers) - pointer semantics: caller sees expanded map after return - mixed defined/undefined variables - variable expanded to empty string is valid - error wraps the underlying undefined-variable error (var name visible) - server name with special chars is quoted in error message (%q format) Previous coverage: 0% (no direct tests) After this change: all branches of expandMapInPlace are covered Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds direct unit tests for internal/config.expandMapInPlace, an internal helper on the stdin config conversion path, to ensure ${VAR} expansion behavior (fast-path, success, and wrapped-error path) is covered and stable.
Changes:
- Introduces a new unit test suite covering nil/empty fast path, successful expansion, and wrapped error formatting.
- Verifies caller-visible pointer semantics (map replacement) and key edge cases (empty-string expansion, mixed defined/undefined vars).
Show a summary per file
| File | Description |
|---|---|
| internal/config/expand_map_in_place_test.go | Adds branch-covering unit tests for expandMapInPlace behavior and error wrapping. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
Comment on lines
+66
to
+71
| assert.True(t, strings.Contains(errStr, "my-server"), | ||
| "error should mention the server name, got: %q", errStr) | ||
| assert.True(t, strings.Contains(errStr, "environment variable(s)"), | ||
| "error should mention the field description, got: %q", errStr) | ||
| assert.True(t, strings.Contains(errStr, "failed to expand"), | ||
| "error should contain 'failed to expand', got: %q", errStr) |
Comment on lines
+89
to
+90
| // Save a reference to the original map to verify identity changes. | ||
| // expandMapInPlace replaces *m with a new map, so the pointer target changes. |
Collaborator
|
@copilot address review feedback |
…d_map_in_place_test
Contributor
Done in the latest commit. Replaced all |
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.
Function Analyzed
internal/configexpandMapInPlaceinternal/config/expand.goWhy This Function?
expandMapInPlaceis an internal helper used in two call sites (convertStdinServerConfig) to expand${VAR}expressions in both theenvandheadersmaps of each MCP server configuration. Despite being called by the critical config loading path, it had zero direct unit tests.The function has three distinct code paths:
nilwithout callingexpandEnvVariablesexpandEnvVariables, replaces*mwith the expanded copyserver %q: failed to expand %s: %wTests Added
fieldDescfrom call siteCoverage
Before: 0% direct coverage (no tests for
expandMapInPlace)After: All 3 branches covered (empty fast path, happy path, error path)
Generated by Test Coverage Improver
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
awmgmcpgproxy.golang.orgSee Network Configuration for more information.