Feature/mtp test adapter 2803

[sheddy123]

#2803
@timcassell

Introduce BenchmarkTestFramework to integrate BenchmarkDotNet with Microsoft.Testing.Platform, enabling benchmark discovery and execution. Implements session management, filtering, event processing, and output routing. Handles test node updates and cancellation, with support for experimental platform features.
Added BenchmarkDotNet.TestAdapter.TestingPlatform and BenchmarkDotNet.IntegrationTests.TestingPlatform projects to the solution. Updated MonoBenchmarks and SharedDiagnosers integration test projects to only build for the solution in Debug configuration.

[timcassell]

Let's name it BenchmarkDotNet.TestingPlatform.

Updated MonoBenchmarks and SharedDiagnosers integration test projects to only build for the solution in Debug configuration.

Why? We run tests in Release configuration.

[timcassell]

ParameterValueDisposer.cs:41 — misses IAsyncDisposable. #3248 has since merged, so a parameter value from an async [ParamsSource]/[ArgumentsSource] can be IAsyncDisposable-only; .OfType<IDisposable>() drops it and it reaches the finalizer — the #1383 hang this class exists to prevent. ParameterInstance is now itself IAsyncDisposable, and its Dispose/DisposeAsync already implement the prefer-sync-then-pump policy, so dispose through it rather than casting the value. DisposeHelper.DisposeAllAsync is internal but BenchmarkDotNet.TestAdapter is already in InternalsVisibleTo, and it aggregates per-value exceptions, which also fixes the missing try/catch below. Suggested replacement (merged with current master, builds clean on netstandard2.0 and net462):

using BenchmarkDotNet.Engines;
using BenchmarkDotNet.Helpers;
using BenchmarkDotNet.Parameters;
using BenchmarkDotNet.Running;
using System.Runtime.CompilerServices;

namespace BenchmarkDotNet.TestAdapter
{
    internal static class ParameterValueDisposer
    {
        /// <inheritdoc cref="DisposeUnusedAsync"/>
        internal static void DisposeUnused(IEnumerable<BenchmarkCase> enumerated, IEnumerable<BenchmarkCase> retained)
        {
            using var context = BenchmarkSynchronizationContext.CreateAndSetCurrent();
            context.ExecuteUntilComplete(DisposeUnusedAsync(enumerated, retained));
        }

        /// <remarks>
        /// The values are matched by reference instead of being disposed case by case, because BenchmarkConverter
        /// gives the same ParameterInstance to every job and every argument set of a benchmark: disposing a dropped
        /// case wholesale would take down values that a benchmark which is about to run still owns. Disposal goes
        /// through ParameterInstance so that a value which is only IAsyncDisposable is disposed as well.
        /// </remarks>
        internal static ValueTask DisposeUnusedAsync(IEnumerable<BenchmarkCase> enumerated, IEnumerable<BenchmarkCase> retained)
        {
            var unused = new Dictionary<object, ParameterInstance>(ReferenceComparer.Instance);

            foreach (var parameter in GetDisposableParameters(enumerated))
                unused[parameter.Value!] = parameter;

            foreach (var parameter in GetDisposableParameters(retained))
                unused.Remove(parameter.Value!);

            return unused.Values.DisposeAllAsync();
        }

        private static IEnumerable<ParameterInstance> GetDisposableParameters(IEnumerable<BenchmarkCase> benchmarkCases)
            => benchmarkCases
                .SelectMany(benchmarkCase => benchmarkCase.Parameters.Items)
                .Where(parameter => parameter.Value is IDisposable or IAsyncDisposable);

        /// <summary>
        /// Compares by reference, so that a parameter value which overrides Equals is still disposed once per instance.
        /// </summary>
        private sealed class ReferenceComparer : IEqualityComparer<object>
        {
            public static readonly ReferenceComparer Instance = new ReferenceComparer();

            public new bool Equals(object? x, object? y) => ReferenceEquals(x, y);

            public int GetHashCode(object obj) => RuntimeHelpers.GetHashCode(obj);
        }
    }
}

Both MTP call sites are already async, so they can take the async form:

// BenchmarkTestFramework.DiscoverAsync
await ParameterValueDisposer.DisposeUnusedAsync(enumeration.All.SelectMany(runInfo => runInfo.BenchmarksCases), []).ConfigureAwait(false);

// BenchmarkTestFramework.RunAsync
await ParameterValueDisposer.DisposeUnusedAsync(
    enumeration.All.SelectMany(runInfo => runInfo.BenchmarksCases),
    runnable.Select(match => match.Node.BenchmarkCase)).ConfigureAwait(false);

BenchmarkEnumerator.GetBenchmarksFromAssembly:81 keeps the sync entry point; CreateAndSetCurrent throws if a pump is already current, but there isn't one there since each BenchmarkConverter.TypeToBenchmarks creates and disposes its own. Worth adding an async-source probe whose value is IAsyncDisposable-only — DisposableProbe/SharedValueProbe are all sync, so nothing in the suite would catch this.

ParameterValueDisposer.cs:37 — one throwing Dispose() aborts the loop. No per-value try/catch: everything after the throw is left undisposed, and from BenchmarkEnumerator.GetBenchmarksFromAssembly:81 it propagates out of enumeration so no benchmarks are returned at all. Fixed by DisposeAllAsync above.

TestingPlatform/BenchmarkTestFramework.cs:107 — discovery disposes values a later run still uses. MTP server mode (VS / VS Code Test Explorer) serves discoverTests and runTests from the same process. The run re-enumerates, but a source returning cached instances — a static field, or a property over a readonly array, the pattern the probes here use — hands back the same objects, now disposed, and the run executes against them. The Discover/Run test helpers each spawn a fresh process, so nothing in the suite covers it.

Helpers/GenericBenchmarksBuilder.cs:51 — swallows attribute errors on the CLI path. The new blanket catch (Exception) also covers BenchmarkSwitcher: [Config(typeof(SomeAbstractConfig))] used to surface the ConfigAttribute ctor exception, now the type is silently dropped (Running/TypeFilter.cs:45) and the user gets "No benchmarks were found." GenericBenchmarksValidator can't report built.Error because it needs at least one surviving benchmark.

TestingPlatform/BenchmarkEventProcessor.cs:131 — a crashed run reports as Skipped. PublishOutstandingResults picks the state from pending.GetErrorMessage() alone, so a node where OnStartRunBenchmark fired but OnEndRunBenchmark never did (e.g. BenchmarkRunnerClean.Run throwing on the Operations == 0 path) looks skipped. A node with a StartTime and no result should be Failed.

TestingPlatform/BenchmarkTestFramework.cs:332 — Matches's _ => true fallback. Any ITestExecutionFilter type the platform adds later is treated as "match everything", so the whole assembly runs instead of the requested subset — a wrong result rather than a visible failure. Better to fail (or at least log) on an unrecognised filter.

TestingPlatform/BenchmarkTestNode.cs:99 — dead null guard. BenchmarkAttribute.SourceCodeFile is a non-nullable string defaulting to "", so != null always passes and an attribute built without caller info publishes TestFileLocationProperty("") at line 0. Use !string.IsNullOrEmpty(...).

🤖 Reviewed with Claude Code

[timcassell]

Follow-up on the latest push. The IAsyncDisposable gap, the per-value exception handling, the Skipped/Failed state and the SourceCodeFile guard all look closed; six things on the new code, inline.

🤖 Reviewed with Claude Code

[timcassell]

Follow-up on the latest push. The lifetime rework does close the value-loss windows for the sequential discover→run→discover case (traced against the new FreshValueProbe for both cached and per-read sources), and the TypeFilter rework reads as behaviour-preserving apart from the intended change. Seven things inline, two worth acting on before merge.

🤖 Reviewed with Claude Code

[timcassell]

Everything from the last round looks closed: the per-request RequestScope holds up under overlapping requests (discovery-during-run, run-during-run, cached and per-read sources), HasEnumerated covers the request that dies before it enumerates, the validation dedup collapses exactly the case-less assembly-wide duplicates, and the filter fallback is split the right way. Three small things inline.

🤖 Reviewed with Claude Code

[timcassell]

The scopes refactor dropped the exit-time safety net. This used to drain held.Concat(inFlight), with the comment that "a value that somehow is [in flight] would otherwise be leaked for good"; now that a request's values live in a RequestScope the lifetime object has no reference to, nothing takes its place.

Server mode, client sends exit (or the IDE cancels) while a run request's finally has not run yet: AfterRunAsync drains held, then the request's CompleteAsync sets held = enumerated, and those values are never disposed — the #1383 finalizer case this class exists to prevent.

Registering the scope in BeginRequest, removing it in CompleteAsync, and draining the still-live ones here restores the guarantee the old Concat gave.

[timcassell]

Neither half of the split is covered: nothing tests the discovery warning and nothing tests this refusal. Since the branch is unreachable with MTP 2.3.3 — NopFilter, TestNodeUidListFilter and TreeNodeFilter are the only implementations, and there's no composite — a regression here would be silent. A fake ITestExecutionFilter fed through the framework pins both paths.

Worth checking what the platform sees when it does fire: this escapes ExecuteRequestAsync after its finally has already called context.Complete(), and no test node has been published, so the request is reported finished before the failure is observed and an IDE gets a run with no per-test feedback. Publishing the nodes as failed (or erroring before Complete) would say it in a place the user can read.

[timcassell]

ran.Count counts nodes, but the message says "benchmark types" and the dedup under test only does anything when two or more BenchmarkRunInfos are validated. "Probe.Identity" currently selects 8 nodes across 4 types, so the test does fail without the fix — but a later probe change leaving 4+ nodes on a single type would make it pass without exercising the dedup at all.

Asserting on distinct type prefixes (>= 2 distinct types) keeps it honest regardless of how the probes evolve.

[sheddy123]

@timcassell the changes have been effected

[timcassell]

@timcassell the changes have been effected

You missed 3 comments.