fix: re-enable RPC tests and modernize harness (#5540)

[thomhurst]

Summary

• Root cause fix: RunTestsRequest was serializing the node filter as testCases, but Microsoft.Testing.Platform reads it from tests — so the server silently ignored the filter and ran the full catalogue on every RunTests call. Renamed the JSON property to match the protocol wire format.
• Pipeline TFM: RunRpcTestsModule targeted net8.0 while TUnit.RpcTests.csproj only targets net10.0, leaving the suite skipped. Aligned the testable framework list to net10.0.
• Modernization: extracted TestHostSession (IAsyncDisposable) and TestProjectBuilds (per-framework shared build cache), parameterized the three scenarios across net8.0/net10.0 via MethodDataSource, removed dead abstractions (IProcessHandle/ProcessHandle, custom LogLevel enum, ConsoleRpcListener, RunRequest), switched to Microsoft.Extensions.Logging.LogLevel with a string-enum converter, and tightened TestNode deserialization via JsonExtensionData.

Closes #5540.

Test plan

• Discovery_ReturnsFullTestCatalogue passes on net8.0 and net10.0
• RunTests_WithUidFilter_ExecutesOnlySelectedTests passes on net8.0 and net10.0 — confirms server honors the filter
• RunTests_WithSkippedTest_ReportsSkippedState passes on net8.0 and net10.0
• Full local run: 6/6 green in ~2m22s
• dotnet build TUnit.RpcTests clean (0 warnings, 0 errors)

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
ErrorProne	1 medium

View in Codacy

🟢 Metrics 9 complexity

Metric	Results
Complexity	9

View in Codacy

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

[claude]

Code Review

This PR is a solid fix — the root cause ("testCases" → "tests" wire-format mismatch) is clear and correct, and the refactoring around it is well-motivated.

Core Fix ✅

RunTestsRequest.cs — The JSON property rename from testCases to tests is the crux of the fix. The protocol was silently ignoring the filter because the server read a field the client wasn't sending under that name. Correct.

RunRpcTestsModule.cs — Re-enabling net10.0 to match the project target is correct. No concerns here.

---

Architecture: TestHostSession ✅

Extracting session lifecycle into an IAsyncDisposable is the right abstraction. The private constructor + static factory (StartAsync) pattern is idiomatic for types that require async initialization. Good.

One issue in DisposeAsync: Client.Dispose() almost certainly disposes _tcpClient internally (since TestingPlatformClient still holds a reference to it), and then _tcpClient.Dispose() is called a second time. TcpClient tolerates double-dispose, but it is a code smell. Consider having TestingPlatformClient.Dispose() only dispose the JsonRpc/stream, and let TestHostSession own _tcpClient disposal exclusively. Similarly, _rpc.Dispose() is called after Client.Dispose() — if TestingPlatformClient disposes the JsonRpc it receives, that is another double-dispose.

---

TestProjectBuilds.EnsureBuiltAsync — Stale-failure risk

public static Task EnsureBuiltAsync(string framework, CancellationToken cancellationToken)
    => _builds.GetOrAdd(framework, tfm => BuildAsync(tfm, cancellationToken));

If the first caller is cancelled (or the build fails transiently), the faulted/cancelled Task is permanently cached in _builds. Every subsequent caller for that TFM will immediately receive the same bad task instead of retrying. In a flaky CI environment this could permanently poison the test run. A safer pattern:

public static Task EnsureBuiltAsync(string framework, CancellationToken cancellationToken)
{
    if (_builds.TryGetValue(framework, out var existing) && !existing.IsFaulted && !existing.IsCanceled)
        return existing;
    var fresh = BuildAsync(framework, cancellationToken);
    _builds[framework] = fresh;
    return fresh;
}

There is also a subtle race: GetOrAdd is not atomic — two threads may both call BuildAsync for the same TFM simultaneously. For a test helper this is probably harmless (both builds succeed), but worth noting.

---

Frameworks() Return Type

public static IEnumerable<Func<string>> Frameworks()
{
    yield return () => "net8.0";
    yield return () => "net10.0";
}

The test methods accept string framework, not Func<string>. If TUnit's MethodDataSource transparently invokes the Func<T> wrapper to materialize the argument, this works — but it is not obvious from reading the code why wrapping in Func<T> is needed here. If TUnit supports IEnumerable<string> directly from a MethodDataSource, prefer that for clarity. If the Func<string> wrapper is load-bearing (e.g., deferred evaluation), a comment explaining why would help future readers.

---

ExitAsync Silent Swallow

catch
{
    // Best effort — connection may already be gone
}

Swallowing all exceptions unconditionally can hide bugs during development. Since this is called in DisposeAsync, consider at least logging to Console.Error or Trace so it is visible in test output without being fatal.

---

TestNode.JsonExtensionData ✅

Using [JsonExtensionData] to round-trip unknown fields back to the server when constructing RunTestsRequest is exactly right. This avoids the common trap of stripping server-generated fields (traits, location info, etc.) that the server expects to see back on runTests.

---

Dead Code Removal ✅

Removing IProcessHandle/ProcessHandle (which had a NotImplementedException in StopAsync and no-ops everywhere else), ConsoleRpcListener, the duplicate LogLevel enum, and RunRequest are all correct.

---

Minor: Magic number

await Assert.That(discovered).Count().IsGreaterThanOrEqualTo(4000);

This will fail if the test project's test count drops below 4000. Consider a named constant or a comment explaining where this threshold comes from.

---

Summary: The fix is correct and the refactoring is a clear improvement. The two issues worth addressing before merge are the stale-task caching in TestProjectBuilds (can permanently break a CI run after a transient failure) and the potential double-dispose in TestHostSession. Everything else is minor.