[code-simplifier] Simplify ExecutableConditionAttribute code#9375
Conversation
- Replace ternary-throw pattern with explicit if-guard for arguments null check - Use string.IsNullOrEmpty instead of manual null-or-length check for pathExt - Use collection spread [string.Empty, ..parts] instead of Array.Copy pattern Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR performs small readability-focused refactors in ExecutableConditionAttribute (MSTest TestFramework) while preserving behavior, primarily around argument cloning and Windows PATHEXT handling.
Changes:
- Replaced a ternary-throw assignment with an explicit
arguments is nullguard and subsequent clone. - Simplified the
PATHEXTnull/empty check viastring.IsNullOrEmpty. - Simplified the executable-extension array construction using a collection expression with spread.
Show a summary per file
| File | Description |
|---|---|
| src/TestFramework/TestFramework/Attributes/TestMethod/ExecutableConditionAttribute.cs | Refactors constructor argument guarding/cloning and streamlines Windows PATHEXT parsing/extension list construction without changing behavior. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
🔍 Build Failure AnalysisSummary — The build fails due to a single Root cause: Missing space in collection-expression spread element
This is consistent with:
The error fires four times (once per target framework compiled from Affected file / error
Proposed fix - return [string.Empty, ..parts];
+ return [string.Empty, .. parts];You can also let the toolchain fix it automatically: dotnet format analyzers src/TestFramework/TestFramework/TestFramework.csproj --diagnostics IDE0055Build overview
All MSBuild errors (4 unique + 1 summary)
The same diagnostic repeats for each target framework ( 🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit 81e36e2
|
Evangelink
left a comment
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Build Failure Analysis workflow. · 775.1 AIC · ⌖ 21.7 AIC · ⊞ 46.9K · ◷
…leConditionAttribute.cs
Evangelink
left a comment
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review: all three changes are semantically equivalent — LGTM
Dimensions checked: Correctness · API Compat · Cross-TFM · Null Safety · Thread Safety · Performance · Resource Safety · PublicAPI / XLF / resx (none changed) · Code Style
Change 1 — Null guard refactor (lines 123–133)
_arguments = arguments is null ? throw ... : ... → if (arguments is null) { throw ... } _arguments = ...
✅ Equivalent. Both branches throw ArgumentNullException when arguments is null; the assignment only runs when arguments is not null. No observable difference in semantics, exception type, or stack frame.
Change 2 — string.IsNullOrEmpty (line 331)
pathExt is null || pathExt.Length == 0 → string.IsNullOrEmpty(pathExt)
✅ Equivalent. string.IsNullOrEmpty carries [return: NotNullWhen(false)], so the Roslyn null-flow analysis after the guard is identical to the explicit pattern — pathExt is known non-null in the remainder of the method in both forms.
Change 3 — Collection spread [string.Empty, .. parts] (lines 338–343)
Manual new string[parts.Length + 1] / result[0] / Array.Copy → return [string.Empty, .. parts]
✅ Equivalent and cross-TFM safe. Collection expressions are a compiler-lowering feature: Roslyn emits the exact same array-allocation + Array.Copy IL when targeting net462 or netstandard2.0. No BCL type unavailable on those targets is required. The project already uses [string.Empty, ".exe", ...] collection expressions in the early-return path of the same method, confirming the pattern compiles correctly across all TFMs. LangVersion preview is confirmed in Directory.Build.props (<LangVersion>preview</LangVersion>).
Code Simplification — 2026-06-23
This PR simplifies
ExecutableConditionAttribute, added in #9369, to improve clarity and consistency while preserving all functionality.Files Simplified
src/TestFramework/TestFramework/Attributes/TestMethod/ExecutableConditionAttribute.cs— three targeted cleanupsImprovements Made
Replaced ternary-throw with explicit null guard
The original ternary-throw pattern is syntactically valid but reads in an unusual order (the "throw" branch precedes the normal path):
The explicit guard-then-assign form is more readable and consistent with patterns used elsewhere in the file (e.g. the
if (process is null) return false;guard inTryRunWithExitCodeZero).Used
string.IsNullOrEmptyfor combined null+empty checkstring.IsNullOrEmptyis the canonical idiom already used throughout the codebase (e.g.CollectionAssert.Subset.cs,StructuredAssertionMessage.cs,Ensure.cs).Replaced manual array construction with collection spread
The spread syntax is already used in the codebase (e.g.
MSTestAdapter.PlatformServices) and generates identical IL. WithLangVersion previewset project-wide, this is fully supported.Changes Based On
efa7e0d— merged 2026-06-23Testing
ExecutableConditionAttributeTestscover all affected code pathsReview Focus
Please verify:
Add this agentic workflows to your repo
To install this agentic workflow, run