[Test Improver] test: add unit tests for mcp command (0% -> ~85%)

[danielmeppiel]

Description

src/apm_cli/commands/mcp.py (379 lines) had zero dedicated unit tests. It implements three user-facing CLI commands (search, show, list) that hit the MCP registry. Each command has two execution paths: a Rich console path and a plain-text fallback path. Testing both paths guards against regressions in either rendering mode.

Added tests/unit/test_mcp_command.py with 25 tests covering:

Area	Tests
mcp search - Rich path with results	4 tests
mcp search - fallback (no Rich) + error handling	4 tests
mcp show - Rich path (success, not found, edge cases)	6 tests
mcp show - fallback + error handling	3 tests
mcp list - Rich path + fallback + error handling	6 tests
mcp --help / group	1 test
Total	25 tests

All tests mock RegistryIntegration to avoid live network calls. The RegistryIntegration and rich.table.Table are imported lazily inside function bodies, so they are patched at their canonical module locations (apm_cli.registry.integration.RegistryIntegration and rich.table.Table).

Following review feedback, the tests were also improved:

• Removed unused imports (sys, pytest, search, show, mcp_list)
• Fixed no-op assertions in test_search_rich_no_results and test_list_rich_empty_registry that were always true due to or result.exit_code == 0
• Strengthened test_search_limit_respected and test_list_limit_option to assert the exact add_row call count and verify no server names beyond the limit appear in rendered rows
• Strengthened test_show_long_package_name_truncated to assert the package-name argument passed to add_row is ≤ 25 chars and ends with "..."

Type of change

• Bug fix
• New feature
• Documentation
• Maintenance / refactor

Testing

• Tested locally
• All existing tests pass
• Added tests for new functionality (if applicable)

All 3448 unit tests pass:

uv run python -m pytest tests/unit tests/test_console.py -x -q
3448 passed in 20.75s

[Copilot]

Pull request overview

Adds dedicated unit test coverage for the apm mcp command group (search, show, list) to protect both Rich-rendered and plain-text fallback execution paths without hitting the live MCP registry.

Changes:

• Introduces a new tests/unit/test_mcp_command.py test module covering mcp search/show/list success and failure flows.
• Mocks RegistryIntegration and Rich Table usage to keep tests fast and deterministic.
• Exercises both Rich console and non-Rich fallback behavior via _get_console patching.

Comments suppressed due to low confidence (2)

tests/unit/test_mcp_command.py:338

• Same issue as above: assert "No MCP servers" in printed or result.exit_code == 0 is always true after assert result.exit_code == 0, so the test does not actually validate the warning output. Please make the assertion strict (or assert against specific print() calls).

        assert result.exit_code == 0
        printed = " ".join(str(c) for c in mock_console.print.call_args_list)
        assert "No MCP servers" in printed or result.exit_code == 0

tests/unit/test_mcp_command.py:375

• test_list_limit_option only asserts exit_code == 0, so it doesn't verify that --limit is actually respected. Consider asserting the number of rows added to the rich table or checking that output contains only the first N entries in the fallback path.

    def test_list_limit_option(self):
        """--limit restricts number of servers displayed."""
        runner = make_runner()
        many_servers = [
            {"name": f"s{i}", "description": f"Srv {i}", "version": "1.0"}
            for i in range(30)
        ]
        mock_console = MagicMock()

        with patch_registry(list_result=many_servers), \
             patch("apm_cli.commands.mcp._get_console", return_value=mock_console), \
             patch("rich.table.Table", MagicMock()):
            result = runner.invoke(mcp, ["list", "--limit", "5"])

        assert result.exit_code == 0

[Copilot]

The assertion is effectively a no-op: ... or result.exit_code == 0 will always be true because the test already asserts exit_code == 0 above. This test should fail if the expected "No MCP servers found" message is not printed (e.g., assert the substring without the or, or assert on specific mock_console.print call args).

This issue also appears on line 336 of the same file.

Suggested change

	assert "No MCP servers found" in printed or result.exit_code == 0
	assert "No MCP servers found" in printed

[Copilot]

There are several unused imports here (sys, pytest, and the imported command functions search, show, mcp_list are never referenced). Removing them will keep the test module clean and avoid failing any lint/formatting checks that enforce unused-import rules.

Suggested change

	import sys
	from unittest.mock import MagicMock, patch
	import pytest
	from click.testing import CliRunner
	from apm_cli.commands.mcp import mcp, search, show
	from apm_cli.commands.mcp import list as mcp_list
	from unittest.mock import MagicMock, patch
	from click.testing import CliRunner
	from apm_cli.commands.mcp import mcp

[Copilot]

test_search_limit_respected doesn't currently assert that --limit changes the rendered output/rows (it only checks exit_code == 0). To actually guard against regressions, assert on the number of rows added to the table (e.g., Table().add_row call count) or that only the first N server names appear.

This issue also appears on line 361 of the same file.

Suggested change

	with patch_registry(search_result=many_servers), \
	patch("apm_cli.commands.mcp._get_console", return_value=mock_console), \
	patch("rich.table.Table", MagicMock()):
	result = runner.invoke(mcp, ["search", "server", "--limit", "3"])
	assert result.exit_code == 0
	table_instance = MagicMock()
	table_factory = MagicMock(return_value=table_instance)
	with patch_registry(search_result=many_servers), \
	patch("apm_cli.commands.mcp._get_console", return_value=mock_console), \
	patch("rich.table.Table", table_factory):
	result = runner.invoke(mcp, ["search", "server", "--limit", "3"])
	assert result.exit_code == 0
	# Ensure a table was created and only the limited number of rows were added.
	table_factory.assert_called_once()
	row_calls = table_instance.add_row.call_args_list
	assert len(row_calls) == 3
	# Verify that no rows reference servers beyond the requested limit.
	for call in row_calls:
	call_str = " ".join(str(arg) for arg in call.args)
	for i in range(3, 20):
	assert f"server-{i}" not in call_str

[Copilot]

test_show_long_package_name_truncated doesn't assert the truncation behavior described in its docstring; it currently only checks exit_code == 0. If this test is meant to protect the len(pkg_name) > 25 truncation logic, assert that a Table.add_row(...) call includes the shortened package name (and consider returning distinct mocks per Table() call to make the assertion unambiguous).

Suggested change

	assert result.exit_code == 0
	assert result.exit_code == 0
	# Verify that the long package name was truncated before being added to the table.
	assert mock_table.add_row.called
	truncated_names = []
	for call in mock_table.add_row.call_args_list:
	for arg in call.args:
	if isinstance(arg, str) and set(arg) == {"a"}:
	truncated_names.append(arg)
	# We expect at least one truncated name in the table rows.
	assert truncated_names, "Expected truncated package name in table rows"
	# The truncated name should be shorter than the original (30) and not exceed 25 characters.
	for name in truncated_names:
	assert len(name) < 30
	assert len(name) <= 25

[danielmeppiel]

@copilot address pr review comments

[Copilot]

@copilot address pr review comments

Addressed all four review comments in commit 3a10545:

• Unused imports — removed sys, pytest, search, show, and mcp_list from the import block.
• No-op assertions (lines 96 and 336) — removed the or result.exit_code == 0 tautology; each now asserts the expected string is actually present in the console output.
• test_search_limit_respected — uses a dedicated table_instance mock, asserts add_row was called exactly 3 times, and verifies no server names beyond the limit appear in the rows.
• test_list_limit_option — same pattern: asserts exactly 5 add_row calls and confirms server names beyond index 5 are absent.
• test_show_long_package_name_truncated — identifies the 4-argument pkg_table.add_row call, then asserts the package-name argument is ≤ 25 chars and ends with "...".

All 25 tests (and 3448 total unit tests) pass.