feat(html-report): omit redundant test body span from trace timeline

[thomhurst]

Summary

• When a test has no before/after hooks, the "test body" span is the only child of the test case span and adds no useful information to the trace timeline — omit it
• If the test body has child spans (e.g. HTTP calls, DB queries), those are re-parented directly under the test case span so they remain visible
• Add SpanTestBody constant to TUnitActivitySource alongside the other span name constants, eliminating the magic string in TestExecutor

Test plan

• Run a test with no hooks — verify the trace timeline is hidden (no redundant "test body" bar)
• Run a test with before/after hooks — verify all spans (hooks + test body) still appear
• Run a test whose body produces child spans (e.g. HttpClient activity) — verify child spans appear directly under the test case span

[claude]

Code Review

Overall this is a clean, focused change with a sensible UX improvement. The constant extraction and re-parenting logic are both well-designed. One architectural concern worth addressing before merge.

---

Hardcoded string in JS creates silent coupling risk

File: TUnit.Engine/Reporters/Html/HtmlReportGenerator.cs, the new JS block

The PR correctly extracts SpanTestBody as a C# constant to eliminate magic strings in TestExecutor.cs, but the JavaScript comparison still uses a hardcoded literal:

if (directChildren.length === 1 && directChildren[0].name === 'test body') {

If TUnitActivitySource.SpanTestBody is ever renamed (e.g., to "test-body" or "body"), the JS will silently stop matching — the if condition never fires, and the redundant span is no longer removed. There's no compiler error, no test failure, just a quietly broken report.

Since this is an embedded JS template generated from C#, injecting the constant via string interpolation eliminates the coupling entirely:

// in the C# string that builds the JS template:
$"""
if (directChildren.length === 1 && directChildren[0].name === '{TUnitActivitySource.SpanTestBody}') {{
"""

Why this is better: the constant and the JS are guaranteed to stay in sync. If SpanTestBody changes, the JS automatically changes with it.

---

The <= 1 threshold may conflict with the test plan's intent

Line 1497: if (sp.length <= 1) return '';

The test plan says:

Run a test whose body produces child spans (e.g. HttpClient activity) — verify child spans appear directly under the test case span

But with this check, a test body with exactly one child span (one HTTP call) will be silently hidden after re-parenting — sp.length becomes 1, which returns ''.

I see that renderSuiteTrace uses the same <= 1 pattern (line 1575), so this is consistent with existing convention. If the intent is "a single bar on a timeline is not useful enough to show", that's a reasonable product decision — but it's worth making that explicit, since the PR description implies single child spans will remain visible. Either the test plan item 3 should clarify "two or more child spans", or the threshold should be < 1.

---

Minor: Object.assign vs spread

Object.assign({}, s, {parentSpanId: rootSpanId})

The rest of the JS uses modern syntax (arrow functions, spread in [...sp]). The spread equivalent {...s, parentSpanId: rootSpanId} is slightly more idiomatic for immutable updates, though this is low priority.

---

What's good

• The constant extraction is the right call — the other span names (SpanTestCase, SpanTestSuite, etc.) are all constants, so SpanTestBody belongs with them.
• The re-parenting logic is correct: filtering out the test body span and re-mapping parentSpanId on its children preserves the full subtree while flattening one unnecessary level.
• The guard directChildren.length === 1 correctly limits the optimization to the "no hooks" case — if before/after hook spans are present as siblings, the test body span is kept.
• The change is fully contained to three files with a clear, single responsibility per change.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity

Metric	Results
Complexity	0

View in Codacy

_{TIP This summary will be updated as you push new changes. Give us feedback}

[thomhurst]

Addressed the review findings:

1. Hardcoded string coupling — The JS is a raw string literal ("""), so C# interpolation would require escaping hundreds of {/} across ~900 lines of JS — not practical. Added a comment linking the JS literal to TUnitActivitySource.SpanTestBody so the coupling is documented. (The same coupling already exists for 'test case' on line 1566.)

2. <= 1 threshold — This is actually fine as-is. getDescendants includes the root span itself, so after removing test body from a test with 1 child span: sp = [testCase, httpChild] → length 2 → the trace IS shown. The <= 1 only hides the timeline when only the bare test case span remains with no children at all.

3. Object.assign vs spread — Fixed to {...s, parentSpanId: rootSpanId} for consistency with the rest of the JS.

[claude]

Follow-up Review

All three concerns from the previous review have been addressed in the follow-up commit.

1. Hardcoded string coupling — The added comment // Note: 'test body' must match TUnitActivitySource.SpanTestBody in C#. is the right pragmatic call. The alternative (C# string interpolation) would require escaping hundreds of {} across ~900 lines of embedded JS — a maintenance burden that outweighs the benefit. Documenting the coupling in-place is a clean solution.

2. <= 1 threshold — The author's clarification is correct. getDescendants includes the root span itself in the returned list, so after stripping the test body, a test with one HTTP child span yields [testCase, httpChild] → length 2 → timeline is shown. The <= 1 guard only hides the timeline when no useful child spans exist at all. My reading of this was wrong.

3. Spread syntax — Fixed; {...s, parentSpanId: rootSpanId} is now consistent with the rest of the JS.

The change is clean, the logic is sound, and all feedback has been incorporated. 🚢

[claude]

Final Review

The current state of the PR incorporates all feedback from the previous two review rounds. Everything looks good.

Changes verified:

• Constant extraction (SpanTestBody) — magic string in TestExecutor.cs replaced with TUnitActivitySource.SpanTestBody. All span name constants are now co-located in TUnitActivitySource.
• JS string coupling — documented with // 'test body' must match TUnitActivitySource.SpanTestBody in C#, the right pragmatic trade-off given the ~900 lines of embedded JS that would require escaping if C# interpolation were used.
• Spread syntax — {...s, parentSpanId: rootSpanId} is consistent with the rest of the JS codebase.
• <= 1 guard — correct as written. getDescendants includes the root span, so after stripping the test-body span a test with one child span yields length 2 and is shown; the guard only hides the timeline when no useful child spans exist at all.
• Re-parenting logic — filtering the test-body span out and remapping parentSpanId on its direct children is the correct structural transform. The directChildren.length === 1 guard correctly limits the optimization to the no-hooks case.

The change is clean, well-scoped, and the three commits tell a clear story. 🚢