[Test Improver] test: add unit tests for MCPIntegrator (0% -> ~75%) by danielmeppiel · Pull Request #632 · microsoft/apm

danielmeppiel · 2026-04-09T01:20:31Z

🤖 Test Improver — automated AI assistant

Goal and Rationale

MCPIntegrator (src/apm_cli/integration/mcp_integrator.py, 1395 lines) is a large, high-risk orchestrator with zero dedicated tests. It owns MCP dependency resolution, deduplication, server-info building, drift detection, runtime detection, lockfile persistence, and stale cleanup across five runtimes (VS Code, Copilot, Codex, Cursor, OpenCode).

Bugs here could silently corrupt MCP configuration files across all runtimes. This PR adds focused unit tests for every pure-logic method.

Approach

Tests target the methods that can be verified without live runtimes or network:

Class	Methods Covered
`TestIsVscodeAvailable`	`_is_vscode_available()` — PATH + dir detection
`TestDeduplicate`	`deduplicate()` — first-wins, dict/str/nameless entries, ordering
`TestGetServerNames`	`get_server_names()` — dep objects, strings, mixed
`TestGetServerConfigs`	`get_server_configs()` — serialization, plain-string fallback
`TestAppendDrifted`	`_append_drifted_to_install_list()` — sorted append, dedup
`TestDetectMcpConfigDrift`	`_detect_mcp_config_drift()` — changed/unchanged/new configs
`TestDetectRuntimes`	`_detect_runtimes()` — word-boundary regex, multi-runtime
`TestBuildSelfDefinedInfo`	`_build_self_defined_info()` — stdio, http/sse, env, args, headers, tools
`TestApplyOverlay`	`_apply_overlay()` — transport, package filter, headers, args (list+dict), tools, warning
`TestUpdateLockfile`	`update_lockfile()` — temp file writes, sorted servers, empty clear
`TestRemoveStaleVscode`	`remove_stale()` — server removal, short-name matching, runtime targeting
`TestRemoveStaleCopilot`	`remove_stale()` — copilot config cleanup
`TestCollectTransitive`	`collect_transitive()` — missing/empty dir edge cases

Coverage Impact

Metric	Before	After
Test count	3,748	3,818
New tests	—	70
`mcp_integrator.py` coverage	~0%	~75%

Test Status

3818 passed in 14.33s

All 70 new tests pass. Full unit suite passes with no regressions.

Reproducibility

# Run new tests only
uv run pytest tests/unit/integration/test_mcp_integrator.py -v

# Run full unit suite
uv run pytest tests/unit tests/test_console.py -x -q

Generated by Daily Test Improver · ◷

To install this agentic workflow, run
gh aw add githubnext/agentics/workflows/daily-test-improver.md@b87234850bf9664d198f28a02df0f937d0447295

Cover pure-logic methods: deduplicate, get_server_names, get_server_configs, _append_drifted_to_install_list, _detect_mcp_config_drift, _detect_runtimes, _build_self_defined_info, _apply_overlay, update_lockfile, remove_stale (vscode + copilot), and collect_transitive edge cases. 70 new tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…264-5b2037248fd572ad

Copilot

Pull request overview

Adds a dedicated unit-test suite for the MCPIntegrator orchestration helpers, improving confidence in MCP dependency handling (deduplication, config building, drift/runtime detection, lockfile persistence, and stale cleanup) without requiring installed runtimes or network access.

Changes:

Introduces tests/unit/integration/test_mcp_integrator.py with focused unit tests for pure-logic/static methods in MCPIntegrator.
Covers filesystem-based behaviors via temporary directories and targeted patching of Path.cwd() / Path.home().

Show a summary per file

File	Description
`tests/unit/integration/test_mcp_integrator.py`	New unit tests exercising MCPIntegrator helpers and edge cases (dedup, overlays, drift detection, runtime detection, lockfile updates, stale removal).

Copilot's findings

Files reviewed: 1/1 changed files
Comments generated: 5

Copilot · 2026-04-18T02:59:29Z

+import tempfile
+from pathlib import Path
+from unittest.mock import MagicMock, patch


Unused imports add noise in this test module (e.g., tempfile and MagicMock are never referenced). Please remove unused imports to keep the test file minimal and avoid future linting failures if/when enforced.

Suggested change

import tempfile

from pathlib import Path

from unittest.mock import MagicMock, patch

from pathlib import Path

from unittest.mock import patch

Copilot · 2026-04-18T02:59:30Z

+    def _write_vscode_mcp(self, path: Path, servers: dict) -> None:
+        path.mkdir(parents=True, exist_ok=True)
+        mcp_json = path / "mcp.json"
+        mcp_json.write_text(json.dumps({"servers": servers}), encoding="utf-8")
+        return mcp_json


Helper has a return statement but is annotated as returning None. Either drop the return value or update the annotation to return Path for accuracy.

Copilot · 2026-04-18T02:59:30Z

+        deps = [_make_dep("alpha"), _make_dep("beta"), _make_dep("alpha")]
+        result = MCPIntegrator.deduplicate(deps)
+        assert len(result) == 2


This test name suggests mixed string/object inputs, but the deps list only contains MCPDependency objects. Rename the test or adjust it to actually include a string entry so the intent matches the coverage.

Suggested change

deps = [_make_dep("alpha"), _make_dep("beta"), _make_dep("alpha")]

result = MCPIntegrator.deduplicate(deps)

assert len(result) == 2

deps = ["alpha", _make_dep("beta"), "alpha"]

result = MCPIntegrator.deduplicate(deps)

assert len(result) == 2

assert result[0] == "alpha"

assert result[1].name == "beta"

Copilot · 2026-04-18T02:59:30Z

+    def test_nameless_items_kept_by_identity(self):
+        # Items with no name should be kept (not merged)
+        dep1 = {"other": "x"}
+        dep2 = {"other": "y"}
+        result = MCPIntegrator.deduplicate([dep1, dep2])
+        assert len(result) == 2


The comment says nameless items are kept "by identity", but MCPIntegrator.deduplicate() uses dep not in result which deduplicates by equality (e.g., two distinct dicts with the same contents will be treated as duplicates). Please reword the comment to match the actual behavior being exercised.

Copilot · 2026-04-18T02:59:30Z

+    def test_stdio_env_vars_in_packages(self):
+        dep = _make_self_defined(
+            "pkg-svc",
+            transport="stdio",
+            command="pkg-svc",
+            env={"KEY": "val"},
+        )
+        info = MCPIntegrator._build_self_defined_info(dep)
+        # For stdio without raw: check packages contains env vars
+        packages = info.get("packages", [])
+        if packages:
+            env_vars = packages[0].get("environment_variables", [])
+            assert any(e["name"] == "KEY" for e in env_vars)
+
+    def test_no_tools_no_override_key(self):
+        dep = _make_self_defined("simple", transport="stdio")
+        info = MCPIntegrator._build_self_defined_info(dep)
+        assert "_apm_tools_override" not in info
+
+    def test_list_args_in_packages(self):
+        dep = _make_self_defined(
+            "args-svc",
+            transport="stdio",
+            command="args-svc",
+            args=["--arg1", "--arg2"],
+        )
+        info = MCPIntegrator._build_self_defined_info(dep)
+        packages = info.get("packages", [])
+        if packages:
+            rt_args = packages[0].get("runtime_arguments", [])
+            hints = [a["value_hint"] for a in rt_args]
+            assert "--arg1" in hints
+


These assertions are guarded by if packages:, which makes the tests pass even if _build_self_defined_info() stops emitting packages for stdio deps (a regression you likely want to catch). Consider asserting that packages is present/non-empty first, then asserting on its contents.

danielmeppiel added automation testing labels Apr 9, 2026

danielmeppiel mentioned this pull request Apr 15, 2026

[Test Improver] Monthly Activity 2026-04 #697

Closed

17 tasks

danielmeppiel marked this pull request as ready for review April 18, 2026 02:56

danielmeppiel requested a review from sergio-sisternes-epam as a code owner April 18, 2026 02:56

Copilot AI review requested due to automatic review settings April 18, 2026 02:56

Merge branch 'main' into test-assist/mcp-integrator-coverage-24166967…

ae5e481

…264-5b2037248fd572ad

Copilot started reviewing on behalf of danielmeppiel April 18, 2026 02:56 View session

Copilot AI reviewed Apr 18, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Test Improver] test: add unit tests for MCPIntegrator (0% -> ~75%)#632

[Test Improver] test: add unit tests for MCPIntegrator (0% -> ~75%)#632
danielmeppiel wants to merge 2 commits intomainfrom
test-assist/mcp-integrator-coverage-24166967264-5b2037248fd572ad

danielmeppiel commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

danielmeppiel commented Apr 9, 2026

Goal and Rationale

Approach

Coverage Impact

Test Status

Reproducibility

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants