Conversation
neelay-aign
requested a review
from helmut-hoffer-von-ankershoffen
as a code owner
|
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end test fixture and a dummy MCP plugin package to validate MCP plugin auto-discovery via Python entry points in the Aignostics SDK.
Changes:
- Introduces a dummy, installable MCP plugin (entry point:
aignostics.plugins) exposing two tools (dummy_echo,dummy_add). - Adds an E2E-style test that installs the dummy plugin, clears discovery caches, and verifies server discovery + client tool round-trip.
- Adds a
.featurespec file for traceability ofTC-UTILS-MCP-01.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/resources/mcp_dummy_plugin/src/mcp_dummy_plugin/_mcp.py |
Implements dummy FastMCP server and tools used by the E2E test. |
tests/resources/mcp_dummy_plugin/src/mcp_dummy_plugin/__init__.py |
Exposes mcp for plugin import/entry-point loading. |
tests/resources/mcp_dummy_plugin/pyproject.toml |
Defines an installable test plugin with an aignostics.plugins entry point. |
tests/aignostics/utils/mcp_test.py |
Adds cache-clearing helpers, pip install/uninstall fixture, and an auto-discovery E2E test using fastmcp.Client. |
tests/aignostics/utils/TC-UTILS-MCP-01.feature |
Adds a feature spec describing the intended discovery + serving behavior. |
| subprocess.check_call( | ||
| [sys.executable, "-m", "pip", "install", "-e", str(DUMMY_PLUGIN_DIR)], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.PIPE, | ||
| ) |
There was a problem hiding this comment.
subprocess.check_call(..., stderr=subprocess.PIPE) will not surface pip’s stderr in the test failure output (CalledProcessError from check_call doesn’t include captured stderr). This makes CI failures hard to debug. Prefer subprocess.run(..., check=True, capture_output=True, text=True) and, on failure, include stderr in the exception/log output (or don’t redirect stderr/stdout).
| import site | ||
|
|
||
| subprocess.check_call( | ||
| [sys.executable, "-m", "pip", "install", "-e", str(DUMMY_PLUGIN_DIR)], |
There was a problem hiding this comment.
Installing the dummy plugin via pip inside the test can trigger dependency/build isolation installs (network access) and makes the test non-hermetic/offline-unfriendly. Consider adding --no-deps (since fastmcp is already a test dependency) and --no-build-isolation / other options to ensure this stays offline and deterministic.
| [sys.executable, "-m", "pip", "install", "-e", str(DUMMY_PLUGIN_DIR)], | |
| [ | |
| sys.executable, | |
| "-m", | |
| "pip", | |
| "install", | |
| "--no-deps", | |
| "--no-build-isolation", | |
| "-e", | |
| str(DUMMY_PLUGIN_DIR), | |
| ], |
| subprocess.check_call( | ||
| [sys.executable, "-m", "pip", "uninstall", "-y", "mcp-dummy-plugin"], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.PIPE, | ||
| ) |
There was a problem hiding this comment.
Same as install: subprocess.check_call(..., stderr=subprocess.PIPE) during uninstall hides the pip error output if uninstall fails, which will be painful to diagnose in CI. Use subprocess.run(..., check=True, capture_output=True, text=True) or let stderr through.
| _clear_mcp_discovery_caches() | ||
|
|
||
|
|
||
| @pytest.mark.e2e |
There was a problem hiding this comment.
This test installs/uninstalls a package into the active venv, which isn’t safe to run under the parallel -m (not sequential) pytest pass (see noxfile’s split between parallel/non-parallel runs). It should be marked @pytest.mark.sequential to avoid xdist concurrency issues. Also, per pyproject.toml marker docs, e2e is intended for tests involving real external network services; this looks like an offline integration test, so integration (plus sequential) would better match the repository’s conventions.
| @pytest.mark.e2e | |
| @pytest.mark.integration | |
| @pytest.mark.sequential |
| And a client connects and lists tools | ||
| Then the tool list includes "dummy_plugin_dummy_echo" and "dummy_plugin_dummy_add" |
There was a problem hiding this comment.
The scenario text says the client “connects and lists tools”, but the corresponding test (test_mcp_server_discovers_and_serves_plugin_tools) asserts tool names via server.get_tools() rather than listing tools through the client. To keep spec ↔ test traceability accurate, either update the scenario wording or update the test to list tools via the Client API (if available).
| And a client connects and lists tools | |
| Then the tool list includes "dummy_plugin_dummy_echo" and "dummy_plugin_dummy_add" | |
| And tools are listed from the server via server.get_tools() | |
| Then the returned tool list includes "dummy_plugin_dummy_echo" and "dummy_plugin_dummy_add" |
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
1 participant
No description provided.