[Test Improver] test: add unit tests for deps list, tree, and info subcommands#532
Conversation
20 tests covering the previously untested list, tree, and info CLI subcommands in src/apm_cli/commands/deps/cli.py: - TestDepsListCommand (7): no apm_modules, package shown in text fallback, --global flag, --all flag (both scopes), empty apm_modules, orphaned-package warning, version display - TestDepsTreeCommand (5): no apm_modules fallback, project name from apm.yml, --global flag, tree with lockfile data, package with no lockfile - TestDepsInfoCommand (8): missing apm_modules, package not found, package details (name/version/description/author), short-name resolution, available-packages hint on error, skill-only package, no-context-files message, no-workflows message Uses a _force_rich_fallback() helper that removes rich.* modules from sys.modules to exercise the click.echo text paths testable via CliRunner captured stdout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds missing unit test coverage for the user-facing apm deps list, apm deps tree, and apm deps info subcommands, aligning these commands with existing test coverage for other deps subcommands.
Changes:
- Added a new unit test module covering
deps list,deps tree, anddeps infobehaviors across project/global scope routing and fallback (no-Rich) rendering paths. - Added coverage for common edge cases (missing
apm_modules/, orphan detection, lockfile-backed tree rendering, short-name resolution, and missing context/workflow outputs).
| def _force_rich_fallback(): | ||
| """Context-manager patches that force the text-only code path. | ||
|
|
||
| Rich imports inside function bodies are resolved from ``sys.modules`` at | ||
| call time, so we stub out the modules there instead of the per-attribute | ||
| path used when the symbols are in a module-level namespace. | ||
| """ | ||
| import contextlib | ||
|
|
||
| @contextlib.contextmanager | ||
| def _ctx(): | ||
| fakes = { | ||
| "rich": None, | ||
| "rich.console": None, | ||
| "rich.table": None, | ||
| "rich.tree": None, | ||
| "rich.panel": None, | ||
| "rich.text": None, | ||
| } | ||
| # Stash originals (None if not imported yet) | ||
| originals = {k: sys.modules.get(k) for k in fakes} | ||
| # Mark each as failed import by removing from sys.modules so the | ||
| # ``from rich.xxx import Yyy`` inside function bodies raises ImportError | ||
| for k in fakes: | ||
| sys.modules.pop(k, None) | ||
| # Now install a sentinel module that raises on attribute access | ||
| sentinel = MagicMock() | ||
| sentinel.__path__ = [] # make it look like a package | ||
|
|
||
| class _BrokenModule: | ||
| def __getattr__(self, name): | ||
| raise ImportError(f"rich not available in test") | ||
|
|
||
| broken = _BrokenModule() | ||
| broken.__path__ = [] | ||
| for k in fakes: | ||
| sys.modules[k] = broken # type: ignore[assignment] |
There was a problem hiding this comment.
_force_rich_fallback() injects instances of a custom class into sys.modules for "rich" and submodules. Importlib typically expects real module objects (types.ModuleType) with spec/loader semantics; using non-modules can lead to brittle behavior if other code does import rich (vs from rich.x import y) or if future Python/importlib behavior changes. Consider creating ModuleType stubs (and optionally making import rich itself fail) and remove the unused sentinel variable to keep the helper minimal and predictable.
| def setup_method(self): | ||
| self.runner = CliRunner() | ||
| try: | ||
| self.original_dir = os.getcwd() | ||
| except FileNotFoundError: | ||
| self.original_dir = str(Path(__file__).parent.parent.parent) | ||
| os.chdir(self.original_dir) | ||
|
|
||
| def teardown_method(self): | ||
| try: | ||
| os.chdir(self.original_dir) | ||
| except (FileNotFoundError, OSError): | ||
| repo_root = Path(__file__).parent.parent.parent | ||
| os.chdir(str(repo_root)) | ||
|
|
||
| @contextlib.contextmanager | ||
| def _chdir_tmp(self): | ||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| try: | ||
| os.chdir(tmp_dir) | ||
| yield Path(tmp_dir) | ||
| finally: | ||
| os.chdir(self.original_dir) | ||
|
|
There was a problem hiding this comment.
This test module repeats identical setup_method/teardown_method and _chdir_tmp helpers across three classes. Consolidating into a shared base class or (preferably) a pytest fixture would reduce duplication and make future updates (e.g., CWD handling) less error-prone.
|
@copilot address pr review comments |
…te test base class Agent-Logs-Url: https://github.com/microsoft/apm/sessions/219ed7d3-f6bf-48a0-966c-4185448421f1 Co-authored-by: danielmeppiel <51440732+danielmeppiel@users.noreply.github.com>
Both review comments addressed in commit
All 20 tests still pass. |
Description
Adds missing unit test coverage for the
apm deps list,apm deps tree, andapm deps infosubcommands. Thecleanandupdatesubcommands already had dedicated test files — this PR fills the gap for the remaining three.File added:
tests/unit/test_deps_list_tree_info.py— 20 tests across 3 classes.TestDepsListCommand (7 tests)
apm_modules/directory → "No APM dependencies installed" messageorg/reposhown in output--globalflag routes to user scope (InstallScope.USER)--allflag calls both project and user scopesapm_modules/→ "no valid packages" messageapm.yml) → orphan warning shownapm.ymlappears in outputTestDepsTreeCommand (5 tests)
apm_modules/+ no Rich → "No dependencies installed" in text outputapm.ymlshown--globalflag routes to user scopedep_key@versionshown in text treeapm.ymlroot nodeTestDepsInfoCommand (8 tests)
apm_modules/→ exit code 1shortrepofindsshortorg/shortrepo)Infrastructure
All imports in
deps/cli.pyare lazy (inside function bodies), so the tests use:patch("apm_cli.core.scope.get_apm_dir", ...)for scope routing_force_rich_fallback()helper usingtypes.ModuleTypestubs (with__getattr__raisingImportError) to exercise theclick.echotext paths captured byCliRunner_DepsCmdBaseclass providingsetup_method,teardown_method, and_chdir_tmp— inherited by all three test classes to eliminate duplicationType of change
Testing