Simplify TestRunParameters argument validation with LINQ (PR #9748 follow-up)#9749
Conversation
Address the code-quality review comment on #9748: replace the implicitly-filtering foreach in MSTestTestRunParametersCommandLineOptionsProvider.ValidateOptionArgumentsAsync with an explicit FirstOrDefault filter. No behavior change — still reports the first argument missing '=' with the same localized message. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to #9748 (native MTP integration, Phase 6a) that addresses a github-code-quality bot review note. It simplifies argument validation in the MSTest-native --test-parameter command-line option provider by replacing an implicit-filtering foreach loop with an explicit FirstOrDefault LINQ query. The change is confined to the experimental native MTP path (#if !WINDOWS_UWP) and has no behavioral impact.
Changes:
- Replaced the
foreach+if (!argument.Contains('='))early-return pattern witharguments.FirstOrDefault(argument => !argument.Contains('=')). - Uses a conditional expression to return
InvalidTask(with the same localizedTestRunParameterOptionArgumentIsNotParametermessage) when an invalid argument exists, otherwiseValidTask.
Show a summary per file
| File | Description |
|---|---|
| src/Adapter/MSTest.TestAdapter/TestingPlatformAdapter/MSTestTestRunParametersCommandLineOptionsProvider.cs | Refactors ValidateOptionArgumentsAsync to use FirstOrDefault instead of a filtering foreach; behavior (first-invalid-argument reporting and empty-array validity) is preserved. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
✅ 22/22 dimensions clean — no findings.
The FirstOrDefault replacement is semantically equivalent to the original foreach with early return: both find the first argument not containing '=' and report it (or return valid when all arguments pass). The lambda captures no variables, so the delegate is compiler-cacheable — no allocation concern on this cold startup path. LINQ is available via implicit usings, and the existing acceptance/unit tests cover the validation behavior end-to-end.
What
Follow-up to the merged #9748 (Phase 6a), addressing the review feedback on that PR.
Change
The
github-code-qualitybot flagged thatMSTestTestRunParametersCommandLineOptionsProvider.ValidateOptionArgumentsAsyncused aforeachthat implicitly filters its sequence. Replaced it with an explicitFirstOrDefaultfilter:No behavior change — it still reports the first argument missing
=with the same localized message, and returns valid when all arguments contain=.Not addressed here (deferred/informational)
The expert-review summary on #9748 raised two informational, non-blocking notes that are intentionally left out of this PR:
MSTEST_EXPERIMENTAL_NATIVE_MTPcasing (is "1" or "true" or "True"won't match"TRUE") — the reviewer explicitly suggested harmonizing this when the flag is promoted in Phase 6b, so it's folded into that work.MSTestRunSettingsConfigurationProvider.TryGet()re-parsing the XDocument — a cold path; caching is optional and not warranted yet.Validation
MSTestAdapter.UnitTests(45) pass.Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>