[Test Improver] test: expand coverage for update command (64% -> ~95%)#657
Conversation
Add 21 new tests covering previously untested paths in update.py: - Platform helper tests: _is_windows_platform(), _get_update_installer_url(), _get_update_installer_suffix(), _get_manual_update_command() (Unix path) - _get_installer_run_command(): /bin/sh fallback, pwsh fallback, no PowerShell error - Update command logic: dev version (with/without --check), can't fetch latest, already on latest, update available with --check, installer failure (exit 1), requests not available (exit 1), network error (exit 1), temp file cleanup Co-authored-by: Copilot <[email protected]>
…307-9cbf3fb30797a8ce
There was a problem hiding this comment.
Pull request overview
This PR expands unit/integration-style test coverage for the apm update command, aiming to exercise platform helper functions and additional success/error branches in src/apm_cli/commands/update.py.
Changes:
- Added unit tests for platform detection + installer URL/suffix/runner command helpers.
- Added CLI-driven tests for dev-version behavior, version-fetch failures, already-up-to-date,
--checkbehavior, installer failures, network errors, missingrequests, and temp-file cleanup.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_update_command.py |
Adds new test classes to cover platform helper utilities and more apm update command branches via CliRunner. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 2
| def setUp(self): | ||
| self.runner = CliRunner() |
There was a problem hiding this comment.
apm update writes the installer script into get_apm_temp_dir() (env var / user config dependent). These tests invoke the command without pinning that temp dir, so they can fail on developer machines where APM_TEMP_DIR or ~/.apm/config.json points to a non-writable/non-existent location. Consider setting APM_TEMP_DIR to a temporary directory (or otherwise patching get_apm_temp_dir) in setUp so the tests are hermetic across environments.
| original_unlink = update_module.os.unlink | ||
|
|
||
| def tracking_unlink(path): | ||
| deleted_paths.append(path) |
There was a problem hiding this comment.
original_unlink is assigned but never used, and tracking_unlink does not call through to the real os.unlink. That means this test will leave the temporary installer script on disk, which can pollute the configured/system temp directory over repeated runs. Consider calling the original unlink inside the tracking wrapper (or avoid patching unlink and assert cleanup via filesystem state in an isolated temp dir).
| deleted_paths.append(path) | |
| deleted_paths.append(path) | |
| original_unlink(path) |
…307-9cbf3fb30797a8ce
…307-9cbf3fb30797a8ce
Two findings on tests/unit/test_update_command.py: - Both TestUpdateCommand and TestUpdateCommandLogic now pin APM_TEMP_DIR to a per-test tempfile.TemporaryDirectory in setUp and restore the previous value in tearDown. apm update writes its installer script via tempfile.NamedTemporaryFile(dir= get_apm_temp_dir()), so without this pin the tests inherit whatever APM_TEMP_DIR / ~/.apm/config.json point to on the developer machine and can fail or leak files into a non-hermetic location. - test_update_temp_file_cleanup_on_success now calls the real os.unlink from tracking_unlink so the temporary installer script is actually removed instead of being left on disk every run. All 24 tests in the file pass. Co-authored-by: Copilot <[email protected]>
|
Addressed both Copilot review findings in 26023f6:
All 24 tests in the file pass. |
🤖 Test Improver automated PR — an AI assistant focused on improving test quality for this repository.
Goal and Rationale
The
apm updatecommand had only 3 tests (64% coverage) covering the success-path installer logic. Many important code paths were untested:_is_windows_platform,_get_update_installer_url, etc.)/bin/shfallback on Unix,requestsnot installedThese are real user-facing code paths that can silently fail (e.g., wrong installer URL served to the wrong platform, missing PowerShell erroring without a clear message).
Approach
Added 21 new tests in two new test classes alongside the existing
TestUpdateCommand:TestUpdatePlatformHelpers: Pure unit tests for all platform-detection helper functions. No mocking of subprocess or network — just platform patches.TestUpdateCommandLogic: Integration-style tests viaCliRunnercovering the full command flow for each error/success branch.Coverage Impact
src/apm_cli/commands/update.pyTotal tests: 3790 (main) → 3811 (branch, +21)
Trade-offs
requests.get,subprocess.run, andget_version— standard patterns for CLI command testing in this repo.requestsImportError test usesbuiltins.__import__patching, which is slightly more involved but accurately tests the fallback path.Reproducibility
Test Status
All 3811 tests pass (3790 pre-existing + 21 new).