test(integration): close install/uninstall/update CLI coverage gaps surfaced by #764 review#767
Merged
danielmeppiel merged 8 commits intomainfrom Apr 19, 2026
Merged
Conversation
Covers gap G1 (apm install -g real package + content verification) and U1 (apm uninstall pkg -g removes files from ~/.apm/). Existing global scope tests only validated directory plumbing and error paths; no test actually deployed a real package under user scope. Three cases: - install -g deploys microsoft/apm-sample-package; lockfile + primitive files appear under fake_home; cwd remains untouched. - uninstall -g removes lockfile entry, manifest entry, and all deployed primitive files. - Global + project installs of the same package coexist without collision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers gap G2: presentation/dry_run.py (extracted in PR #764) had zero binary-level tests. Three cases lock the contract end-to-end: - Plain --dry-run lists APM deps, prints banner/footer, makes no changes on disk (no lockfile, no apm_modules/, no .github/ artifacts). - --dry-run --only=apm correctly suppresses MCP dependency lines. - --dry-run after manifest edit previews orphan removals (Files that would be removed: N) without actually deleting anything. Guards the NameError regression on the orphan-preview path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The canonical update command had zero CLI-level integration coverage; the closest existing test (test_apm_dependencies::test_dependency_update_workflow) called GitHubPackageDownloader directly rather than the binary. Four cases: - Up1: deps update (all packages) with real ref change bumps lockfile SHA and re-deploys files (pinned commit -> main). - Up2: deps update <pkg> with two installed packages updates only the named one. - Up3: deps update -g respects user scope. Lockfile lands at ~/.apm/apm.lock.yaml; cwd remains clean (regression guard against the historical silent-deploy-to-project bug). - G3: deps update unknown-pkg exits non-zero with a clear error. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Covers gap G4: PR #764 fixed a PAT leak in install/validation.py:218-231 by piping git ls-remote stderr through ado_downloader._sanitize_git_error. Without an integration test, that fix could regress silently. Two cases inject a recognizable canary token via GITHUB_TOKEN / GITHUB_APM_PAT, run apm install --verbose against nonexistent repos, and assert the canary substring is fully absent from stdout/stderr: - 404 repo via shorthand (org/repo) - 404 repo via explicit git+https URL Allows redacted forms (***, [REDACTED]) but never the literal token. Verified the canary is fully scrubbed; no leak detected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The marketplace CLI flow had ~1604 LOC of unit coverage but zero binary-level integration tests. This adds three config-side cases plus one documented skip: - list shows seeded ~/.apm/marketplaces.json entries - add rejects invalid OWNER/REPO format before hitting network - remove clears the entry from disk and from list output - install plugin@marketplace deploy is skipped (needs a stable public marketplace.json fixture; documented in test for follow-up) Uses fake_home isolation pattern; no real network required for the three active tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two cases: - Real install + uninstall --dry-run leaves all files, manifest, and lockfile untouched while emitting the dry-run preview. - uninstall --dry-run with an unknown package emits a warning and makes no mutations (locks current behavior: exit 0 with warning, NOT failure -- engine.py:60-101). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two cases using two real public APM packages: - uninstall PKG_A PKG_B in a single command removes both from manifest, lockfile, and disk in one operation. - uninstall known unknown/pkg removes the known one and emits a warning for the unknown (exit 0, behavior locked). Packages: microsoft/apm-sample-package and github/awesome-copilot/skills/aspire (both used elsewhere in the integration suite). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Builds a real 3-level dependency chain using local-path APM deps so the test is fast and network-free: consumer -> pkg-a -> pkg-b -> pkg-c (leaf) Two cases exercise the post-#764 install/resolve walker end-to-end: - Resolution: all 3 packages materialize in apm_modules/_local/, lockfile records depth (1/2/3) and resolved_by chain, instructions deploy from every level into .github/instructions/. - Uninstall cascade: uninstall ../pkg-a removes pkg-b and pkg-c from disk, lockfile, and .github/instructions/ via _cleanup_transitive_orphans. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
danielmeppiel
force-pushed
the
tests/integration-coverage-gaps
branch
from
d1577e9 to
de02514
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds binary-level integration tests for key CLI workflows (install, uninstall, deps update, marketplace) and includes the install-engine modularization work (install pipeline engine package + service/request abstractions), plus associated unit-test updates and changelog entries.
Changes:
- Add new integration E2E tests covering global/project install, dry-run previews, multi-uninstall, deps update, marketplace config commands, transitive chains, and verbose redaction.
- Introduce/expand the
apm_cli/install/engine package (context/pipeline/phases/services/sources/template/presentation) and add direct unit tests for theInstallServicecontract + architecture invariant checks. - Update existing tests/patch points for moved symbols and add changelog entries describing the refactor/behavioral fixes.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_local_content_install.py | Update patch target for moved integration function while keeping local-content tests. |
| tests/unit/test_install_command.py | Update patch path for GitHubPackageDownloader after refactor. |
| tests/unit/integration/test_cleanup_helper.py | Point marker-based source inspection at new cleanup phase module. |
| tests/unit/install/test_service.py | New unit tests for InstallRequest immutability and InstallService delegation. |
| tests/unit/install/test_architecture_invariants.py | New invariants ensuring install engine package exists and LOC budgets stay bounded. |
| tests/unit/install/init.py | New package marker for unit tests under tests/unit/install/. |
| tests/integration/test_global_install_e2e.py | New E2E coverage for apm install -g and apm uninstall -g plus cross-scope coexistence. |
| tests/integration/test_install_dry_run_e2e.py | New E2E coverage for apm install --dry-run, including orphan preview regression guard. |
| tests/integration/test_deps_update_e2e.py | New E2E coverage for apm deps update (all, selective, -g, unknown pkg error). |
| tests/integration/test_install_verbose_redaction_e2e.py | New E2E regression guard preventing token leakage in verbose output. |
| tests/integration/test_marketplace_e2e.py | New E2E coverage for marketplace list/add validation/remove (plus one documented skip). |
| tests/integration/test_uninstall_dry_run_e2e.py | New E2E coverage for uninstall dry-run preview + no-mutation assertions. |
| tests/integration/test_uninstall_multi_e2e.py | New E2E coverage for multi-arg uninstall behavior and partial-unknown behavior. |
| tests/integration/test_transitive_chain_e2e.py | New deterministic local-path 3-level dependency chain install/uninstall coverage. |
| tests/integration/test_selective_install_mcp.py | Update patch path for downloader in existing MCP integration suite. |
| src/apm_cli/install/init.py | New install engine package entry-point docstring/overview. |
| src/apm_cli/install/context.py | New InstallContext shared state object for phase orchestration. |
| src/apm_cli/install/request.py | New frozen InstallRequest typed input record for the service layer. |
| src/apm_cli/install/service.py | New InstallService application service delegating to pipeline. |
| src/apm_cli/install/pipeline.py | New orchestrator function coordinating phases and assembling context. |
| src/apm_cli/install/services.py | New integration services (integrate_package_primitives, integrate_local_content) with compatibility aliases. |
| src/apm_cli/install/sources.py | New Strategy implementations for dependency acquisition paths (local/cached/fresh). |
| src/apm_cli/install/template.py | New shared Template Method integration flow after acquisition. |
| src/apm_cli/install/validation.py | New extracted validation helpers, including token-redaction logic for verbose probe output. |
| src/apm_cli/install/helpers/init.py | Package marker for install helpers. |
| src/apm_cli/install/helpers/security_scan.py | Extracted pre-deploy security scan wrapper used by install pipeline. |
| src/apm_cli/install/presentation/init.py | Package marker for presentation layer. |
| src/apm_cli/install/presentation/dry_run.py | Extracted dry-run preview renderer for apm install --dry-run. |
| src/apm_cli/install/phases/init.py | Package marker for install phases. |
| src/apm_cli/install/phases/resolve.py | Resolve phase: lockfile load, BFS resolution, --only expansion, intended keys. |
| src/apm_cli/install/phases/targets.py | Targets phase: resolve targets, init integrators, create auto-create dirs. |
| src/apm_cli/install/phases/download.py | Parallel pre-download phase using thread pool + Rich progress. |
| src/apm_cli/install/phases/integrate.py | Sequential integration phase using sources+template and root .apm/ integration. |
| src/apm_cli/install/phases/cleanup.py | Cleanup phase: orphan + stale-file cleanup routed through #762 chokepoint. |
| src/apm_cli/install/phases/lockfile.py | Lockfile builder + deployed-file hashing helper. |
| src/apm_cli/install/phases/post_deps_local.py | Post-deps local .apm/ stale cleanup + lockfile persistence. |
| src/apm_cli/install/phases/finalize.py | Finalize phase: verbose stats + fallback success + result assembly. |
| src/apm_cli/install/phases/local_content.py | Root .apm/ detection helpers + local-path package copy helper. |
| CHANGELOG.md | Add Unreleased entries describing install refactor and local .apm/ stale-cleanup provenance fix. |
Copilot's findings
- Files reviewed: 8/8 changed files
- Comments generated: 0
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.
Why
The risk audit during PR #764 review (the install.py modularization) surfaced a stark gap: of the four real bugs caught by review, none had any integration test coverage. They all slipped past CI and were only caught by careful human review. This PR closes the highest-leverage integration-test gaps for
install,uninstall, anddeps updateso equivalent regressions would fail CI before review.What
8 new test files, ~1,557 LOC, 22 cases (21 passing + 1 documented skip), running in ~95s:
test_global_install_e2e.pyapm install -greal-package deploy / file removal — thecallback_failuresset/dict bug atresolve.py:135had zero coveragetest_install_dry_run_e2e.pypresentation/dry_run.pyorphan-preview NameError onsetshadowingtest_deps_update_e2e.pyapm deps updateCLI was untested at the binary level; Up3 is a regression guard for the historical-gsilent-deploy-to-project bugtest_install_verbose_redaction_e2e.pyvalidation.py:218-231(sanitizer regression guard)test_marketplace_e2e.pytest_uninstall_dry_run_e2e.pyapm uninstall --dry-runpreview pathtest_uninstall_multi_e2e.pyapm uninstall pkg1 pkg2documented behavior locked intest_transitive_chain_e2e.pyHow
microsoft/apm-sample-packageandgithub/awesome-copilot/skills/aspire. No new external test dependencies.marketplace.jsonfixture; documented in the test as a follow-up.Verification
Run locally with:
Notes
refactor/install-modularization(PR refactor(install): modularize install.py into engine package #764). Once refactor(install): modularize install.py into engine package #764 merges, this PR's diff reduces to the 8 new test files.apm uninstall <unknown-pkg>exits 0 with a warning (engine.py:60-101). Arguably a UX gap; tests assert current behavior.~/(so primitives land at~/.copilot/...), while metadata is at~/.apm/. Inline-documented in the test where this matters.