[Prototype] Option C: read CLI options from testconfig.json via IConfiguration (#6349)#8664
Draft
Evangelink wants to merge 3 commits into
Draft
[Prototype] Option C: read CLI options from testconfig.json via IConfiguration (#6349)#8664Evangelink wants to merge 3 commits into
Evangelink wants to merge 3 commits into
Conversation
…rosoft#6349) Introduces a CLI-backed IConfigurationSource (Order=0) so values parsed from the command line, env vars, and testconfig.json all flow through the same IConfiguration. CommandLineHandler becomes a facade over IConfiguration when one is supplied so every existing ICommandLineOptions consumer transparently sees JSON-sourced options. This is a prototype to highlight rough edges: - CommandLineOptionsValidator still walks parseResult.Options only (arity / per-option / unknown-option detection skips JSON-only options); TODOs added in place. - Bootstrap-time readers (TestApplication diagnostic plumbing, --config-file, --no-banner) read parseResult before IConfiguration is built. - IConfiguration only exposes string?; multi-value reads walk indexed keys (commandLineOptions:<name>:<index>) which the JSON parser already produces for arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cation Review-round fixes: - Add AggregatedConfiguration.TryGetCommandLineOptionFromProviders that resolves command-line options at the option granularity by walking providers in registration order. The first provider with any data for the option wins outright, so a CLI zero-arity flag is no longer silently merged with JSON indexed arguments. - Route ConfigurationExtensions.IsCommandLineOptionSet / TryGetCommandLineOptionArguments through the new provider-aware path when the IConfiguration is an AggregatedConfiguration; keep the merged-view fallback for test mocks. - Update AggregatedConfiguration.GetResultsDirectoryCore to consult the unified command-line view first so JSON-supplied results-directory is honored. - Strengthen tests: rewrite the precedence test to use a shared storage key (a JSON array) so a flipped Order would actually fail it; add ProviderAwareResolution_CliZeroArityShadowsJsonIndexedArgs end-to-end test that locks in the new behavior through the CommandLineHandler facade. - Expand validator TODOs with the concrete runtime crash sites surfaced during review (timeout args[0], exit-on-process-exit int.Parse). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…directory Review-round-2 fixes: - AggregatedConfiguration.GetResultsDirectoryCore now checks for the CommandLineConfigurationProvider before going through the unified path. Without it (legacy hand-built AggregatedConfiguration), the parseResult fallback runs first so a custom provider cannot silently demote CLI precedence for --results-directory. - Document the provider contract for the commandLineOptions section directly on TryGetCommandLineOptionFromProviders: TryGet returning true with a null value is treated as absent at this provider, and indexed entries must be contiguous from :0. - Add focused in-memory provider tests that lock the provider-aware invariants without requiring a JSON file: * FirstProviderWithDataShadowsLaterProvidersForSameOption * ExplicitDisableAtFirstProviderShortCircuits Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This prototype routes command-line options through the platform IConfiguration model so testconfig.json can provide values consumed by existing ICommandLineOptions users, while preserving CLI precedence.
Changes:
- Adds a CLI-backed configuration source/provider under
commandLineOptions:*. - Adds provider-aware command-line option lookup helpers on
AggregatedConfiguration/ConfigurationExtensions. - Wires
CommandLineHandlerto use the unified configuration view and adds unit coverage for precedence and lookup behavior.
Show a summary per file
| File | Description |
|---|---|
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.CommonServices.cs |
Registers the CLI configuration source and passes built configuration into command-line handling. |
src/Platform/Microsoft.Testing.Platform/Configurations/PlatformConfigurationConstants.cs |
Adds the commandLineOptions section constant. |
src/Platform/Microsoft.Testing.Platform/Configurations/ConfigurationExtensions.cs |
Adds unified command-line option lookup helpers. |
src/Platform/Microsoft.Testing.Platform/Configurations/CommandLineConfigurationSource.cs |
Adds the configuration source for parsed CLI options. |
src/Platform/Microsoft.Testing.Platform/Configurations/CommandLineConfigurationProvider.cs |
Flattens parsed CLI options into configuration keys. |
src/Platform/Microsoft.Testing.Platform/Configurations/AggregatedConfiguration.cs |
Adds provider-aware option resolution and uses it for results-directory. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineOptionsValidator.cs |
Documents current validation gaps for JSON-sourced options. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineManager.cs |
Passes configuration into CommandLineHandler. |
src/Platform/Microsoft.Testing.Platform/CommandLine/CommandLineHandler.cs |
Delegates option reads to configuration when available. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/CommandLineConfigurationProviderTests.cs |
Adds tests for CLI provider flattening and precedence behavior. |
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Configuration/CommandLineConfigurationExtensionsTests.cs |
Adds tests for helper and handler behavior over configuration-backed options. |
Copilot's findings
- Files reviewed: 11/11 changed files
- Comments generated: 1
Comment on lines
+232
to
+239
| => _configuration is not null | ||
| ? _configuration.IsCommandLineOptionSet(optionName) | ||
| : ParseResult.IsOptionSet(optionName); | ||
|
|
||
| public bool TryGetOptionArgumentList(string optionName, [NotNullWhen(true)] out string[]? arguments) | ||
| => _configuration is not null | ||
| ? _configuration.TryGetCommandLineOptionArguments(optionName, out arguments) | ||
| : ParseResult.TryGetOptionArgumentList(optionName, out arguments); |
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.
Summary
Prototype for #6349: let users specify CLI options inside
testconfig.jsonby routingICommandLineOptionsthroughIConfiguration.This is a draft for review. It is not a finished feature — I'm opening it so we can look at the approach end-to-end and decide whether to invest in the rough edges listed below or move to a different design.
Approach (Option C – unified configuration view)
CommandLineConfigurationSource(Order = 0) wraps the parsed CLI into anIConfigurationProviderthat flattens each option undercommandLineOptions:<name>(zero-arity) orcommandLineOptions:<name>:<index>(arg-bearing).TestHostBuilder.CommonServicesbefore the JSON source, so CLI keeps highest precedence.AggregatedConfiguration.TryGetCommandLineOptionFromProvidersdoes a provider-aware lookup: it walks providers in registration order, and the first provider with any data for that option wins outright (no per-key cross-provider merging). This avoids cases whereIConfiguration[key]accidentally returns the JSON:0while the CLI set the zero-arity bare key, or vice versa.IsCommandLineOptionSet,TryGetCommandLineOptionArguments) delegate to that method when given anAggregatedConfiguration, andCommandLineHandlerbecomes a thin facade that does the same. End result: every consumer ofICommandLineOptionstransparently sees JSON-sourced options.What works
testconfig.jsonkeys undercommandLineOptionsare surfaced everywhereICommandLineOptionsis consumed.--results-directorydefined in JSON is honored (was previously ignored — the legacyGetResultsDirectoryCorepath was bypassing JSON).CommandLineHandleris kept so external code keeps compiling.Rough edges I'd like to discuss before going further
These are documented in the code with
OPTION C LIMITATION/OPTION C TODOcomments — I deliberately did not fix them in this PR because I want a design call first:CommandLineOptionsValidatorwalksparseResult.Optionsonly. Options set exclusively via JSON bypass arity, per-option, and "unknown option" validation. Concrete crash sites I found:--timeoutdoesargs[0]without a length check,--exit-on-process-exitdoesint.Parseonargs[0]. Fixing this means giving the validator a unified view too (probably moving it afterIConfigurationis built).--diagnostic*,--config-file,--no-bannerare read offparseResultbeforeIConfigurationis built. Honoring them from JSON requires either a two-phase config build or rewriting those bootstrap paths."true"/"false"is ambiguous with the zero-arity presence marker. I documented that callers must use the array form ("foo": ["true"]) for non-boolean string values that happen to be"true"/"false". No code enforces this today.commandLineOptions:*correspond to real registered options or have the right arity.internalfor now. If we ship Option C we need to decide whether they become public surface.A hybrid B→C follow-up plan exists outside the repo (option B = explicit per-feature JSON keys for the highest-impact options, option C = generic unified view for the rest). Happy to share if useful.
Tests
CommandLineConfigurationProviderTestscovers the source/provider flattening + the provider-aware invariants (includingProviderAwareResolution_CliZeroArityShadowsJsonIndexedArgsend-to-end, plus two in-memory provider tests added in the latest review round).CommandLineConfigurationExtensionsTestscovers the helper-level and handler-end-to-end behavior.Why draft?
Looking for a thumbs-up on the shape of this approach (unified
IConfigurationview) before I invest in (1)–(4). If you'd rather take a more explicit per-feature path (Option B) for the most-requested options, I can switch direction without much waste — most of the JSON binding plumbing is reusable either way.