Unify duration parsing across MTP CLI options (#8430)#8506
Open
Evangelink wants to merge 8 commits into
Open
Conversation
The three duration-style CLI options used inconsistent grammars:
* --timeout used a hand-rolled parser that only accepted single-character
suffixes (h|m|s) and rejected bare numbers entirely.
* --hangdump-timeout and --retry-failed-tests-delay used the shared
TimeSpanParser, which accepts long-form suffixes (90min, 1.5hour,
5400seconds), ms/s/m/h/d, and treats bare numbers as milliseconds.
Copy-pasting a value across options would silently break: `--timeout 90min`
errored while `--hangdump-timeout 90min` worked. `--timeout 200` was
rejected; `--retry-failed-tests-delay 200` parsed as 200 ms. And the
existing parser silently accepted bogus alphabetic tails (`30monkey`
became 30 minutes because the regex's `s?[a-z]*` tail matched and dispatch
ran StartsWith("m")).
All three options now route through a single parser:
* TimeSpanParser regex tightened to an explicit suffix vocabulary
(ms|mils?|milliseconds?|s|secs?|seconds?|m|mins?|minutes?|h|hours?|d|days?),
so bogus tails like 30monkey are rejected.
* New TryParseRequireSuffix overload for callers (i.e. --timeout) that
must require an explicit unit; a new TimeSpanDefaultUnit enum lets
other callers express their own bare-number default unit.
* Suffix dispatch uses OrdinalIgnoreCase so 1H/1M/1D parse correctly.
* Number parsing uses InvariantCulture; TimeSpan.From* calls are
wrapped so overflow returns false instead of throwing.
* --timeout validator also bounds the value to Timer.MaxSupportedTimeout
(uint.MaxValue - 1 ms, ~49.7 days) so `--timeout 60d` produces a
friendly CLI error instead of crashing inside
CancellationTokenSource.CancelAfter.
Help text and error messages for all three options now describe the same
grammar; XLFs regenerated for Platform, HangDump, and Retry.
Behavior changes worth calling out:
1. --timeout is now a strict superset of its previous grammar (long-form
suffixes, ms, d now accepted) but rejects -1s, 1e3s, +1s, and .5s
(the old float.TryParse accepted those).
2. Bogus suffixes like 30monkey are now hard-rejected on all three
options. Previously the regex silently accepted them and dispatched
on the first letter.
3. --timeout now validates against Timer.MaxSupportedTimeout.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Unifies duration parsing for Microsoft.Testing.Platform (MTP) CLI options by routing --timeout, --hangdump-timeout, and --retry-failed-tests-delay through a single TimeSpanParser grammar, and updates help/error text + tests to match the new behavior.
Changes:
- Tightens and extends
TimeSpanParser(explicit suffix vocabulary, case-insensitive suffix handling, invariant-culture parsing, overflow-safeTryParse, new “require suffix” API, and configurable default unit for bare numbers). - Updates
--timeouthandling to use the shared parser and adds a validation guard forTimer.MaxSupportedTimeout. - Refreshes help/info expectations and localized resources (Platform, HangDump, Retry) to document the unified grammar, and adds/updates unit + acceptance tests.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTests/Microsoft.Testing.Platform.UnitTests/Helpers/TimeSpanParserTests.cs | Adds unit tests for the expanded duration grammar, default-unit behavior, and edge cases. |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/HelpInfoTests.cs | Updates MSTest help text expectations for the new --timeout grammar. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/TimeoutTests.cs | Updates timeout acceptance tests and adds new scenarios (bogus suffix tail, max-range guard, new suffix forms). |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoTests.cs | Updates MTP help/info expectations for --timeout to the unified grammar. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HelpInfoAllExtensionsTests.cs | Updates help/info expectations for --timeout, --hangdump-timeout, and --retry-failed-tests-delay with unified grammar details. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hant.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.zh-Hans.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.tr.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ru.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pt-BR.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.pl.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ko.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.ja.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.it.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.fr.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.es.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.de.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/xlf/PlatformResources.cs.xlf | Updates localized resource entries impacted by the new --timeout strings. |
| src/Platform/Microsoft.Testing.Platform/Resources/PlatformResources.resx | Updates the English help/validation strings for --timeout to the unified grammar. |
| src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.CommonServices.cs | Switches runtime consumption of --timeout to the shared TimeSpanParser API. |
| src/Platform/Microsoft.Testing.Platform/Helpers/TimeSpanParser.cs | Implements the unified duration parsing behavior and new APIs (default units + require-suffix). |
| src/Platform/Microsoft.Testing.Platform/CommandLine/PlatformCommandLineProvider.cs | Updates --timeout validation to use TimeSpanParser and applies the max supported timeout guard. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.zh-Hant.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.zh-Hans.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.tr.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ru.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.pt-BR.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.pl.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ko.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.ja.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.it.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.fr.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.es.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.de.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/xlf/ExtensionResources.cs.xlf | Updates Retry extension localized strings for the unified duration examples/grammar. |
| src/Platform/Microsoft.Testing.Extensions.Retry/Resources/ExtensionResources.resx | Updates Retry extension English strings to include unified examples (e.g., 500ms, 1d). |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.zh-Hant.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.zh-Hans.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.tr.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.ru.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.pt-BR.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.pl.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.ko.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.ja.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.it.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.fr.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.es.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.de.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/xlf/ExtensionResources.cs.xlf | Updates HangDump extension localized help text to list unified suffix formats and bare-number default. |
| src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/ExtensionResources.resx | Updates HangDump extension English help text to include ms/d formats and bare-number default behavior. |
Copilot's findings
- Files reviewed: 50/50 changed files
- Comments generated: 3
Evangelink
commented
May 22, 2026
Member
Author
Evangelink
left a comment
Evangelink
left a comment
There was a problem hiding this comment.
✅ 21/21 dimensions clean — no findings.
This PR successfully unifies duration parsing across MTP CLI options. The implementation is well-designed:
Strengths:
- Regex tightened to explicit vocabulary, rejecting bogus suffixes
- Overflow handled gracefully (
TryCreateTimeSpanwrapper) Timer.MaxSupportedTimeoutvalidation preventsCancelAftercrashes- Case-insensitive suffix matching (
OrdinalIgnoreCase) InvariantCulturenumber parsing- Comprehensive test coverage (89/89 tests passing)
.xlffiles properly auto-generated (not manually edited)- Help text consistent across all three options
Technical Review:
- ✅ Algorithmic Correctness: Parser logic correct, edge cases handled (null, overflow, invalid input)
- ✅ Defensive Coding: Input validation robust, range checks in place
- ✅ Localization:
.resxchanges proper,.xlfauto-generated - ✅ API Surface: New
TimeSpanDefaultUnitenum andTryParseRequireSuffixare appropriate additions (internal scope) - ✅ Cross-TFM:
#if NET7_0_OR_GREATERcorrectly gates source-generated regex - ✅ Code Structure: Clean, readable, uses pattern matching and switch expressions appropriately
- ✅ Naming:
TryParseRequireSuffixclearly communicates intent - ✅ Documentation: Error messages and help text accurate and helpful
Behavior Changes (documented in PR description):
--timeoutnow accepts long-form suffixes +ms/d(strict superset, backward compatible)- Rejects previously-silently-accepted bogus inputs (
-1s,1e3s,+1s,.5s,30monkey) - Range validation against
Timer.MaxSupportedTimeout(prevents runtime crash)
All changes are improvements. No issues found.
Generated by Expert Code Review (on open) for issue #8506 · ● 8.1M
- Tighten TimeSpanParser regex so trailing whitespace is only accepted when a unit suffix follows (e.g. "90 " no longer matches). - Expand PlatformCommandLineTimeoutArgumentErrorMessage to mention the positive value and supported range (~49.7 days), not just the suffix grammar. Regenerated XLFs. - Update MSTest acceptance TimeoutTests to expect the new message. - Update TryParse_UppercaseSuffix_ParsesCorrectly to assert the exact TimeSpan for each input instead of just non-zero. Addresses review feedback on PR #8506. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>
Contributor
Conflicts are resolved and merged with |
Remove duplicate <value>/<source>+<target> entries for PlatformCommandLineTimeoutArgumentErrorMessage left over from merging origin/main, which caused 'trans-unit has invalid child element source' build errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rtedTimeout, remove dead TryParseTimeoutArgument Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+659
to
+660
| <value>A global test execution timeout. | ||
| Takes one argument as string in the format <value>[h|m|s] where 'value' is float.</value> | ||
| Takes one argument as a time value with an explicit unit suffix. Accepted units are 'ms', 's', 'm', 'h', 'd' (long forms 'milliseconds', 'seconds', 'minutes', 'hours', 'days' are also accepted), e.g. '500ms', '5400s', '90m', '1.5h', '1d'.</value> |
Member
Author
There was a problem hiding this comment.
Addressed in 26e7b26: --timeout help, error message, and the shared TimeSpanParser grammar hint now list the full alias set (ms/mil(s)/millisecond(s), s/sec(s)/second(s), m/min(s)/minute(s), h/hour(s), d/day(s)), matching the existing HangDump style. Acceptance test expectations were updated to match and a new TimeoutWithValidArg_WithAliasSuffix integration test pins --timeout 500mil end-to-end so the documented aliases keep working through the --timeout path. While here also fixed a pre-existing test/resx mismatch for --retry-failed-tests-delay.
Address PR #8506 review: the TimeSpanParser regex accepts mil/mils/sec/secs/min/mins short-form aliases but they were not documented in the --timeout help text or error message, leading users to think only 'ms'/'s'/'m'/'h'/'d' were valid. Align the --timeout description and error message with the existing HangDump style by listing all accepted suffixes exhaustively. - Update PlatformCommandLineTimeoutOptionDescription and PlatformCommandLineTimeoutArgumentErrorMessage to list every accepted suffix. - Update TimeSpanParser.SuffixGrammar (used by other consumers via GetGrammarHint) to keep the wording consistent. - Update HelpInfo* acceptance test expectations to match. - Add TimeoutWithValidArg_WithAliasSuffix integration test pinning that '500mil' flows through the --timeout option (not just the shared parser). - Fix a pre-existing test/resx mismatch for --retry-failed-tests-delay in HelpInfoAllExtensionsTests that was introduced earlier in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/Platform/Microsoft.Testing.Extensions.HangDump/Resources/ExtensionResources.resx:192
HangDumpTimeoutOptionInvalidArgumentis returned when--hangdump-timeoutfailsTimeSpanParser.TryParse(...), but the message currently says the option "expects a single timeout argument". Arity is alreadyExactlyOne, so this error is misleading for format/unit failures (e.g.--hangdump-timeout 30monkey). Consider changing the resource text to say it requires a valid time value and (optionally) include a few accepted examples / mention the bare-number default to milliseconds to match the updated description.
Default is 30m.</value>
</data>
<data name="HangDumpTimeoutOptionInvalidArgument" xml:space="preserve">
<value>'--hangdump-timeout' expects a single timeout argument</value>
</data>
- Files reviewed: 52/52 changed files
- Comments generated: 0 new
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.
Fixes #8430.
The three duration-style CLI options used inconsistent grammars:
--timeoutused a hand-rolled parser that only accepted single-character suffixes (h|m|s) and rejected bare numbers entirely.--hangdump-timeoutand--retry-failed-tests-delayused the sharedTimeSpanParser, which accepts long-form suffixes (90min,1.5hour,5400seconds),ms/s/m/h/d, and treats bare numbers as milliseconds.Copy-pasting a value across options would silently break (
--timeout 90minerrored while--hangdump-timeout 90minworked;--timeout 200was rejected;--retry-failed-tests-delay 200parsed as 200 ms). The existing parser also silently accepted bogus alphabetic tails (30monkeybecame 30 minutes because the regex'ss?[a-z]*tail matched and dispatch ranStartsWith(""m"")).What this PR does
All three options now route through a single parser:
TimeSpanParserregex tightened to an explicit suffix vocabulary (ms|mils?|milliseconds?|s|secs?|seconds?|m|mins?|minutes?|h|hours?|d|days?).TryParseRequireSuffixoverload for callers (i.e.--timeout) that must require an explicit unit; a newTimeSpanDefaultUnitenum lets other callers express their own bare-number default unit (preserving existing ms default for--hangdump-timeoutand--retry-failed-tests-delay).OrdinalIgnoreCase(wasOrdinal, breaking1H/1M/1D).InvariantCulture;TimeSpan.From*calls are wrapped so overflow returnsfalseinstead of throwing.--timeoutvalidator also bounds the value toTimer.MaxSupportedTimeout(uint.MaxValue - 1ms, ~49.7 days) so--timeout 60dproduces a friendly CLI error instead of crashing insideCancellationTokenSource.CancelAfter.Help text and error messages for all three options now describe the same grammar; XLFs regenerated for Platform, HangDump, and Retry.
Behavior changes worth calling out in release notes
--timeoutis now a strict superset of its previous grammar (long-form suffixes,ms,dnow accepted) but rejects-1s,1e3s,+1s, and.5s(the oldfloat.TryParseaccepted those).30monkeyare now hard-rejected on all three options. Previously the regex silently accepted them and dispatched on the first letter.--timeoutnow validates againstTimer.MaxSupportedTimeout.Validation
TimeSpanParserTests: 89/89 pass.HelpInfoTests+HelpInfoAllExtensionsTests+TimeoutTests(including 5 new tests for the new grammar / range guard /30monkeyrejection): 63/63 pass.HelpInfoTests: 6/6 pass.Reviews
Two rubber-duck + expert-reviewer passes. MAJOR (
CancelAfterrange guard), two MODERATE (API renameRequired→TryParseRequireSuffix, HangDump/Retry help parity), and several NITs were addressed before pushing.