test: rewrite cli.claude config tests to assert JSON output directly#2311
test: rewrite cli.claude config tests to assert JSON output directly#2311felixweinberger merged 3 commits intomainfrom
Conversation
The previous test_command_execution shelled out to 'uv run --frozen --with mcp[cli] mcp run <path> --help' with a 20s timeout to verify the generated command was executable. Appending --help only proves uv is installed; it doesn't validate the command would actually launch a server. update_claude_config is a config-file writer — its contract is producing correct JSON. Test that directly: - Assert exact command + args structure (replaces the subprocess smoke test) - Mock get_uv_path so tests don't depend on the host's uv install - Cover previously-untested option paths: with_packages (sorted/deduped), with_editable, env_vars, env merge-on-reinstall, file specs without :object, preserving other mcpServers entries - Add get_uv_path tests for shutil.which resolution and fallback Removes 7 now-stale '# pragma: no cover' annotations from claude.py. Moves the file to tests/cli/test_claude.py to mirror src/mcp/cli/claude.py.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — pure test rewrite with no production logic changes.
Extended reasoning...
Overview
This PR rewrites the Claude Desktop config tests (tests/client/test_config.py → tests/cli/test_claude.py). The only production code change in src/mcp/cli/claude.py is removing 7 # pragma: no cover annotations — zero logic changes. The new test file replaces 2 flaky subprocess-dependent tests with 11 hermetic, mocked tests covering all branch paths.
Security risks
None. The production code is unchanged in behavior. The test file uses standard pytest fixtures with tmp_path and monkeypatch — no security-sensitive patterns.
Level of scrutiny
Low scrutiny warranted. This is a test-only improvement that makes the test suite more reliable and comprehensive. The pragma removals are mechanical and correct since the branches are now covered by tests.
Other factors
The reported bug (Windows drive-letter colon parsing) is pre-existing and not introduced or modified by this PR. The new tests are well-structured, use proper mocking (monkeypatch.setattr over unittest.mock.patch), and the test file location now correctly mirrors the source tree. The PR description clearly explains the motivation and changes.
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Nice thanks - LGTM.
Could consider fixing the pre-existing thing @claude caught above, but optional.
|
Claude finished @felixweinberger's task in 1m 58s —— View job Fixed Windows drive-letter bug ✅I've implemented and committed the fix for the pre-existing Windows path parsing bug. Changes MadeSource code fix ( # Convert file path to absolute before adding to command
# Split off any :object suffix first
# First check if we have a Windows path (e.g., C:\...)
has_windows_drive = len(file_spec) > 1 and file_spec[1] == ":"
# Split on the last colon, but only if it's not part of the Windows drive letter
if ":" in (file_spec[2:] if has_windows_drive else file_spec):
file_path, server_object = file_spec.rsplit(":", 1)
file_spec = f"{Path(file_path).resolve()}:{server_object}"
else:
file_spec = str(Path(file_spec).resolve())This applies the same Windows drive-letter detection logic that was already present in Test coverage (
What This FixesBefore this fix, on Windows:
After this fix:
CommitPushed to CI will verify tests pass, coverage remains at 100%, and formatting/type checking succeed. |
Before this fix, `update_claude_config` would incorrectly split Windows paths like `C:\Users\foo\server.py` on the drive letter colon, treating `\Users\foo\server.py` as an object suffix. This would generate broken config entries that made MCP servers unlaunchable from Claude Desktop on Windows. Apply the same Windows drive-letter detection logic already present in `_parse_file_path` (cli.py:97-102): check if position 1 is a colon, and if so, only consider colons after position 2 as object-suffix separators. Add test coverage for Windows paths with and without `:object` suffixes. Co-authored-by: Felix Weinberger <felixweinberger@users.noreply.github.com>
The previous tests mocked Path.resolve with a conditional that fell through
to the original implementation — but with the fix in place, only the full
path ever hits resolve(), so the fallthrough was dead code and missed
coverage (lines 129, 146).
Replace with a single parametrized test that records what resolve() was
called with. Before the fix, a bare 'C:\path\server.py' would rsplit on
the drive colon and call resolve(Path('C')); now it calls resolve on the
full path. The recorder asserts that directly — no dead branches.
Also covers the has_windows_drive=True + :object-suffix branch that was
previously missed.
There was a problem hiding this comment.
LGTM — straightforward test rewrite replacing non-hermetic subprocess tests with direct JSON assertions, plus a clean bug fix for Windows drive-letter handling.
Extended reasoning...
Overview
This PR replaces two flaky, subprocess-dependent tests in tests/client/test_config.py with 11+ hermetic unit tests in tests/cli/test_claude.py. The source changes in src/mcp/cli/claude.py are limited to removing stale # pragma: no cover annotations (now that the branches are tested) and adding a Windows drive-letter guard that mirrors the existing pattern in _parse_file_path.
Security risks
None. The source changes are a bug fix for path parsing and removal of coverage pragmas. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low scrutiny is appropriate. This is a test-quality improvement PR. The only behavioral change in production code is the Windows drive-letter fix, which applies an existing, well-understood pattern (has_windows_drive = len(file_spec) > 1 and file_spec[1] == ":") that is already used elsewhere in the same codebase.
Other factors
The new tests are well-structured: they mock external dependencies (get_claude_config_path, get_uv_path), test edge cases (env-var merging, package deduplication, preserving other servers, missing config dir), and cover the Windows drive-letter fix with two dedicated parameterized tests. The PR description clearly explains the motivation and scope. No outstanding reviewer comments exist.
There was a problem hiding this comment.
LGTM — clean test rewrite with good coverage, and the Windows drive-letter fix follows the existing pattern from _parse_file_path.
Extended reasoning...
Overview
This PR rewrites the mcp.cli.claude config tests, moving them from tests/client/test_config.py to tests/cli/test_claude.py to mirror the source tree. It replaces 2 flaky subprocess-based tests (which depended on uv being on PATH and had a 20-second timeout) with 11 hermetic unit tests that directly assert the JSON config output. The source changes in claude.py are limited to removing 7 stale # pragma: no cover annotations and adding a Windows drive-letter guard that mirrors the existing pattern in _parse_file_path.
Security risks
None. The production code changes are minimal — removing pragma comments and adding a defensive check for Windows path parsing. No auth, crypto, or permissions code is touched.
Level of scrutiny
Low scrutiny is appropriate. This is primarily a test improvement PR. The only logic change in production code is the Windows drive-letter fix, which applies an identical pattern already established elsewhere in the same file (cli.py:97-102). The fix was identified in a previous review round and has been addressed with corresponding test coverage.
Other factors
- The pre-existing Windows drive-letter bug I flagged in my previous review has been fixed and the inline comment is resolved.
- Test coverage went from 2 tests to 11, covering previously untested branches (env var merging, package deduplication, editable installs, missing config dir,
get_uv_pathfallback). - The tests are well-structured with proper mocking and clear assertions.
- No CODEOWNERS concerns — this touches test infrastructure and a utility module.
2 participants
Motivation
test_command_executionintests/client/test_config.pyshelled out touv run --frozen --with mcp[cli] mcp run <path> --helpwith a 20-second timeout to verify the generated config was "executable". This was:uvbeing on PATH on the test host--helponly provesuvaccepts--help; it doesn't validate the command would actually launch an MCP server (the test passed even thoughtest_server.pydidn't exist)update_claude_configis a config-file-writing function. Its contract is producing a correct JSON blob. That's directly testable without any subprocess.Changes
command+argsJSON structure — replaces the subprocess smoke test with a direct assertion on what the function actually producesget_uv_pathin the fixture — tests no longer depend on the host'suvinstallwith_packages(sorted/deduplicated),with_editable,env_vars, env-var merge on reinstall, file specs without:objectsuffix, preserving othermcpServersentries,get_uv_pathfallback behavior# pragma: no coverannotations fromsrc/mcp/cli/claude.py— these branches are now actually testedtests/client/test_config.py→tests/cli/test_claude.pyto mirror the source tree (the file testsmcp.cli.claude, not anything inmcp.client)2 tests → 11 tests, no subprocess, 100% branch coverage on
src/mcp/cli/claude.py.