Skip to content

Feature/mtp test adapter 2803 - #3229

Open
sheddy123 wants to merge 126 commits into
dotnet:masterfrom
sheddy123:feature/mtp-test-adapter-2803
Open

sheddy123 wants to merge 126 commits into
dotnet:masterfrom
sheddy123:feature/mtp-test-adapter-2803

Conversation

@sheddy123

Copy link
Copy Markdown
Contributor

#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.

Added a new guide for running benchmarks with Microsoft.Testing.Platform (MTP), covering setup, usage, and caveats. Updated the table of contents to include the new page and added a note to the VSTest docs about the MTP adapter option.
Added InternalsVisibleTo attribute in AssemblyInfo.cs to expose internal members to the BenchmarkDotNet.TestAdapter.TestingPlatform assembly, ensuring it uses the same public key as other related assemblies.
Deleted the internal static method GetUnrandomizedJobDisplayInfo from BenchmarkCaseExtensions.cs. This method handled normalization of job display info by removing randomness from job IDs for consistent benchmark referencing. No other code changes were made.
Introduced BenchmarkCaseIdentityExtensions with GetUnrandomizedJobDisplayInfo to normalize Job DisplayInfo by removing random ID components. This ensures consistent benchmark identification across processes for test adapters.
Introduce GetBenchmarksFromAssembly to extract benchmarks from an already loaded Assembly. Refactor existing logic to use this method, improving code reuse and enabling benchmark retrieval from both loaded assemblies and file paths.
Add MSBuild props to enable TestingPlatform integration, set defaults for `dotnet test` compatibility, disable parallel TFM runs, and auto-register BenchmarkDotNet builder hook.
Introduced AsyncWorkQueue, an internal sealed class in BenchmarkDotNet.TestAdapter.TestingPlatform. It enables ordered, thread-safe queuing of asynchronous work items, allowing synchronous producers and asynchronous consumers. Utilizes ConcurrentQueue and SemaphoreSlim, supports completion signaling, and implements IDisposable for resource cleanup.
Created a new .csproj targeting netstandard2.0 for the TestingPlatform adapter. Configured project metadata, packaging, and references. Integrated Microsoft.Testing.Platform.MSBuild and BenchmarkDotNet, and linked shared source files for benchmark enumeration. Set IsTestingPlatformApplication to false to avoid test app behavior.
Introduced BenchmarkDotNetExtension class implementing IExtension to provide extension metadata and enablement status for Microsoft.Testing.Platform integration.
Introduced BenchmarkEventProcessor to process BenchmarkDotNet events and translate them into test node updates for the testing platform. Handles validation errors, build results, benchmark execution, and ensures all benchmarks have published results. Includes logic for error aggregation, output formatting, and timing information.
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.
Introduced the internal sealed class BenchmarkTestNode to encapsulate immutable BenchmarkCase data for Microsoft.Testing.Platform integration. This includes stable UID generation, display name and path construction, property management, and support for test filtering and message bus conversion.
Introduced OutputDeviceLogger class implementing ILogger to forward BenchmarkDotNet logs to the platform output device. Handles log kinds, buffers lines, and asynchronously displays output to ensure build progress and results are visible in test run output.
Introduce TestApplicationBuilderExtensions with AddBenchmarkDotNet methods for integrating BenchmarkDotNet benchmarks into Microsoft.Testing.Platform. Includes overloads for entry assembly and specific assemblies, null checks, test framework registration, and tree node filter service support.
Introduced a static TestingPlatformBuilderHook class in the BenchmarkDotNet.TestAdapter.TestingPlatform namespace. This class provides an AddExtensions method to register BenchmarkDotNet with the test application builder, intended for use by generated code and hidden from IntelliSense.
Added a "test" section to global.json to specify "Microsoft.Testing.Platform" as the test runner. This configures the project to use the designated testing platform.
Introduce BenchmarkDotNet.IntegrationTests.TestingPlatform.csproj targeting net10.0 as an executable. The project includes assembly metadata, references BenchmarkDotNet.TestAdapter.TestingPlatform, manually imports its build props, and uses shared common.props and common.targets for build configuration.
Introduced SampleBenchmarks class in BenchmarkDotNet.IntegrationTests.TestingPlatform. Defines Add and Multiply benchmarks with parameterized Size, categorized as "Fast" and "Slow". Uses a custom FastConfig to run benchmarks in-process with a single dry iteration for quick end-to-end testing.
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.
Explicitly set BenchmarkDotNet.TestAdapter.TestingPlatform and BenchmarkDotNet.IntegrationTests.TestingPlatform to not build in the Debug configuration by adding <Build Solution="Debug|*" Project="false" /> in BenchmarkDotNet.slnx. No other changes made.
@timcassell

Copy link
Copy Markdown
Collaborator

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.

Comment thread src/BenchmarkDotNet.TestAdapter.TestingPlatform/BenchmarkTestNode.cs Outdated
- Correct NuGet package and namespace in documentation
- Add GetBenchmarkUid for stable benchmark identification
- Change namespace in BenchmarkCaseIdentityExtensions
- Update InternalsVisibleTo for TestingPlatform assembly
Deleted all source, project, and props files from BenchmarkDotNet.TestAdapter.TestingPlatform. This removes all implementation and integration for running benchmarks as tests via Microsoft.Testing.Platform, including test discovery, execution, and result processing logic.
Add BenchmarkDotNet.TestingPlatform.props to enable seamless integration with Microsoft.Testing.Platform. This includes setting required properties for Testing Platform application behavior, ensuring `dotnet test` compatibility on older SDKs, disabling parallel test execution for multi-targeted projects by default, and registering BenchmarkDotNet as a builder hook.
Introduced AsyncWorkQueue in BenchmarkDotNet.TestingPlatform to enable thread-safe, ordered queuing of asynchronous work items. Supports synchronous enqueuing, asynchronous draining, completion signaling, and resource disposal using ConcurrentQueue and SemaphoreSlim.
Introduce a new project to integrate BenchmarkDotNet with Microsoft.Testing.Platform, enabling benchmarks to be discovered and executed as tests. Implements extension identification, test framework, event processing, test node representation, and output logging. Provides builder extensions for easy registration and an MSBuild hook for automatic integration. Updates project configuration for packaging and dependencies.
Updated BenchmarkDotNet.IntegrationTests.TestingPlatform.csproj to reference BenchmarkDotNet.TestingPlatform instead of BenchmarkDotNet.TestAdapter.TestingPlatform. Adjusted both the ProjectReference and Import paths accordingly.
- Use ExceptionDispatchInfo to capture/rethrow exceptions during async disposal, ensuring cleanup and event signaling always occur
- Update artifacts cleanup log message for consistency
- Prevent duplicate ValidationError entries by tracking errors in a HashSet during validation
Added AnAssemblyWideValidationWarningIsReportedOncePerNode test to ensure assembly-wide validation warnings are reported once per node. Updated ServerNode in TestingPlatformServerModeSession.cs to include StandardOutput property and populated it from node JSON data.
Refactored parameter value lifetime management by introducing a RequestScope class to encapsulate parameter values per request, improving isolation and handling of overlapping requests. Moved tracking, hiding, and completion logic into RequestScope and updated BenchmarkTestFramework to use the new scope. Updated disposal logic to ensure correct cleanup and clarified comments. Improved handling of unrecognized filters: discovery lists all benchmarks, run requests reject unsupported filters.

@timcassell timcassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread tests/BenchmarkDotNet.IntegrationTests/TestingPlatformAdapterTests.cs Outdated
Comment thread src/BenchmarkDotNet/Running/BenchmarkRunnerClean.cs Outdated
Refactored post-benchmark cleanup to remove ExceptionDispatchInfo usage. Cleanup and event notification now occur in a finally block, guaranteeing artifact removal and stage completion even if disposal throws, simplifying error handling and ensuring consistent resource management.
@sheddy123

Copy link
Copy Markdown
Contributor Author

@timcassell the changes have been effected

@timcassell

Copy link
Copy Markdown
Collaborator

@timcassell the changes have been effected

You missed 3 comments.

Add InternalsVisibleTo for BenchmarkDotNet.IntegrationTests.TestingPlatform.Internals using BenchmarkDotNetInfo.PublicKey. This allows direct testing of Microsoft.Testing.Platform internals, supporting scenarios like test execution filters and incomplete application requests not accessible via a real test host. Added comments to clarify intent.
Refactored BenchmarkTestFramework to mark benchmarks as failed with clear messages for unrecognized filters, enhancing IDE feedback. Updated ParameterValueLifetime to track live request scopes, ensuring all parameter values are disposed properly and preventing resource leaks. Added comments to clarify resource management and error handling changes.
- Added project reference to BenchmarkDotNet.IntegrationTests.TestingPlatform.Internals.
- Introduced `InternalsProbe` constant in `TestingPlatformAdapterTests.cs`.
- Improved test to count distinct benchmark types.
- Added tests for unrecognized filters and parameter disposal.
- Implemented `RunInternalsProbe` helper and `InternalsReport` record for probe execution and parsing.
Introduce a new project to test BenchmarkDotNet's Microsoft.Testing.Platform integration. Includes benchmarks for resource cleanup and filtering, a platform stub for simulation, and a test orchestrator. Adds project file and documentation detailing test scenarios not possible from a real test host.
Added BenchmarkDotNet.IntegrationTests.TestingPlatform.Internals project to the solution file for improved test coverage and integration.

@timcassell timcassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the internals probe. PlatformStub checks out against Microsoft.Testing.Platform 2.3.3 - every reflected member exists with the shape it assumes - the InternalsVisibleTo is sound, and the BenchmarkRunnerClean rewrite is behaviour-preserving, including that it still loses a dispose failure if cleanup itself throws. Four things inline, one of them a consequence of the live-scope sweep I asked for.

🤖 Reviewed with Claude Code

Comment thread src/BenchmarkDotNet.TestAdapter/TestingPlatform/ParameterValueLifetime.cs Outdated
Comment thread tests/BenchmarkDotNet.IntegrationTests/TestingPlatformAdapterTests.cs Outdated
A volatile flag tracks BenchmarkDotNet's run stage to ensure parameter values are disposed by the correct owner. The adapter now hands over ownership explicitly, preventing double or premature disposal. Comments, methods, and error handling are enhanced for clarity and robustness.
Introduce HandedOverBenchmarks to test disposal of parameters handed to BenchmarkDotNet, including a disposable type and fast in-process config.
Update Program.cs to add "handed" and "empty" report sections, implement ReportHandedOverRequestAsync, and refactor ExecuteAsync for flexible assembly input.
Expand README.md with documentation for new scenarios, clarifying disposal distinctions and handling of assemblies with no benchmarks.
Extend TestingPlatformAdapterTests with cases for unrecognized filters and parameter value disposal. Update InternalsReport with Handed and Empty sections, and enhance RunInternalsProbe to parse them. Add assertions and comments for clarity.

@timcassell timcassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the ownership hand-off. The rule is the right one and the takenOver set does what the comment says; three things inline, the first a reorder that quietly undoes the previous round's fix.

🤖 Reviewed with Claude Code

Comment thread src/BenchmarkDotNet.TestAdapter/TestingPlatform/ParameterValueLifetime.cs Outdated
Comment thread src/BenchmarkDotNet.TestAdapter/TestingPlatform/BenchmarkTestFramework.cs Outdated
Comment thread tests/BenchmarkDotNet.IntegrationTests.TestingPlatform.Internals/Program.cs Outdated
Simplify ownership transfer to BenchmarkDotNet by removing volatile runStageStarted and related logic. Introduce TakeBack method to reclaim values if disposal stage is not reached. Update comments and documentation to clarify disposal responsibilities and prevent premature or duplicate disposal, addressing hangs and ObjectDisposedExceptions. Ensure adapter does not dispose values already handled by BenchmarkDotNet.
Introduce AlreadyDisposedBenchmarks and TakenBackBenchmarks to test disposal handling in various request lifecycles. Add reporting methods in Program.cs for new scenarios and update the main method to print their results. Revise README.md to document the three request states and disposal behaviors.
Added three new tests to verify parameter value disposal behavior in TestingPlatformAdapterTests.cs. Enhanced RunInternalsProbe to process "== taken-back" and "== already-disposed" output sections, updating assertions accordingly. Modified InternalsReport to include TakenBack and AlreadyDisposed arrays.

@timcassell timcassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the hand-over rework. The combined filter closes the double dispose from last round and both new probes check what they claim; one ordering problem inline.

🤖 Reviewed with Claude Code

Update BenchmarkEventProcessor to check buildResult.Exception and clarify error messages. Refactor ParameterValueLifetime to track disposed values with a new HashSet, ensuring proper disposal when app ends with in-flight requests. Add comments explaining updated disposal and resource management logic.
Updated the comment in ParameterValuesBenchmarkDotNetHandedBackAreDisposedWhenTheApplicationEnds to clarify disposal timing and ownership when runs do not reach the run stage. The new comment explains that parameter values are not disposed immediately in such cases, but are instead handled after the sweep, referencing BenchmarkDotNet issue dotnet#1383. No code logic was changed.
Removed NoopGenerator and its usage in FailingBuildToolchain. FailingBuildToolchain now directly uses FailingBuilder and UnreachableExecutor. Updated FailingBuilder to implement GetSupportsConcurrency (returns true) and changed BuildAsync signature to match the new interface, returning a failed BuildResult with ArtifactsPaths.Empty.
Updated XML documentation to clarify parameter handover/take-back scenarios and disposal responsibilities. Ensured proper disposal tracking by adjusting CompleteAsync and TakeBack call order in request handlers, with improved comments on disposal sequence and lifecycle.

@timcassell timcassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking on the latest push: the post-sweep completion path closes the leak from last round. Two nits inline.

🤖 Reviewed with Claude Code

Comment thread src/BenchmarkDotNet.TestAdapter/TestingPlatform/ParameterValueLifetime.cs Outdated
Comment thread tests/BenchmarkDotNet.IntegrationTests.TestingPlatform.Internals/Program.cs Outdated
Expand docs and comments to detail disposal order of parameter
values in BenchmarkDotNet when requests are handed over and
taken back. Update tests to assert both possible sweep orders,
ensuring values are disposed exactly once in either case.
Revise README and comments for clearer scenario descriptions.
@timcassell

Copy link
Copy Markdown
Collaborator

I tested it out in VS. Running the AsyncDisposableProbe.Identity group, individual detail summary:

Standard Output: 
  WARNING: Type WithAbstractConfig was ignored because its attributes could not be read: Cannot dynamically create an instance of type 'BenchmarkDotNet.Configs.DebugConfig'. Reason: Cannot create an abstract class.
  WARNING: Type WithInaccessibleConfig was ignored because its attributes could not be read: No parameterless constructor defined for type 'BenchmarkDotNet.IntegrationTests.TestingPlatform.InvalidConfigProbe+WithInaccessibleConfig+NoPublicConstructorConfig'.
  AsyncDisposableProbe.Identity: Dry(Runtime=.NET 10.0, Toolchain=InProcessEmitToolchain 10.0, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=1) [Value=async-1]
  Runtime = ; GC = 
  -------------------- Histogram --------------------
  [46.099 μs ; 46.100 μs) | @
  ---------------------------------------------------
  Mean = 46.100 μs, StdErr = 0.000 μs (0.00%), N = 1, StdDev = 0.000 μs
  Min = 46.100 μs, Q1 = 46.100 μs, Median = 46.100 μs, Q3 = 46.100 μs, Max = 46.100 μs
  IQR = 0.000 μs, LowerFence = 46.100 μs, UpperFence = 46.100 μs
  ConfidenceInterval = [NaN μs; NaN μs] (CI 99.9%), Margin = NaN μs (NaN% of Mean)
  Skewness = NaN, Kurtosis = NaN, MValue = 2

The warnings look correct, but unrelated to the individual result. And Runtime = ; GC = looks wrong. I got the same result on VSTest, so it at least doesn't look like a regression.

Test output:

Building Test Projects
========== Starting test run ==========
//    * Type WithAbstractConfig was ignored because its attributes could not be read: Cannot dynamically create an instance of type 'BenchmarkDotNet.Configs.DebugConfig'. Reason: Cannot create an abstract class.
//    * Type WithInaccessibleConfig was ignored because its attributes could not be read: No parameterless constructor defined for type 'BenchmarkDotNet.IntegrationTests.TestingPlatform.InvalidConfigProbe+WithInaccessibleConfig+NoPublicConstructorConfig'.

// * Warnings *
MinIterationTime
  AsyncDisposableProbe.Identity: Dry -> The minimum observed iteration time is 46.1us which is very small. It's recommended to increase it to at least 100ms using more operations.
  AsyncDisposableProbe.Identity: Dry -> The minimum observed iteration time is 400ns which is very small. It's recommended to increase it to at least 100ms using more operations.
========== Test run finished: 2 Tests (2 Passed, 0 Failed, 0 Skipped) run in 567.3 ms ==========

It's missing all the info of the run. Probably just an issue with passing a correct ILogger.


If possible with new MTP, we should try to add the summary table to the group summary (#2502). It looks like the protocol supports it via parentTestNodeUid optional param in TestNodeUpdateMessage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants