Uh oh!
There was an error while loading. Please reload this page.
Add benchmarks for data source display names - #10889
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: None
What changed in this PR
Adds allocation-tracked benchmarks for the hot-path display-name computation used by data-driven MSTest cases.
Changes:
- Benchmarks mixed-type and single
object[]argument scenarios. - References TestFramework and grants benchmark access to its internals.
| File | Description |
|---|---|
TestDataSourceUtilitiesBenchmarks.cs | Adds two BenchmarkDotNet scenarios. |
MSTest.Performance.Benchmarks.csproj | References TestFramework. |
TestFramework.csproj | Adds internals visibility for benchmarks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🧪 Expert test review — PR #10889No new or modified test methods were identified in the changed regions Re-run with
|
There was a problem hiding this comment.
Note
🤖 Automated review by GitHub Copilot. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.
Review Summary
Clean PR — adds a BenchmarkDotNet benchmark for TestDataSourceUtilities.ComputeDefaultDisplayName, a hot path in data-driven test discovery/execution.
Dimension Verdict
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ N/A — no logic changes |
| 2 | Threading & Concurrency | ✅ Clean — static readonly fields are safely initialized; instance fields are per-benchmark-run |
| 3 | Security & IPC | ✅ N/A |
| 4 | Public API & Binary Compat | ✅ No public API changes — IVT grant is test-only |
| 5 | Performance & Allocations | ✅ Clean — benchmark itself is well-designed ([MemoryDiagnoser], results returned to prevent dead-code elimination) |
| 6 | Cross-TFM Compat | ✅ Benchmark project targets single TFM, no concern |
| 7 | Resource Management | ✅ No disposables |
| 8 | Error Handling | ✅ N/A |
| 9 | Naming & Conventions | ✅ Clean |
| 10 | Documentation | ✅ Good XML doc on the class |
| 11 | Test Quality | ✅ N/A — this is a perf benchmark, not a test |
| 12 | Localization | ✅ N/A |
| 13 | Configuration | ✅ N/A |
| 14 | Logging | ✅ N/A |
| 15 | Serialization | ✅ N/A |
| 16 | Build & Packaging | ✅ ProjectReference and IVT additions are correct |
| 17 | Telemetry | ✅ N/A |
| 18 | Accessibility | ✅ N/A |
| 19 | Extensibility | ✅ N/A |
| 20 | Migration | ✅ N/A |
| 21 | Code Style | ✅ Follows repo conventions |
| 22 | PowerShell | ✅ N/A |
No issues found. The IVT grant to MSTest.Performance.Benchmarks is appropriately scoped (the benchmark needs access to internalTestDataSourceUtilities). The null! field pattern with [GlobalSetup] is standard BenchmarkDotNet practice. The ! on GetMethod return values is safe since the methods are defined in the same class.
🧵 Parallel-safety audit — PR #10889Nothing audited here touches process-global state, shared filesystem paths, or This PR only adds a BenchmarkDotNet class ( Re-run with
|
Uh oh!
There was an error while loading. Please reload this page.
TestDataSourceUtilities.ComputeDefaultDisplayNameis a hot path for data-driven test discovery and execution, but it had no dedicated regression benchmark. This adds allocation-tracked BenchmarkDotNet coverage for mixed-type arguments and the singleobject[]argument path, along with the project reference and internals visibility required to exercise the implementation directly.The change is benchmark infrastructure only and does not alter production behavior.
Closes#10867