Conversation
…e-dashoffset Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
|
@copilot your image of the "fix" isn't right. The green would then account for an50% pass rate. And 25% is grey implying not run. |
…erted colors Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
You're right, the sign removal was wrong. The real root cause is locale-dependent decimal formatting — Fixed in 007ad90 by adding |
There was a problem hiding this comment.
Review: Fix inverted colors in HTML report ring chart due to locale-dependent decimal formatting
This is a correct and targeted fix for a genuine locale-sensitivity bug. The root cause analysis is accurate: ToString("F2") without CultureInfo.InvariantCulture produces comma as the decimal separator on many locales (e.g., Italian, French, German), which breaks SVG stroke-dasharray and stroke-dashoffset attribute parsing since SVG uses commas as value separators.
Correctness
The fix is correct. All five numeric formatting call sites that produce SVG/CSS attribute values are addressed:
passRate.ToString("F0", CultureInfo.InvariantCulture)— ring center percentage label (line 224)len.ToString("F2", CultureInfo.InvariantCulture)— stroke-dasharray segment length (line 256)(circumference - len).ToString("F2", CultureInfo.InvariantCulture)— stroke-dasharray gap (line 258)offset.ToString("F2", CultureInfo.InvariantCulture)— stroke-dashoffset (line 260)FormatDurationreturns viastring.Create(CultureInfo.InvariantCulture, ...)for all three branches (lines 372, 377, 380)
Minor Observations (not blocking)
1. FormatDuration — inconsistent approach vs. AppendRingSegment
The AppendRingSegment method uses .ToString("F2", CultureInfo.InvariantCulture) directly on double, while FormatDuration uses string.Create(CultureInfo.InvariantCulture, $"..."). Both are correct, but mixing styles across the same file is slightly inconsistent. For what it's worth, string.Create with an interpolated string handler avoids a format string allocation and is the more modern idiom — so if anything, the FormatDuration style is the better pattern.
2. FormatDuration output is used as display text, not SVG attribute values
The duration string is rendered inside an HTML <span> (line 243: sb.Append(FormatDuration(totalDurationMs))), not as an SVG numeric attribute. This means locale-dependent decimals here would not break SVG parsing — but they would cause visual inconsistency in the HTML text (e.g., "1,50s" vs "1.50s"), so fixing it here is still the right call for a consistent user experience.
3. Integer ToString() calls (lines 232–236) are locale-safe
The summary.Total.ToString(), summary.Passed.ToString(), etc. are integer types — ToString() on integers never uses a decimal separator, so these are safe to leave as-is.
4. passRate.ToString("F0", ...) — "F0" rounds to integer
"F0" produces no decimal point regardless of locale (it rounds to the nearest whole number), so the original passRate.ToString("F0") was already locale-safe. Adding CultureInfo.InvariantCulture here is harmless and makes the intent explicit, which is fine.
Summary
The fix is correct, minimal in scope, and directly addresses the reported bug. No unrelated changes are included. The approach is idiomatic for .NET locale-safe number formatting. Ready to merge.
|
@copilot failing pr checks |
…atDuration Co-authored-by: thomhurst <30480171+thomhurst@users.noreply.github.com>
Head branch was pushed to by a user without write access
[//]: # (dependabot-start)⚠️ **Dependabot is rebasing this PR**⚠️ Rebasing might not happen immediately, so don't worry if this takes some time. Note: if you make any changes to this PR yourself, they will take precedence over the rebase. --- [//]: # (dependabot-end) Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.19.57 to 1.21.6. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.21.6 <!-- Release notes generated using configuration in .github/release.yml at v1.21.6 --> ## What's Changed ### Other Changes * perf: replace object locks with Lock type for efficient synchronization by @thomhurst in thomhurst/TUnit#5219 * perf: parallelize test metadata collection for source-generated tests by @thomhurst in thomhurst/TUnit#5221 * perf: use GetOrAdd args overload to eliminate closure allocations in event receivers by @thomhurst in thomhurst/TUnit#5222 * perf: self-contained TestEntry<T> with consolidated switch invokers eliminates per-test JIT by @thomhurst in thomhurst/TUnit#5223 ### Dependencies * chore(deps): update tunit to 1.21.0 by @thomhurst in thomhurst/TUnit#5220 **Full Changelog**: thomhurst/TUnit@v1.21.0...v1.21.6 ## 1.21.0 <!-- Release notes generated using configuration in .github/release.yml at v1.21.0 --> ## What's Changed ### Other Changes * perf: reduce ConcurrentDictionary closure allocations in hot paths by @thomhurst in thomhurst/TUnit#5210 * perf: reduce async state machine overhead in test execution pipeline by @thomhurst in thomhurst/TUnit#5214 * perf: reduce allocations in EventReceiverOrchestrator and TestContextExtensions by @thomhurst in thomhurst/TUnit#5212 * perf: skip timeout machinery when no timeout configured by @thomhurst in thomhurst/TUnit#5211 * perf: reduce allocations and lock contention in ObjectTracker by @thomhurst in thomhurst/TUnit#5213 * Feat/numeric tolerance by @agray in thomhurst/TUnit#5110 * perf: remove unnecessary lock in ObjectTracker.TrackObjects by @thomhurst in thomhurst/TUnit#5217 * perf: eliminate async state machine in TestCoordinator.ExecuteTestAsync by @thomhurst in thomhurst/TUnit#5216 * perf: eliminate LINQ allocation in ObjectTracker.UntrackObjectsAsync by @thomhurst in thomhurst/TUnit#5215 * perf: consolidate module initializers into single .cctor via partial class by @thomhurst in thomhurst/TUnit#5218 ### Dependencies * chore(deps): update tunit to 1.20.0 by @thomhurst in thomhurst/TUnit#5205 * chore(deps): update dependency nunit3testadapter to 6.2.0 by @thomhurst in thomhurst/TUnit#5206 * chore(deps): update dependency cliwrap to 3.10.1 by @thomhurst in thomhurst/TUnit#5207 **Full Changelog**: thomhurst/TUnit@v1.20.0...v1.21.0 ## 1.20.0 <!-- Release notes generated using configuration in .github/release.yml at v1.20.0 --> ## What's Changed ### Other Changes * Fix inverted colors in HTML report ring chart due to locale-dependent decimal formatting by @Copilot in thomhurst/TUnit#5185 * Fix nullable warnings when using Member() on nullable properties by @Copilot in thomhurst/TUnit#5191 * Add CS8629 suppression and member access expression matching to IsNotNullAssertionSuppressor by @Copilot in thomhurst/TUnit#5201 * feat: add ConfigureAppHost hook to AspireFixture by @thomhurst in thomhurst/TUnit#5202 * Fix ConfigureTestConfiguration being invoked twice by @thomhurst in thomhurst/TUnit#5203 * Add IsEquivalentTo assertion for Memory<T> and ReadOnlyMemory<T> by @thomhurst in thomhurst/TUnit#5204 ### Dependencies * chore(deps): update dependency gitversion.tool to v6.6.2 by @thomhurst in thomhurst/TUnit#5181 * chore(deps): update dependency gitversion.msbuild to 6.6.2 by @thomhurst in thomhurst/TUnit#5180 * chore(deps): update tunit to 1.19.74 by @thomhurst in thomhurst/TUnit#5179 * chore(deps): update verify to 31.13.3 by @thomhurst in thomhurst/TUnit#5182 * chore(deps): update verify to 31.13.5 by @thomhurst in thomhurst/TUnit#5183 * chore(deps): update aspire to 13.1.3 by @thomhurst in thomhurst/TUnit#5189 * chore(deps): update dependency stackexchange.redis to 2.12.4 by @thomhurst in thomhurst/TUnit#5193 * chore(deps): update microsoft/setup-msbuild action to v3 by @thomhurst in thomhurst/TUnit#5197 **Full Changelog**: thomhurst/TUnit@v1.19.74...v1.20.0 ## 1.19.74 <!-- Release notes generated using configuration in .github/release.yml at v1.19.74 --> ## What's Changed ### Other Changes * feat: per-hook activity spans with method names by @thomhurst in thomhurst/TUnit#5159 * fix: add tooltip to truncated span names in HTML report by @thomhurst in thomhurst/TUnit#5164 * Use enum names instead of numeric values in test display names by @Copilot in thomhurst/TUnit#5178 * fix: resolve CS8920 when mocking interfaces whose members return static-abstract interfaces by @lucaxchaves in thomhurst/TUnit#5154 ### Dependencies * chore(deps): update tunit to 1.19.57 by @thomhurst in thomhurst/TUnit#5157 * chore(deps): update dependency gitversion.msbuild to 6.6.1 by @thomhurst in thomhurst/TUnit#5160 * chore(deps): update dependency gitversion.tool to v6.6.1 by @thomhurst in thomhurst/TUnit#5161 * chore(deps): update dependency polyfill to 9.20.0 by @thomhurst in thomhurst/TUnit#5163 * chore(deps): update dependency polyfill to 9.20.0 by @thomhurst in thomhurst/TUnit#5162 * chore(deps): update dependency polyfill to 9.21.0 by @thomhurst in thomhurst/TUnit#5166 * chore(deps): update dependency polyfill to 9.21.0 by @thomhurst in thomhurst/TUnit#5167 * chore(deps): update dependency polyfill to 9.22.0 by @thomhurst in thomhurst/TUnit#5168 * chore(deps): update dependency polyfill to 9.22.0 by @thomhurst in thomhurst/TUnit#5169 * chore(deps): update dependency coverlet.collector to 8.0.1 by @thomhurst in thomhurst/TUnit#5177 ## New Contributors * @lucaxchaves made their first contribution in thomhurst/TUnit#5154 **Full Changelog**: thomhurst/TUnit@v1.19.57...v1.19.74 Commits viewable in [compare view](thomhurst/TUnit@v1.19.57...v1.21.6). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

The SVG ring chart in the HTML test report renders pass/fail segment colors in the wrong positions on systems using a non-English locale (e.g. Italian, French, German) where the decimal separator is a comma instead of a period.
Root cause:
ToString("F2")withoutCultureInfo.InvariantCultureproduces comma-separated decimals on non-English locales (e.g. Italian:254,47instead of254.47). SVG parsers interpret commas as value separators, so:stroke-dasharray="254,47 84,82"→ parsed as 4 values[254, 47, 84, 82]instead of 2stroke-dashoffset="-254,47"→ truncated to-254This causes the ring segments to render with completely wrong sizes and positions.
Fix: Added
CultureInfo.InvariantCultureto all numericToStringcalls that produce SVG/CSS attribute values, ensuring period decimal separators regardless of system locale. Also fixed theFormatDurationmethod for the same reason.Left: Bug (comma decimals from Italian locale) — ring segments broken. Right: Fix (period decimals) — green correctly covers 75%, red covers 25%.
Original prompt
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.