Uh oh!
There was an error while loading. Please reload this page.
Add passive command-line option defaults - #10895
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f86614b5-b40f-415d-8f94-a3290fbe7154
🧵 Parallel-safety audit — PR #10895Parallelization — assemblies touched by this PR's changed test files:
No Findings: A (global-state) I reviewed every test added/modified by this PR across the three changed test files:
No process-global mutation (env vars, CWD, culture, console), no shared/relative filesystem paths, and no Advisory only — heuristic, non-blocking. Re-run with
|
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.
| # | Dimension | Verdict |
|---|---|---|
| 5 | Performance & Allocations | 🟡 1 NIT |
| 15 | Code Structure & Simplification | 🟡 1 NIT |
✅ 20/22 dimensions clean.
Summary: This is a well-structured, thorough PR that adds passive commandLineOptionDefaults support to testconfig.json with clean separation between active options and defaults. The design is sound — defaults never activate features, explicit values take precedence, and the MSBuild integration via TestingPlatformCommandLineOptionDefault items is ergonomic.
Key positives:
- Public API surface is minimal (one extension method + one internal interface)
PublicAPI.Unshipped.txtandInternalAPI.Unshipped.txtare both updated correctly- The
#if NETCOREAPP/ Jsonite split keeps the netstandard2.0 MSBuild task self-contained - Validation (unknown options, arity, bootstrap-only, per-arg) is consistently extended to the new section
- Test coverage is comprehensive: unit tests for merge logic, defaults-don't-activate, explicit-wins, error cases; plus acceptance tests for both MSBuild generation and TRX runtime behavior
- Localization resources and xlf files are properly updated
- JSON schema updated to match
Minor observations (inline):
- The MSBuild
Inputssentinels for option defaults may disable incremental build for this target — worth monitoring CommandLineOptionsProxydelegates to the extension method which re-checks the interface cast (functionally correct, slightly indirect)
| bool ICommandLineOptionsWithDefaults.TryGetOptionArgumentListOrDefault(string optionName, [NotNullWhen(true)] out string[]? arguments) | ||
| => _commandLineOptions is null | ||
| ? throw new InvalidOperationException(Resources.PlatformResources.CommandLineOptionsNotReady) |
There was a problem hiding this comment.
Dimension 15 – Code Structure (NIT):_commandLineOptions.TryGetOptionArgumentListOrDefault(...) resolves to the extension method on ICommandLineOptions, which will then check whether the target is ICommandLineOptionsWithDefaults again. It works correctly (the proxy delegates to the inner object which is typically CommandLineHandler), but the round-trip through the extension method is a small indirection. Calling the interface method directly would be slightly clearer:
boolICommandLineOptionsWithDefaults.TryGetOptionArgumentListOrDefault(stringoptionName,[NotNullWhen(true)]outstring[]?arguments)=>_commandLineOptionsisnull?thrownewInvalidOperationException(Resources.PlatformResources.CommandLineOptionsNotReady):_commandLineOptionsisICommandLineOptionsWithDefaultswithDefaults?withDefaults.TryGetOptionArgumentListOrDefault(optionName,outarguments):_commandLineOptions.TryGetOptionArgumentList(optionName,outarguments);Not blocking — the current code is functionally correct.
| Condition=" '$(GenerateTestingPlatformConfigurationFile)' == 'true' And (Exists('$(_TestingPlatformConfigurationFileSourcePath)') Or '@(TestingPlatformCommandLineOptionDefault)' != '') " > | ||
| <ConfigurationFileTask | ||
| MSBuildProjectDirectory="$(MSBuildProjectDirectory)" | ||
| AssemblyName="$(AssemblyName)" |
There was a problem hiding this comment.
Dimension 5 – Performance (MODERATE): The sentinel Inputs items (__MTP_OPTION_DEFAULT__%(Identity)=%(Value)) are not real files, so MSBuild will always consider them non-existent and mark the target out-of-date on every build when any TestingPlatformCommandLineOptionDefault items are defined. This effectively disables incremental build for this target.
$(MSBuildAllProjects) already covers the case where a .props/.targets/.csproj file changes the item values, so the sentinels primarily address CLI-property overrides (/p:TrxFileName=x). One alternative to avoid the always-dirty state is to write a small stamp file alongside the output whose content is the hash/concatenation of the option default values, and use that as both Input and Output. Not blocking, but worth considering if build-perf feedback comes in.
| /// <see langword="true"/>. Extensions should use this method only after determining that the feature | ||
| /// which owns the option is enabled. | ||
| /// </remarks> | ||
| public static bool TryGetOptionArgumentListOrDefault( | ||
| this ICommandLineOptions commandLineOptions, |
There was a problem hiding this comment.
Dimension 4 – Public API (NIT): The new CommandLineOptionsExtensions class is appropriately declared in PublicAPI.Unshipped.txt. One thing to note: the ArgumentNullException on commandLineOptions is good, but the optionName parameter is passed straight through without a null guard. The existing TryGetOptionArgumentList may or may not guard it internally. If consistency with the existing API is intentional (letting the downstream method throw), this is fine — just flagging for awareness.
| } | ||
| entry.Values.Add(item.GetMetadata("Value")); | ||
| valuesByOption[optionName] = entry; | ||
| } |
There was a problem hiding this comment.
Dimension 6 – Cross-TFM Compatibility (NIT): The #if NETCOREAPP / #else split for System.Text.Json vs Jsonite is correctly applied. Good approach keeping the netstandard2.0 MSBuild task self-contained without a runtime JSON package dependency.
🧪 Expert test review — PR #10895
All reviewed tests directly exercise the passive command-line option defaults feature added in this PR (MSBuild This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with
|
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 | src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets — These transformed values are not real files, so MSBuild cannot use their timestamps as stable… |
| src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/ConfigurationFileTask.cs — This shipped fallback is not as strict as the System.Text.Json branch. The package always loads the… |
| src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationProvider.CommandLineOptions.cs — Reusing this enumerator also reuses its permissive empty-container behavior: empty arrays are… |
What changed in this PR
Adds passive command-line option defaults across MTP configuration, MSBuild integration, and report filename handling, addressing #6648.
Changes:
- Adds validated defaults with explicit-option precedence.
- Projects MSBuild items into generated
testconfig.json. - Applies defaults to report filenames and adds documentation/tests.
| File | Description |
|---|---|
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/JsonCommandLineOptionsTests.cs | Tests parsing and validation. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/CommandLineHandlerTests.cs | Tests passive lookup and precedence. |
test/UnitTests/Microsoft.Testing.Platform.MSBuild.UnitTests/MSBuildTests.cs | Tests configuration merging. |
test/UnitTests/Microsoft.Testing.Platform.MSBuild.UnitTests/InvokeTestingPlatformTaskTests.cs | Updates filesystem stub. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TrxTests.cs | Tests TRX defaults and precedence. |
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/MSBuildTests.ConfigurationFile.cs | Tests MSBuild-generated defaults. |
src/Platform/SharedExtensionHelpers/ReportEngineBase.cs | Applies defaults to shared reporters. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf | Updates Traditional Chinese resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf | Updates Simplified Chinese resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf | Updates Turkish resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf | Updates Russian resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf | Updates Portuguese resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf | Updates Polish resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf | Updates Korean resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf | Updates Japanese resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf | Updates Italian resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf | Updates French resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf | Updates Spanish resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf | Updates German resources. |
src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf | Updates Czech resources. |
src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx | Adds defaults-specific diagnostics. |
src/Platform/Microsoft.Testing.Platform/PublicAPI/PublicAPI.Unshipped.txt | Tracks the new public extension. |
src/Platform/Microsoft.Testing.Platform/InternalAPI/InternalAPI.Unshipped.txt | Tracks internal API changes. |
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.CommonServices.cs | Loads and validates defaults. |
src/Platform/Microsoft.Testing.Platform/Configurations/PlatformConfigurationConstants.cs | Defines the defaults section name. |
src/Platform/Microsoft.Testing.Platform/Configurations/JsonConfigurationProvider.CommandLineOptions.cs | Parses default entries. |
src/Platform/Microsoft.Testing.Platform/Configurations/JsonCommandLineOptionEntry.cs | Generalizes entry documentation. |
src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationExtensions.cs | Implements default lookup and precedence. |
src/Platform/Microsoft.Testing.Platform/Configurations/AggregatedConfiguration.cs | Resolves defaults across providers. |
src/Platform/Microsoft.Testing.Platform/CommandLine/ICommandLineOptions.cs | Exposes passive default lookup. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.UnknownAndBootstrapValidation.cs | Validates unknown/bootstrap defaults. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs | Integrates default validation. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.ArityValidation.cs | Validates default arity. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.ArgumentAndConfigurationValidation.cs | Validates default arguments. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsProxy.cs | Forwards default lookup. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineHandler.cs | Implements runtime default lookup. |
src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/ConfigurationFileTask.cs | Merges defaults into JSON. |
src/Platform/Microsoft.Testing.Platform.MSBuild/PACKAGE.md | Documents authoring surfaces. |
src/Platform/Microsoft.Testing.Platform.MSBuild/Microsoft.Testing.Platform.MSBuild.csproj | Embeds Jsonite for down-level tasks. |
src/Platform/Microsoft.Testing.Platform.MSBuild/IFileSystem.cs | Adds configuration-file reading. |
src/Platform/Microsoft.Testing.Platform.MSBuild/FileSystem.cs | Implements file reading. |
src/Platform/Microsoft.Testing.Platform.MSBuild/buildMultiTargeting/Microsoft.Testing.Platform.MSBuild.targets | Passes defaults into generation. |
src/Platform/Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.cs | Applies passive TRX filename defaults. |
docs/testconfig.schema.md | Documents schema coverage. |
docs/testconfig.schema.json | Defines defaults schema. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <Target Name="_GenerateTestingPlatformConfigurationFileCore" | ||
| Inputs="$(_TestingPlatformConfigurationFileSourcePath)" | ||
| Inputs="@(_TestingPlatformConfigurationFileInput);@(TestingPlatformCommandLineOptionDefault->'__MTP_OPTION_DEFAULT__%(Identity)=%(Value)')" |
| else if (Json.Deserialize( | ||
| source, | ||
| new JsonSettings | ||
| { | ||
| AllowComments = true, |
| internal IReadOnlyList<JsonCommandLineOptionEntry> EnumerateCommandLineOptionDefaults() | ||
| => EnumerateCommandLineOptionEntries(PlatformConfigurationConstants.CommandLineOptionDefaultsSectionName, allowBooleanMarkers: false); |
Summary
commandLineOptionDefaultssupport totestconfig.jsonTryGetOptionArgumentListOrDefaultso extensions can consume defaults without enabling their featureTestingPlatformCommandLineOptionDefaultMSBuild items that are projected into the built module configurationValidation
Fixes#6648