Replace integration test with proper unit tests for config generation#1584
Open
Replace integration test with proper unit tests for config generation#1584
Conversation
maxisbey
force-pushed
the
fix-config-tests
branch
from
3ff4f7f to
a645364
Compare
maxisbey
force-pushed
the
fix-config-tests
branch
from
a645364 to
9480186
Compare
The test_command_execution test was an integration test disguised as a unit test. It executed external commands (uv run) which caused several problems: - Modified uv.lock file as a side effect - Required external dependencies (uv binary, network access) - Was slow and potentially flaky - Tested the wrong layer (uv CLI instead of config generation) Replaced with 15 comprehensive unit tests that properly test the update_claude_config function's actual responsibilities: - JSON structure generation - Argument array construction - Path resolution to absolute paths - Environment variable merging - Package deduplication - Server preservation and updates - Error handling Benefits: - Fast: runs in ~1.5s vs several seconds - No external dependencies or side effects - Tests the correct abstraction layer - Better coverage of edge cases - More maintainable and reliable
maxisbey
force-pushed
the
fix-config-tests
branch
from
9480186 to
9853ce7
Compare
felixweinberger
added
enhancement
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
2 participants
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.
Refactored
tests/client/test_config.pyto use proper unit testing practices instead of executing external commands.Motivation and Context
The
test_command_executiontest was executinguv runvia subprocess, which violated unit testing principles and caused several issues:uv.lockfile during test execution (without--frozenflag)uvbinary, network access, and package availabilityuvCLI behavior rather than the config generation logicThe function under test (
update_claude_config) is responsible for JSON file manipulation, not command execution. Tests should verify the generated config structure, not whether external tools work.How Has This Been Tested?
Replaced 1 integration test with 15 focused unit tests covering:
["run", "--with", "mcp[cli]", ...]):objectsuffix)All tests pass in ~1.5s with no external dependencies or side effects.
Breaking Changes
None. This is purely a test refactor with no changes to production code.
Types of changes
Checklist
Additional context
The test suite now properly isolates the unit under test using mocked file system operations. Tests verify the actual contract of
update_claude_config():If end-to-end validation of the generated commands is needed, it should be in a separate integration test suite marked with
@pytest.mark.integrationand run independently from unit tests.