feat(mcp): allow the test tool to target specific paths - #1676
feat(mcp): allow the test tool to target specific paths#1676HadiHassan22 wants to merge 5 commits into
test tool to target specific paths#1676Conversation
The `test` tool exposed no way to run a subset of a suite, so every call ran everything. The CLI already supports positional test targets — the test command forwards `argResults.rest` to the runner — but the MCP tool had no argument that reached them, and `directory` is deliberately applied as the working directory rather than as a target. Adds an optional `paths` array that is appended after every option, so the args land in `rest`. Behaviour matches the CLI, including the existing rule that targeting specific files disables test optimization.
Some points to raise on this PR:
|
|
@HadiHassan22 thanks for the contribution, its a good catch! @jmadren left really nice notes can you check those? (thank you for the review A LOT) |
`paths` was described as relative to "the project root", which reads as though a package path belongs in it. That lands on the VeryGoodOpenSource#1599/VeryGoodOpenSource#1600 failure: from a monorepo root with no pubspec.yaml, targeting packages/a/test/a_test.dart exits 66. The schema now says paths are targets inside the package selected by `directory`, and that a package path belongs in `directory` instead. `recursive` and `paths` were also incoherent together. TestCLIRunner spawns one run per package with that package as the working directory and forwards the same positional targets to each, so a relative path resolves in one package and fails to load in the rest. From a shell the cause is visible; through the tool it is not, so the pair is now rejected up front with a message naming the fix. Targets are emitted behind a `--` terminator so a path beginning with `-` is no longer a UsageException. The parser strips the `--` back out of `rest`, so nothing downstream changes. Also documents `paths` in doc/mcp.md, whose example enumerates every argument.
A recursive run executes once per package with that package as the working directory and forwards the same positional targets to each, so a relative path resolves in one package and fails to load in the rest. Both `very_good test` and `very_good dart test` now exit with a usage code and an explanation. The check sits beside the existing pubspec.yaml guard in each command, where `argResults.rest` is unambiguous. It cannot live in `TestCLIRunner.test`, which receives targets already merged into `arguments` alongside option values whose values do not start with `-`. This replaces the equivalent guard in the MCP test handler, which goes back to building argv and nothing else. The tool schema still documents the exclusion, since that is what an agent reads before making a call. Refs VeryGoodOpenSource#1704
|
@jmadren Thank you for taking the time to review my PR. Here's how I addressed your points:
On point 2: Passing the pair through applies the same relative path in every package, so it resolves in one and fails to load in the rest leading to N-1 spurious failures around one success. The combination also has no coherent meaning: My first pass guarded it in One placement note, since it isn't the obvious spot: the check can't live in The schema still documents the exclusion. That's what an agent reads before calling, rather than after a run fails. Happy to trim this back to a doc note if you'd rather keep the PR purely MCP, in which case the CLI fix moves to #1704 on its own. |
4 participants
Description
Closes #1675.
The
testMCP tool had no way to run a subset of a suite, so every call ran the whole thing. There was no workaround through the existing arguments:directoryis deliberately applied as the working directory rather than as a target (per the existingNOTEin_parseTest), andtags/exclude_tagsonly filter on annotations already present in the test source.The CLI has no such gap —
very_good test test/foo_test.dartworks today, because the test commands keepargResults.restand forward it to the runner. This just exposes that through MCP.Changes
pathsargument to thetesttool: a list of strings, each a test file or directory._parseTestappends them after every option, so they are parsed asrestrather than consumed as the value of a preceding option.very_good testandvery_good dart test; omitting the argument leaves behaviour exactly as before.The argument description also notes that targeting specific paths disables the test optimization step. That is pre-existing CLI behaviour via
TestCLIRunner.isTargettingTestFiles, not something this PR changes — it just makes it discoverable to a caller who can no longer see the command line.Why this matters for MCP callers
An agent driving the CLI through MCP was strictly less capable than one shelling out. Beyond the wasted wall-clock of running everything to check one directory, the tool returns the runner's full output, so a suite whose failure produces a large widget-tree or stack dump can return well over 100k characters — nearly all of it irrelevant to the tests the caller cared about. Narrowing the run is the cheapest mitigation available.
Testing
Three tests added to
test/src/mcp/mcp_server_test.dart, covering both branches of the new code:dart+concurrency)Verified locally:
dart format lib test— no changesdart analyze --fatal-infos --fatal-warnings lib test— no issuesvery_good dart test -x pull-request-only— 516 passingOpen questions
Happy to change the argument name (
pathsvstargetsvstest_paths) or its shape — I used a string array, thoughpackages_get'signoreuses a comma-separated string, so let me know if you'd rather stay consistent with that.