Uh oh!
There was an error while loading. Please reload this page.
Use generated descriptors for MTP discovery - #10777
Conversation
Bypass legacy method enumeration and validation for the bounded generated synchronous TestMethod and DataRow subset while retaining per-method fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: fd0b7d0f-8590-4c8b-ae58-635c652c60ef
Uh oh!
There was an error while loading. Please reload this page.
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
There was a problem hiding this comment.
Pull request overview
Adds an MTP-only fast discovery path using generated descriptors while preserving legacy fallback and VSTest behavior.
Changes:
- Extends source-generation metadata with descriptor eligibility and completeness.
- Uses descriptors during MTP discovery with per-method fallback.
- Adds unit, acceptance, and NativeAOT coverage.
Show a summary per file
| File | Description |
|---|---|
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/TestableImplementations/TestablePlatformServiceProvider.cs | Supports generated reflection providers in tests. |
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/TestableImplementations/MockableReflectionOperations.cs | Forwards descriptor lookups. |
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/SourceGeneration/SourceGeneratedReflectionOperationsTests.cs | Tests descriptor retrieval and completeness. |
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Discovery/TypeEnumeratorTests.cs | Tests fast-path selection and fallback. |
test/UnitTests/MSTest.SourceGeneration.UnitTests/MSTestReflectionMetadataGeneratorTests.cs | Verifies generated descriptor metadata. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/SourceGenerationNonAotTests.cs | Checks non-AOT generated output. |
test/IntegrationTests/MSTest.Acceptance.IntegrationTests/NativeAotTests.cs | Checks NativeAOT eligibility metadata. |
src/Analyzers/MSTest.SourceGeneration/Models/TestClassModel.cs | Adds descriptor capability fields. |
src/Analyzers/MSTest.SourceGeneration/Generators/TestClassModelBuilder.cs | Determines descriptor eligibility. |
src/Analyzers/MSTest.SourceGeneration/Generators/RuntimeRegistrationEmitter.cs | Emits descriptor registration. |
src/Analyzers/MSTest.SourceGeneration/Generators/MetadataRegistryEmitter.cs | Emits descriptor metadata properties. |
src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionOperations.cs | Exposes registered descriptors. |
src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/SourceGeneratedReflectionDataProvider.cs | Stores descriptor data. |
src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/ReflectionMetadataHook.cs | Registers descriptor metadata. |
src/Adapter/MSTestAdapter.PlatformServices/SourceGeneration/CompositeSourceGeneratedReflectionDataProvider.cs | Merges descriptor providers. |
src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs | Provides reflection-mode fallback. |
src/Adapter/MSTestAdapter.PlatformServices/PublicAPI/PublicAPI.Unshipped.txt | Tracks the new public overload. |
src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/UnitTestElement.cs | Marks descriptor-originated tests. |
src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs | Defines descriptor lookup. |
src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs | Consumes descriptors during discovery. |
src/Adapter/MSTestAdapter.PlatformServices/Discovery/AssemblyEnumeratorWrapper.cs | Enables descriptors for MTP. |
src/Adapter/MSTestAdapter.PlatformServices/Discovery/AssemblyEnumerator.cs | Propagates descriptor selection. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 22/22 changed files
- Comments generated: 3
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 — PR #10777
This PR introduces source-generated test descriptors for native MTP discovery, allowing the discovery path to skip the legacy runtime-method scan when generated metadata is complete. The design is well-layered: the source generator emits per-method/per-class support flags, the ReflectionMetadataHook.Register overload carries the new dictionaries, and the TypeEnumerator consumes them with a clean fallback path.
Verdict Table
| # | Dimension | Verdict |
|---|---|---|
| 1 | Algorithmic Correctness | DescriptorCompleteTypes are last-writer-wins — may silently produce wrong result when multiple providers register the same type |
| 2 | Threading & Concurrency | ✅ N/A — no new shared mutable state introduced |
| 3 | Security & IPC | ✅ N/A |
| 4 | Public API & Binary Compat | ✅ New Register overload added (additive), old overload delegates. PublicAPI.Unshipped.txt updated. |
| 5 | Performance & Allocations | ✅ Good — List pre-sized, HashSet used for skip-set |
| 6 | Cross-TFM Compatibility | ✅ N/A — no TFM-specific APIs used |
| 7 | Resource & IDisposable | ✅ N/A |
| 8 | Defensive Coding | ✅ Null checks on new parameters, graceful fallback when descriptors unavailable |
| 9 | Localization | ✅ N/A |
| 10 | Test Isolation | ✅ Tests set up their own providers |
| 11 | Assertion Quality | ✅ Uses AwesomeAssertions per project policy |
| 12 | Flakiness Patterns | ✅ N/A |
| 13 | Test Completeness | ✅ Good coverage of complete, incomplete, and non-MTP paths |
| 14 | Data-Driven Test Coverage | ✅ N/A |
| 15 | Code Structure | AssemblyEnumerator |
| 16–22 | Remaining dimensions | ✅ N/A or clean |
Key Findings
MAJOR —
CompositeSourceGeneratedReflectionDataProvidermerge semantics:MergeIntoforDescriptorCompleteTypesuses last-writer-wins. When two providers disagree on completeness for the same type, correctness depends on registration order. The safe semantic is logical AND. Same concern applies toDescriptorTestMethods(arrays should be concatenated, not overwritten).Minor — Redundant ternary: The call in
AssemblyEnumerator.DiscoverTestsInTypecould use the two-arg overload directly.Minor — Name-based duplicate detection vs. signature-based:
TestClassModelBuilderdisqualifies overloaded methods by name, while the runtime usesToString()(includes signature). This is conservative but worth documenting.
Overall this is a solid, well-tested addition. The merge-semantics issue (finding #1) is the only one that could cause a real bug in multi-assembly/multi-provider scenarios.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 144.4 AIC · ⌖ 1.22 AIC · ⊞ 16.9K · ◷
Uh oh!
There was an error while loading. Please reload this page.
🔍 Build Failure AnalysisSummary — The Linux Release build failed because Root cause: Collection initialization can be simplified
Affected files / errors
Proposed fix - : new HashSet<MethodInfo>(descriptorMethods);+ : [.. descriptorMethods];Build overview
All MSBuild errors (7)
🤖 Generated by the Build Failure Analysis workflow using (a href="(dev.azure.com/redacted) · commit 025fb6e
|
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Build Failure Analysis workflow. · auto · 82.8 AIC · ⌖ 1.54 AIC · ⊞ 13.3K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Preserve legacy discovery for partial and unresolved generated methods, merge repeated descriptor registrations conservatively, and record the internal API additions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 272.7 AIC · ⌖ 1.19 AIC · ⊞ 16.9K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Verify inaccessible generated test methods keep the containing class on legacy discovery without expecting metadata that is intentionally omitted. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
Exercise generated descriptor selection through the MTP discovery wrapper while preserving VSTest fallback, and pin unsupported execution-shape guards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 191.6 AIC · ⌖ 1.19 AIC · ⊞ 16.9K · ◷
Comments that could not be inline-anchored
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Discovery/AssemblyEnumeratorWrapperTests.cs:262
🧪 Test review · Grade D (60–69) — Registers a method into the process-global ReflectionMetadataHook.Composite registry with no reset, so the registration persists across the whole test-assembly run.
Reset the registration after the test (or route registration through a disposable/mockable seam) so other tests can't observe GeneratedDescriptorTestClass as permanently registered.
Replacement: none
Classify overrides from the same inherited attribute set used for emitted metadata so custom inherited TestMethod attributes retain legacy fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Detect test attributes on non-ordinary methods and property or event accessors so generated discovery never suppresses tests only visible to the legacy runtime scan. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 238.2 AIC · ⌖ 1.25 AIC · ⊞ 16.9K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Verify complete descriptors exclude unregistered methods and partial test classes include methods from every declaration while retaining legacy fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 212.5 AIC · ⌖ 1.25 AIC · ⊞ 16.9K · ◷
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Assert descriptor support per partial-class method and mark the process-wide registration test non-parallel. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧪 Expert test review — PR #10777
This advisory comment was generated automatically. Grades are heuristic
|
There was a problem hiding this comment.
🤖 Automated content by GitHub Copilot. Generated by the Test Reviewer on PR (on open / sync) workflow. · auto · 153.5 AIC · ⌖ 1.23 AIC · ⊞ 16.9K · ◷
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
Pin that complete generated descriptor sets never invoke legacy method validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
🧵 Parallel-safety audit — PR #10777Parallelization — one row per test assembly audited:
Findings: A (global-state) Top actions (by expected value):
Warning (readiness)
Info
Nothing else in the changed lines ( Advisory only — heuristic, non-blocking. Re-run with
|
Uh oh!
There was an error while loading. Please reload this page.
Summary
Add a bounded MTP-only generated descriptor discovery path for plain synchronous
[TestMethod]and[DataRow]methods.Generated methods that declare complete support bypass the legacy runtime method enumeration and validation pass. Mixed classes fall back per method, while VSTest continues using the existing discovery path.
Old and new flow
This deliberately retains
UnitTestElement, filtering, lifecycle, execution, and result conversion. It is the first production vertical slice, not a second lifecycle engine.Supported subset
The fast path is limited to complete generated descriptors for public, instance, non-abstract, non-async,
voidmethods with the exact built-inTestMethodAttributeand optionalDataRowAttributes.The legacy path remains authoritative for:
TestMethodAttributeimplementations;ITestDataSource;The selection is observable internally through generated-descriptor metadata and focused path-selection tests.
Impact
Projects/layers changed:
TestContext, execution, retry, timeout, cleanup, filtering, and result pipelines are unchanged.Controlled discovery benchmark, 10,000 iterations with two tests per iteration and five samples:
Release assembly size cost:
Validation
Future phases
This PR does not bypass
UnitTestElement,TypeCacheduring execution, the lifecycle engine, or MTP result conversion. Follow-up work can introduce a generated execution abstraction and broader descriptor eligibility while preserving this per-method fallback boundary.