Use generated descriptors for MTP discovery - #10777
Use generated descriptors for MTP discovery#10777Amaury Levé (Evangelink) wants to merge 1 commit into
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
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| // ToString() outputs method name and its signature. This is necessary for overloaded methods to be recognized as distinct tests. | ||
| foundDuplicateTests = foundDuplicateTests || !foundTests.Add(method.ToString() ?? method.Name); | ||
| UnitTestElement testMethod = GetTestFromMethod(method, classDisablesParallelization, warnings); | ||
| if (descriptorMethodSet?.Contains(method) == true) |
|
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
| @@ -1,2 +1,3 @@ | |||
| #nullable enable | |||
| static Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.SourceGeneration.ReflectionMetadataHook.Register(System.Reflection.Assembly! assembly, System.Type![]! types, System.Collections.Generic.IReadOnlyDictionary<System.Type!, System.Reflection.MethodInfo![]!>! testMethods, System.Collections.Generic.IReadOnlyDictionary<System.Type!, System.Attribute![]!>! typeAttributes, object![]! assemblyAttributes, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.MethodInfo!, System.Attribute![]!>! methodAttributes, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.MethodInfo!, System.Func<object?, object?[]?, object?>!>! methodInvokers, System.Collections.Generic.IReadOnlyDictionary<System.Type!, Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.SourceGeneration.ConstructorInvokerInfo[]!>! constructorInvokers, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.PropertyInfo!, System.Action<object?, object?>!>! propertySetters) -> void | |||
| static Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.SourceGeneration.ReflectionMetadataHook.Register(System.Reflection.Assembly! assembly, System.Type![]! types, System.Collections.Generic.IReadOnlyDictionary<System.Type!, System.Reflection.MethodInfo![]!>! testMethods, System.Collections.Generic.IReadOnlyDictionary<System.Type!, System.Attribute![]!>! typeAttributes, object![]! assemblyAttributes, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.MethodInfo!, System.Attribute![]!>! methodAttributes, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.MethodInfo!, System.Func<object?, object?[]?, object?>!>! methodInvokers, System.Collections.Generic.IReadOnlyDictionary<System.Type!, Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.SourceGeneration.ConstructorInvokerInfo[]!>! constructorInvokers, System.Collections.Generic.IReadOnlyDictionary<System.Reflection.PropertyInfo!, System.Action<object?, object?>!>! propertySetters, System.Collections.Generic.IReadOnlyDictionary<System.Type!, System.Reflection.MethodInfo![]!>! descriptorTestMethods, System.Type![]! descriptorCompleteTypes) -> void | |||
| bool areGeneratedDescriptorsComplete = supportsGeneratedDescriptors | ||
| && !hasUnsupportedTestMethod | ||
| && finalizedMethods.Where(static method => method.IsTestMethod).All(static method => method.IsDescriptorSupported); |
| using (sb.Block("if (testClass.AreGeneratedDescriptorsComplete)")) | ||
| { | ||
| sb.AppendLine("descriptorCompleteTypes.Add(type);"); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
| @@ -203,6 +205,8 @@ private static SourceGeneratedReflectionDataProvider BuildMergedSnapshot(IReadOn | |||
| MergeInto(typeConstructorsInvoker, provider.TypeConstructorsInvoker); | |||
| MergeInto(typeMethodInvokers, provider.TypeMethodInvokers); | |||
There was a problem hiding this comment.
Merge semantics for DescriptorCompleteTypes may silently regress completeness (Algorithmic Correctness).
MergeInto uses last-writer-wins (target[key] = value). If two source-generated providers register the same type — one with true (complete) and one with false (incomplete) — the final result depends on iteration order. The unit test TryGetTestMethodDescriptors_RetainsPerMethodFallbackWhenRegistrationIsIncomplete passes only because the false provider is added second.
The correct merge semantic for completeness is logical AND: a type is only complete if all providers declare it complete. Similarly, the MethodInfo[] for DescriptorTestMethods should be merged (union), not overwritten.
Consider replacing MergeInto here with a specialized merge:
if (target.TryGetValue(kvp.Key, out bool existing))
target[kvp.Key] = existing && kvp.Value;
else
target[kvp.Key] = kvp.Value;| int synchronousMethodIndex = registry.IndexOf("Name = \"TestMethod1\"", StringComparison.Ordinal); | ||
| int synchronousNextMethodIndex = registry.IndexOf("Name = \"TestMethod2\"", synchronousMethodIndex, StringComparison.Ordinal); | ||
| Assert.IsGreaterThan(-1, synchronousMethodIndex); | ||
| Assert.IsGreaterThan(synchronousMethodIndex, synchronousNextMethodIndex); |
There was a problem hiding this comment.
Assertion arguments appear inverted (Algorithmic Correctness).
Assert.IsGreaterThan<T>(T lowerBound, T value) asserts that value > lowerBound. Here the call is:
Assert.IsGreaterThan(synchronousMethodIndex, synchronousNextMethodIndex);This asserts synchronousNextMethodIndex > synchronousMethodIndex, i.e. TestMethod2 appears after TestMethod1. But looking at the existing pattern on line 171, Assert.IsGreaterThan(asyncMethodIndex, nextMethodIndex) asserts that nextMethodIndex > asyncMethodIndex — which is the same ordering intent ("next method appears later"). So this is correct.
However, just double-check: is the intent to verify that TestMethod1 appears before TestMethod2? If so, this is correct. If the intent is the reverse, the arguments need to be swapped. Consider adding a clarifying comment for future readers.
| AreAttributesComplete: methodAttributes.IsComplete, | ||
| DynamicDataSources: DynamicDataSourceBuilder.BuildDynamicDataSources(inheritedAttributes, method, consumingAssembly)); | ||
| } | ||
|
|
There was a problem hiding this comment.
HasOnlyDescriptorSupportedAttributes is restrictive — only [TestMethod] and [DataRow] are supported (Design observation).
This is a deliberate, conservative starting point for the descriptor path. However, as a note for reviewers: tests decorated with [Timeout], [Priority], [Owner], or other well-known attributes will fall back to legacy discovery even though those attributes don't change discovery semantics. Worth calling out in a tracking issue or inline comment for future expansion.
| typeFullName = type.FullName; | ||
| TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals); | ||
| List<UnitTestElement>? unitTestCases = testTypeEnumerator.Enumerate(warningMessages); | ||
| List<UnitTestElement>? unitTestCases = useGeneratedDescriptors |
There was a problem hiding this comment.
Ternary branch is redundant — just pass the value directly (Code Structure).
List<UnitTestElement>? unitTestCases = useGeneratedDescriptors
? testTypeEnumerator.Enumerate(warningMessages, useGeneratedDescriptors: true)
: testTypeEnumerator.Enumerate(warningMessages);Since the two-arg overload simply delegates to the same EnumerateCore, this can be simplified to a single call:
List<UnitTestElement>? unitTestCases = testTypeEnumerator.Enumerate(warningMessages, useGeneratedDescriptors);The overload Enumerate(List<string> warnings, bool useGeneratedDescriptors) already exists and handles both paths.
| && classAttributes.Attributes[0].FullyQualifiedAttributeType == TestClassAttributeName; | ||
|
|
||
| var duplicateTestMethodNames = new HashSet<string>( | ||
| methods |
There was a problem hiding this comment.
duplicateTestMethodNames detection uses method.Name but duplicate-test detection at runtime uses method.ToString() (Algorithmic Correctness).
In TypeEnumerator.GetTests, duplicates are detected via method.ToString() (which includes the signature, catching overloads). Here, methods with the same name are disqualified from descriptor support, even if they have different signatures (legitimate overloads).
This means overloaded test methods (e.g. TestAdd(int) and TestAdd(string)) will always fall back to legacy discovery even though they're not truly "duplicate" tests. This seems intentionally conservative, but consider documenting this as a known limitation or using signature-based grouping for a tighter match.
🧪 Expert test review — PR #10777
General observation (not test-specific): three This advisory comment was generated automatically. Grades are heuristic and informational — they do not block merging. Suggestions on the Files changed tab can be applied with one click. Re-run with
|
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 · ◷
| operations.TryGetTestMethodDescriptors(typeof(Sample), out MethodInfo[]? methods, out bool isComplete) | ||
| .Should().BeTrue(); | ||
| methods.Should().ContainSingle().Which.Should().BeSameAs(method); | ||
| isComplete.Should().BeFalse(); |
There was a problem hiding this comment.
🧪 Test review · Grade B (80–89) — Both providers register the same MethodInfo, so the test never proves the composite actually merges distinct per-provider descriptor sets.
Use a second real method on Sample for the incomplete provider (or assert on a differing method reference) so the test would fail if merging picked the wrong provider entry.
🔍 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 · ◷
| var tests = new List<UnitTestElement>(descriptorMethods.Length); | ||
| HashSet<MethodInfo>? descriptorMethodSet = descriptorMethods.Length == 0 | ||
| ? null | ||
| : new HashSet<MethodInfo>(descriptorMethods); |
There was a problem hiding this comment.
🔧 IDE0306/IDE0028 — Collection initialization can be simplified; use a collection expression instead of the HashSet<T> constructor with an argument, matching the analyzer rule enforced as an error in this build.
| : new HashSet<MethodInfo>(descriptorMethods); | |
| : [.. descriptorMethods]; |
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.