perf(engine): use SearchValues<char> for reporter filename sanitization

[thomhurst]

Summary

Implements #6035. Replaces the allocating string.Concat(name.Split(Path.GetInvalidFileNameChars())) pattern used to sanitize the entry-assembly name in HtmlReporter and JUnitReporter with a shared PathValidator.SanitizeFileName helper.

• net8.0+: single-pass SearchValues<char> scan into a stack (or heap, for names > 256 chars) buffer, with an allocation-free fast path that returns the original string unchanged when no invalid characters are present.
• netstandard2.0: keeps the existing Split implementation as a fallback (guarded with #if NET8_0_OR_GREATER), so all TFMs compile.

Behavior is identical (invalid filename characters are removed). The two previously-duplicated call sites now share one helper.

Testing

dotnet build TUnit.Engine/TUnit.Engine.csproj -c Release succeeds across all four TFMs (netstandard2.0, net8.0, net9.0, net10.0). The GitVersion MSBuild task fails in the worktree due to git state, so the build was run with -p:DisableGitVersionTask=true; the failure is unrelated to these code changes.

Closes #6035

[claude]

Code Review

Clean implementation overall. The SearchValues<char> approach is a well-known .NET pattern for this kind of hot-path sanitization and the fast-path (return original string unchanged when no invalid chars exist) is the right call. A few things worth discussing:

Missing Unit Tests

PathValidator.SanitizeFileName is a pure function with multiple distinct code paths: fast path (no invalid chars), stack-allocated slow path (≤256 chars), and heap-allocated slow path (>256 chars). None of these are covered by new tests.

Given the method now consolidates a security-adjacent responsibility (stripping characters that could affect file paths), I'd suggest adding tests for at least:

• A clean name → same reference returned (fast path)
• A name with embedded / or \ → chars removed
• An empty string
• A 257-char name with an invalid char (exercises the heap-alloc branch)

Doc Comment Overstates Security Guarantee

/// Removes any characters that are invalid in a file name (e.g. path separators),
/// preventing path traversal via crafted assembly names.

This is copied from the JUnit reporter's inline comment but is misleading as a method contract. SanitizeFileName removes Path.GetInvalidFileNameChars(), which on Linux is only ['/', '\0'] — it does not strip . so a name like ..malicious.. passes through unchanged. Full path traversal prevention is ValidateAndNormalizePath's job. The doc comment should say "strips characters that are invalid in a file name component" without the path-traversal framing, to avoid implying stronger guarantees.

ArrayPool for Large Names (Minor)

The heap-allocation branch (name.Length > 256) allocates a new char[name.Length] array that immediately becomes garbage. For assembly names this will never fire in practice, but for correctness/completeness the pattern ArrayPool<char>.Shared.Rent + try/finally return would eliminate that allocation entirely. That said, the current code is simpler and this case is effectively unreachable, so I wouldn't block on it.

Everything Else Looks Good

• The SearchValues<char> static field is correctly private static readonly and thread-safe.
• The ternary stackalloc char[name.Length] : new char[name.Length] correctly produces Span<char> in both branches — valid C# since the array is implicitly convertible.
• The #if NET8_0_OR_GREATER / netstandard2.0 fallback structure is the right approach for multi-TFM support.
• Consolidating the two previously-duplicated call sites into one helper is a worthwhile deduplication.

Main ask before merging: add unit tests and tighten the doc comment's security claim.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 7 complexity

Metric	Results
Complexity	7

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review — perf/issue-6035-searchvalues-reporters

Overall this is a solid, well-motivated PR: the SearchValues<char> fast path is correct, the buffer sizing is right (no off-by-one), the #else fallback preserves the original behavior exactly, and the callers are unchanged in semantics. The implementation itself is fine.

Three things worth addressing before merge:

---

1. Misleading docstring: "allocation-free" claim is false on netstandard2.0

File: TUnit.Engine/Helpers/PathValidator.cs, line 20

"allocation-free when the name is already clean"

This is only true for the #if NET8_0_OR_GREATER branch. The #else fallback always calls string.Concat(name.Split(Path.GetInvalidFileNameChars())), which allocates a char[] from GetInvalidFileNameChars(), a string[] from Split, and a new string from Concat — even when the name has no invalid characters.

TUnit.Engine ships a netstandard2.0 TFM, so consumers on that target will see the opposite of what the doc promises. The doc should either scope the claim (NET8+: allocation-free fast path; older TFMs: O(n) allocation) or the #else path should add its own fast-path check before calling Split.

---

2. The netstandard2.0 #else path has zero test coverage

File: TUnit.UnitTests/PathValidatorTests.cs

TestProject.props defaults all test projects to net10.0 only. The five new tests therefore exclusively exercise the NET8_0_OR_GREATER branch. A regression in string.Concat(name.Split(...)) would ship unnoticed.

The two reference-identity tests (SanitizeFileName_CleanName_ReturnsSameReference and SanitizeFileName_EmptyString_ReturnsSameReference) would also fail if the project were ever multi-targeted to include a pre-NET8 TFM, because string.Concat(new[]{"MyAssembly.Tests"}) allocates a new string object — IsSameReferenceAs would throw. The tests lock in an implementation detail that only holds on one branch.

Suggested fix: Either add a test that explicitly covers the #else branch's contract (output equality, not reference identity), or replace IsSameReferenceAs with a value-equality assertion that holds on all TFMs:

// Instead of: await Assert.That(result).IsSameReferenceAs(name);
await Assert.That(result).IsEqualTo(name); // correct on all TFMs

---

3. SanitizeFileName_StripsPathSeparators assertion is too weak

File: TUnit.UnitTests/PathValidatorTests.cs, line 33

var result = PathValidator.SanitizeFileName("foo/bar");
await Assert.That(result).DoesNotContain("/");

A broken implementation that returns "" (or "fo") would satisfy DoesNotContain("/"). The test should verify the full output:

await Assert.That(result).IsEqualTo("foobar");

---

Minor / informational

• Null input divergence (latent): SanitizeFileName(null) silently returns null on NET8+ (empty span hits the fast path, returning the original name which is null) but throws NullReferenceException on netstandard2.0. Both current callers guard with ?? "TestResults", so this can't fire today — but a future caller could hit different behaviour on different TFMs with no indication from the method signature. A single ArgumentNullException.ThrowIfNull(name) at the top of the method would make the contract explicit and consistent.

• The stackalloc char[name.Length] at the 256-char threshold is reasonable (512 bytes), and AOT/SearchValues compatibility is correct — no issues there.