Fix flaky TimingProperty.ToString test by using an explicit format string

[Evangelink]

Problem

TimingProperty_ToStringIsCorrect (and its sibling _WithStepTimings_ variant) occasionally fails with:

expected: "TimingProperty { GlobalTiming = TimingInfo { StartTime = 09/01/2021 00:00:00 +00:00, EndTime = 09/01/2021 01:02:03 +00:00, Duration = 01:02:03 }, StepTimings = [] }"
actual:   "TimingProperty { GlobalTiming = TimingInfo { StartTime = 09/01/2021 00:00:00 +00:00 +00:00, EndTime = 09/01/2021 01:02:03 +00:00 +00:00, Duration = 01:02:03 }, StepTimings = [] }"

Note the duplicated +00:00 +00:00 in the actual output.

Root cause

TimingInfo.ToString formats the start/end via:

StartTime.ToString(CultureInfo.InvariantCulture)

DateTimeOffset.ToString(IFormatProvider) is implemented as roughly format = dtfi.GeneralLongTimePattern + " zzz". If anything causes GeneralLongTimePattern (which derives from ShortDatePattern + " " + LongTimePattern) to already contain zzz, you get the doubled offset.

The existing tests tried to defend against this by setting CultureInfo.CurrentCulture = CultureInfo.InvariantCulture, but:

1. ToString(InvariantCulture) does not consult CurrentCulture, so the workaround did not actually protect the formatter path that fails.
2. Mutating CurrentCulture is per-thread, and acceptance/unit tests run with assembly-level method parallelization, so the mutation itself becomes a source of cross-test interference.

Fix

• TimingInfo.ToString now uses an explicit format string "MM/dd/yyyy HH:mm:ss zzz" with InvariantCulture. Output is fully deterministic regardless of culture/runtime state.
• Removed the no-longer-needed CultureInfo.CurrentCulture mutation from the two timing tests.

Verification

• Full build.cmd -c Debug succeeds (0 warnings, 0 errors).
• Ran the 3 timing tests 5× on net9.0 and 1× on net8.0 — all 3/3 pass on every iteration.

[Copilot]

Pull request overview

Fixes flakiness in Microsoft.Testing.Platform timing ToString() output by making DateTimeOffset formatting deterministic and removing culture mutation from unit tests.

Changes:

• Updated TimingInfo.ToString() to format StartTime/EndTime using an explicit invariant format string.
• Removed CultureInfo.CurrentCulture mutation from timing-related unit tests to avoid cross-test interference.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Messages/TestNodePropertiesTests.cs	Removes per-test CurrentCulture mutation and keeps assertions stable against deterministic formatting.
src/Platform/Microsoft.Testing.Platform/Messages/TimingProperties.cs	Switches TimingInfo.ToString() DateTimeOffset formatting to an explicit invariant format to prevent duplicated offsets.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Evangelink]

Expert Code Review — PR #8502

I've completed a systematic review applying all 21 dimensions from the expert-reviewer agent definition. Here's the summary:

#	Dimension	Verdict
13	Test Completeness & Coverage	🟡 1 MODERATE
15	Code Structure & Simplification	🔵 1 NIT
17	Documentation Accuracy	🔵 1 NIT

✅ 18/21 dimensions clean — no findings in:

• Algorithmic Correctness
• Threading & Concurrency
• Public API & Binary Compatibility
• Performance & Allocations
• Cross-TFM Compatibility
• Test Isolation
• Flakiness Patterns
• Localization & Resources
• Assertion Quality
• Scope & PR Discipline

(3 dimensions skipped as N/A: Security & IPC Contract Safety, Resource Management, Defensive Coding at Boundaries — no changes in those areas)

---

Overall Assessment

This is an excellent bug fix with a thorough root cause analysis. The fix correctly addresses the flaky test by:

✅ Using an explicit format string ("MM/dd/yyyy HH:mm:ss zzz") to eliminate culture-dependent behavior
✅ Removing ineffective CurrentCulture mutation that was causing cross-test interference under parallel execution
✅ Making output deterministic regardless of culture/runtime state
✅ Improving test isolation by eliminating shared state mutation

The PR demonstrates strong engineering discipline:

• Clear problem statement with concrete failure examples
• Root cause traced to DateTimeOffset.ToString(IFormatProvider) implementation details
• Minimal, surgical fix (2 files, +14/-32 lines)
• Thorough verification (5× on net9.0, 1× on net8.0)

---

Findings Summary

• Test Completeness — Missing coverage for non-UTC timezone offsets (positive/negative offsets like +05:00, -08:00)
• Documentation — Public ToString() method should document the deterministic format
• Code Structure — Minor StringBuilder consolidation opportunity

All findings are non-blocking improvements. The core fix is sound and ready to merge.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• builds.dotnet.microsoft.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "builds.dotnet.microsoft.com"

See Network Configuration for more information.

Generated by Expert Code Review (on open) for issue #8502 · ● 27.4M

Comments that could not be inline-anchored

test/UnitTests/Microsoft.Testing.Platform.UnitTests/Messages/TestNodePropertiesTests.cs:164

[MODERATE] Test Completeness & Coverage

Both existing tests only use UTC offset (+00:00 via default TimeSpan). The PR fixes a bug where timezone offsets could be duplicated in the format string, but no tests verify the new explicit format string works correctly with non-UTC offsets.

Scenario: A DateTimeOffset with a non-UTC offset like +05:30 or -08:00 should format correctly without duplication.

Recommendation: Add a test case with a non-UTC offset to provide confidenc…

src/Platform/Microsoft.Testing.Platform/Messages/TimingProperties.cs:51

[NIT] Code Structure & Simplification

The StringBuilder usage could be consolidated for better readability. Currently, the method uses 7 separate Append calls with some very short interpolated strings.

Recommendation: Consider combining adjacent string literals to reduce the number of calls:

builder.Append($"{nameof(TimingInfo)} {{ {nameof(StartTime)} = ");
builder.Append(StartTime.ToString("MM/dd/yyyy HH:mm:ss zzz", CultureInfo.InvariantCulture));
builder.Append($", {name…

</details>

<details><summary>src/Platform/Microsoft.Testing.Platform/Messages/TimingProperties.cs:40</summary>

**[NIT] Documentation Accuracy**

`TimingInfo.ToString()` is a public API (listed in `PublicAPI.Shipped.txt`) that now uses an explicit format string to ensure deterministic output. The method currently only has `/// &lt;inheritdoc /&gt;` which provides no information about the specific format.

**Recommendation:** Add XML documentation mentioning the output format is deterministic and culture-invariant:
```csharp
/// &lt;summary&gt;
/// Returns a string representation of this timing information using inva…

</details>

[Youssef1313]

Mutating CurrentCulture is per-thread, and acceptance/unit tests run with assembly-level method parallelization, so the mutation itself becomes a source of cross-test interference.

I don't think this is correct. CurrentCulture is most likely an async local.