Add regression test for tool name in response file; drop outdated TODO#8447
Merged
Conversation
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix response-file expansion to recognize tool name correctly
Allow response files to provide MTP tool name
May 21, 2026
Evangelink
marked this pull request as ready for review
May 21, 2026 11:31
Member
|
/review |
Contributor
|
✅ Expert Code Review (command) completed successfully! Expert-reviewer launched in background for PR #8447. It will post the review directly. |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes MTP command-line parsing so that a response file used as the first argument (mtp @tool.rsp) can supply the tool name from within the expanded contents, aligning response-file expansion with tool-name detection behavior.
Changes:
- Expand
@response-filecontents in-place (replacing the@filetoken) to ensure the first expanded non-option token can be recognized asToolName. - Add a regression unit test validating
ToolNameextraction and option parsing when argv[0] is a response file.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineTests.cs | Adds regression coverage for tool-name detection when the first argument is a response file. |
| src/Platform/Microsoft.Testing.Platform/CommandLine/Parser.cs | Adjusts response-file expansion to replace the @file token in-place so tool-name detection can see the first expanded token. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 0
Youssef1313
reviewed
May 22, 2026
The existing @-expansion logic already lands on the first expanded token on the next loop iteration (isFirstRealArgument stays true), so the new test passes without any product change. Just remove the now-incorrect TODO. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Evangelink
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a regression test covering the case where the first argument provided to MTP is a response file whose contents include the tool name (
mtp @tool.rspwithtool.rspcontainingmytool --option1 a).While investigating, it turned out the existing
@-expansion logic already handles this correctly: the response-file branch does not flipisFirstRealArgumentand does not advance past the expanded content, so the next loop iteration naturally picks up the first expanded token as the tool name. The TODO comment onParser.csclaiming otherwise was outdated and has been removed.Changes
(31, ["@31_test.rsp"], ...)inCommandLineTests.ParserTestsData.Parser.cs.No product logic change.