fix: don't render test's own trace as Linked Trace in HTML report#5580
fix: don't render test's own trace as Linked Trace in HTML report#5580
Conversation
The engine auto-registers the test's own traceId in TraceRegistry for OTLP cross-process log correlation. HtmlReporter then read it back as an "additional" trace, causing the primary trace to render twice — once filtered under "Trace Timeline", once raw under "Linked Trace: TUnit". Filter out the primary traceId so only user-registered external traces (via TestContext.RegisterTrace) appear as Linked Traces, matching documented behavior.
There was a problem hiding this comment.
Code Review
Summary
Clean, well-scoped fix for a real UX bug. The implementation is correct and the test coverage is thorough.
What's Good
- Early-exit optimization (
allTraceIds.Length == 0 || primaryTraceId is null) avoids unnecessary work in the common case. - Reference preservation (
filtered.Count == allTraceIds.Length ? allTraceIds : filtered.ToArray()) is a nice touch — theShouldBeSameAstests validate this contract, confirming no allocation when filtering is a no-op. - Case-insensitive comparison (
StringComparison.OrdinalIgnoreCase) is the right choice for hex trace IDs that may arrive in different casing from different OTLP exporters. internal staticmakes the method unit-testable without leaking it as public API.- Test coverage hits all four meaningful branches: primary null, no match, match-only, and case-insensitive match.
- The
#if NETguard is consistent with the surrounding context.
Architectural Observation (non-blocking)
The fix is applied at the consumer side (HtmlReporter), filtering out a trace ID that the engine registered on the producer side (TraceRegistry) for a different purpose (OTLP cross-process correlation). This is pragmatic and avoids breaking OTLP, but it means any future reporter that reads TraceRegistry.GetTraceIds would have the same bug and need the same filter.
A more robust long-term approach would be to have TraceRegistry natively distinguish between primary and user-registered traces, exposing something like GetAdditionalTraceIds(testId, excludePrimary). That way the filtering logic lives once, at the data layer, and reporters get correct data by default.
That said, given this is a focused bug fix and the current reporter count is one, the current approach is a reasonable tradeoff. Worth keeping in mind if a second reporter is ever added.
Minor
The multi-line comment explaining why the filter exists is appropriate here — the reason is genuinely non-obvious (engine auto-registration for OTLP), and a future reader would be confused without it. Good call including it.
Verdict: Approved. The fix is correct, well-tested, and the tradeoff is understood. The architectural note above is a future consideration, not a blocker.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
TIP This summary will be updated as you push new changes. Give us feedback
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.35.2 to 1.37.10. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.37.10 <!-- Release notes generated using configuration in .github/release.yml at v1.37.10 --> ## What's Changed ### Other Changes * docs(test-filters): add migration callout for --filter → --treenode-filter by @johnkattenhorn in thomhurst/TUnit#5628 * fix: re-enable RPC tests and modernize harness (#5540) by @thomhurst in thomhurst/TUnit#5632 * fix(mocks): propagate [Obsolete] and null-forgiving raise dispatch (#5626) by @JohnVerheij in thomhurst/TUnit#5631 * ci: use setup-dotnet built-in NuGet cache by @thomhurst in thomhurst/TUnit#5635 * feat(playwright): propagate W3C trace context into browser contexts by @thomhurst in thomhurst/TUnit#5636 ### Dependencies * chore(deps): update tunit to 1.37.0 by @thomhurst in thomhurst/TUnit#5625 ## New Contributors * @johnkattenhorn made their first contribution in thomhurst/TUnit#5628 * @JohnVerheij made their first contribution in thomhurst/TUnit#5631 **Full Changelog**: thomhurst/TUnit@v1.37.0...v1.37.10 ## 1.37.0 <!-- Release notes generated using configuration in .github/release.yml at v1.37.0 --> ## What's Changed ### Other Changes * fix: stabilize flaky tests across analyzer, OTel, and engine suites by @thomhurst in thomhurst/TUnit#5609 * perf: engine hot-path allocation wins (#5528 B) by @thomhurst in thomhurst/TUnit#5610 * feat(analyzers): detect collection IsEqualTo reference equality (TUnitAssertions0016) by @thomhurst in thomhurst/TUnit#5615 * perf: consolidate test dedup + hook register guards (#5528 A) by @thomhurst in thomhurst/TUnit#5612 * perf: engine discovery/init path cleanup (#5528 C) by @thomhurst in thomhurst/TUnit#5611 * fix(assertions): render collection contents in IsEqualTo failure messages (#5613 B) by @thomhurst in thomhurst/TUnit#5619 * feat(analyzers): code-fix for TUnit0015 to insert CancellationToken (#5613 D) by @thomhurst in thomhurst/TUnit#5621 * fix(assertions): add Task reference forwarders on AsyncDelegateAssertion by @thomhurst in thomhurst/TUnit#5618 * test(asp-net): fix race in FactoryMethodOrderTests by @thomhurst in thomhurst/TUnit#5623 * feat(analyzers): code-fix for TUnit0049 to insert [MatrixDataSource] (#5613 C) by @thomhurst in thomhurst/TUnit#5620 * fix(pipeline): isolate AOT publish outputs to stop clobbering pack DLLs (#5622) by @thomhurst in thomhurst/TUnit#5624 ### Dependencies * chore(deps): update tunit to 1.36.0 by @thomhurst in thomhurst/TUnit#5608 * chore(deps): update modularpipelines to 3.2.8 by @thomhurst in thomhurst/TUnit#5614 **Full Changelog**: thomhurst/TUnit@v1.36.0...v1.37.0 ## 1.36.0 <!-- Release notes generated using configuration in .github/release.yml at v1.36.0 --> ## What's Changed ### Other Changes * fix: don't render test's own trace as Linked Trace in HTML report by @thomhurst in thomhurst/TUnit#5580 * fix(docs): benchmark index links 404 by @thomhurst in thomhurst/TUnit#5587 * docs: replace repeated benchmark link suffix with per-test descriptions by @thomhurst in thomhurst/TUnit#5588 * docs: clearer distributed tracing setup and troubleshooting by @thomhurst in thomhurst/TUnit#5597 * fix: auto-suppress ExecutionContext flow for hosted services (#5589) by @thomhurst in thomhurst/TUnit#5598 * feat: auto-align DistributedContextPropagator to W3C by @thomhurst in thomhurst/TUnit#5599 * feat: TUnit0064 analyzer + code fix for direct WebApplicationFactory inheritance by @thomhurst in thomhurst/TUnit#5601 * feat: auto-propagate test trace context through IHttpClientFactory by @thomhurst in thomhurst/TUnit#5603 * feat: TUnit.OpenTelemetry zero-config tracing package by @thomhurst in thomhurst/TUnit#5602 * fix: restore [Obsolete] members removed in v1.27 (#5539) by @thomhurst in thomhurst/TUnit#5605 * feat: generalize OTLP receiver for use outside TUnit.Aspire by @thomhurst in thomhurst/TUnit#5606 * feat: auto-configure OpenTelemetry in TestWebApplicationFactory SUT by @thomhurst in thomhurst/TUnit#5607 ### Dependencies * chore(deps): update tunit to 1.35.2 by @thomhurst in thomhurst/TUnit#5581 * chore(deps): update dependency typescript to ~6.0.3 by @thomhurst in thomhurst/TUnit#5582 * chore(deps): update dependency coverlet.collector to v10 by @thomhurst in thomhurst/TUnit#5600 **Full Changelog**: thomhurst/TUnit@v1.35.2...v1.36.0 Commits viewable in [compare view](thomhurst/TUnit@v1.35.2...v1.37.10). </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>
Summary
traceIdinTraceRegistry(for OTLP cross-process log correlation).HtmlReporterthen read that same ID back as an "additional" trace, so the primary trace rendered twice — once as Trace Timeline (descendants of the test case span, flattenstest body) and once as Linked Trace: TUnit (raw full trace, includestest body). Near-duplicate with a subtle difference, confusing for users.HtmlReporter.FilterAdditionalTraceIdsto strip the primarytraceIdfrom theTraceRegistry.GetTraceIdsresult. Linked Traces now appear only when the user callsTestContext.RegisterTracewith a genuinely different trace, matching the documented behavior indocs/docs/guides/html-report.md.Test plan
HtmlReporterTestscover: case-insensitive match, null primary, no match (returns same array), only primary (returns empty).HtmlReporterTestspass (4 new + 5 existing).