Refactor TestMethod type metadata to a single semantic class identity by Copilot · Pull Request #8213 · microsoft/testfx

Copilot · 2026-05-14T09:24:05Z

✨ Enhancement

TestMethod was carrying overlapping type metadata (FullClassName, ManagedTypeName, and historically syntactic/declaring context), which made inheritance scenarios harder to reason about. This change consolidates semantic type identity to one source and keeps adapter behavior aligned with that model.

What does this improve?

Removes duplicate semantic type storage from TestMethod.
Makes semantic vs. syntactic class handling clearer in discovery/execution flows.
Simplifies API expectations: semantic class identity is FullClassName.

Why is this valuable?

Reduces model redundancy and ambiguity in inherited test scenarios.
Improves maintainability by eliminating parallel fields representing the same concept.
Makes metadata handling more obvious for callers and future refactors.

Implementation approach:

TestMethod model cleanup
- ManagedTypeName is now derived from FullClassName instead of stored separately.
- Removed managedTypeName from the main TestMethod constructor.
Construction path updates
- Updated discovery and TestCase conversion paths to construct TestMethod without a separate managed type argument.
- In TestCase rehydration, semantic type is preferred when present.
Contract/documentation alignment
- Updated ITestMethod comments to reflect that managed type maps to semantic class name (FullClassName).
Focused unit test updates
- Adjusted constructor call sites in unit tests.
- Added coverage for semantic-type preference and ManagedTypeName/FullClassName equivalence.

internal sealed class TestMethod : ITestMethod
{
    public string FullClassName { get; }

    // No separate backing field anymore.
    public string? ManagedTypeName => FullClassName;
}

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Evangelink · 2026-05-14T10:27:36Z

/review

github-actions · 2026-05-14T10:29:02Z

❌ Expert Code Review (command) failed. Please review the logs for details.

Copilot

Pull request overview

Consolidates TestMethod's type metadata so that ManagedTypeName is computed from FullClassName rather than stored as a separate field. The constructor's managedTypeName parameter is removed, and discovery/rehydration paths and tests are updated accordingly. The interface comments are clarified to reflect the new mapping.

Changes:

TestMethod.ManagedTypeName becomes a derived property (=> FullClassName); the main constructor drops the managedTypeName parameter, and TypeEnumerator no longer captures it from ManagedNameHelper.
TestCaseExtensions.ToUnitTestElementWithUpdatedSource now prefers the managed-type property (semantic type) over the legacy TestClassNameProperty when constructing the TestMethod.
ITestMethod doc comments clarify that FullClassName is the semantic class name and that ManagedTypeName maps to it; tests updated for new constructor and added coverage for managed-type preference and equivalence.

Show a summary per file

File	Description
src/Adapter/MSTestAdapter.PlatformServices/ObjectModel/TestMethod.cs	Removes `managedTypeName` constructor parameter and field; `ManagedTypeName` now returns `FullClassName`.
src/Adapter/MSTestAdapter.PlatformServices/Interfaces/ObjectModel/ITestMethod.cs	Clarifies semantic meaning of `FullClassName` and notes that `ManagedTypeName` maps to it.
src/Adapter/MSTestAdapter.PlatformServices/Discovery/TypeEnumerator.cs	Discards `managedType` from `ManagedNameHelper` and constructs `TestMethod` without it.
src/Adapter/MSTestAdapter.PlatformServices/Extensions/TestCaseExtensions.cs	Uses `GetManagedType()` (falling back to `TestClassNameProperty`) as the semantic class name for the new `TestMethod`.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/ObjectModel/UnitTestElementTests.cs	Adds test asserting `ManagedTypeProperty` is set from `FullClassName`.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Extensions/TestCaseExtensionsTests.cs	Adds test verifying managed type is preferred over `TestClassNameProperty`.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TypeCacheTests.cs	Updates helper to new constructor signature.
test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestExecutionManagerTests.cs	Updates `GetTestCase` to new constructor signature.

Copilot's findings

Files reviewed: 8/8 changed files
Comments generated: 0

Youssef1313 · 2026-05-14T10:40:00Z

        DebugEx.Assert(_type.AssemblyQualifiedName != null, "AssemblyQualifiedName for method is null.");

-        ManagedNameHelper.GetManagedNameAndHierarchy(method, out string managedType, out string managedMethod, out string?[] hierarchyValues);
+        ManagedNameHelper.GetManagedNameAndHierarchy(method, out _, out string managedMethod, out string?[] hierarchyValues);


Please keep the out string managedType and instead drop the _type.FullName!.

managedType handles generic cases more properly, I think.

Consider also finding a unit or E2E test that captures the difference in behavior.

@copilot address this comment

Addressed in d56a41d. After extensive discussion (see Evangelink's MAJOR finding), we cannot drop _type.FullName! for FullClassName because TypeCache.LoadType calls assembly.GetType(FullClassName) which requires the closed-generic CLR name to instantiate the type — passing the open-generic managed name (e.g. MyClass1) would return an uninstantiable open-generic Type`.

The current resolution:

FullClassName keeps _type.FullName! (closed-generic, required for type loading).

ManagedTypeName is derived from FullClassName by stripping the generic argument list ([...]), producing the open-generic managed name expected by VSTest's ManagedType property.

The unused managedType out parameter is now discarded with out _ and an explanatory comment is added.

Coverage exists in TypeEnumeratorTests.GetTestFromMethodShouldUseClosedFullClassNameAndOpenManagedTypeNameForGenericTypes (verifies both values for DummyGenericTestClass<int>, then round-trips through ToTestCase and TypeCache.GetTestMethodInfo to prove type loading still works) and TestCaseExtensionsTests.ToUnitTestElementShouldParseLegacyClosedGenericFullyQualifiedNameWhenManagedTypeIsOpenGeneric (verifies legacy closed-generic FQN rehydration).

Evangelink · 2026-05-14T12:25:03Z

@copilot address review comments

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-14T12:32:54Z

@copilot address review comments

Addressed the review feedback in f1c37dd.

The PR removed the leading `managedTypeName` parameter from the `TestMethod` constructor (it is now derived from `FullClassName`), but two test helper methods and one new test still used the old 8-argument signature, breaking the Windows build: - `UnitTestDiscovererTests.CreateTestMethod` and `UnitTestRunnerTests.CreateTestMethod`: passed `typeFullName` twice (once as the now-removed `managedTypeName` and once as `fullClassName`). Drop the leading argument so they target the new 7-arg overload. - `TypeEnumeratorTests.GetTestFromMethodShouldUseManagedTypeNameForGenericTypes`: used `nameof(DummyGenericTestClass<int>.GenericTestMethod)` which triggers IDE0340 (Use unbound generic type). Replace with `nameof(DummyGenericTestClass<>.GenericTestMethod)`. Also merges the latest `main` into the PR branch to pick up build infrastructure changes that the previous CI run did not have. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Address review comment #3240781680: explain why ToUnitTestElementWithUpdatedSource still falls back to TestClassNameProperty when GetManagedType() returns null. The fallback is required for TestCases produced by older runners that pre-date the managed-type properties (UnitTestElement.ToTestCase does not write the managed-type pair when ManagedMethodName is unknown). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 11/11 changed files
Comments generated: 0 new

Evangelink · 2026-05-14T14:27:19Z

/review

github-actions · 2026-05-14T14:28:13Z

✅ Expert Code Review (command) completed successfully!

Expert-reviewer launched in background for PR #8213. It will post the review directly.

Evangelink

Review Summary

This PR has a clear goal — eliminate the redundant managedTypeName constructor parameter and derive ManagedTypeName from FullClassName — and the non-generic path is clean. However, the change introduces two behavioural regressions for generic test classes that need to be addressed before merging.

Verdict table

#	Dimension	Finding	Severity
1	Algorithmic Correctness	`TypeEnumerator` now passes `managedType` (open-generic name) as `fullClassName`. `TypeCache.LoadType` uses this to call `assembly.GetType(typeName)`, which returns an uninstantiable open-generic `Type` for closed-generic test classes.	MAJOR
2	Backward Compatibility	`GetTestName` strips the class-name prefix from the FQN using string comparison. Old TestCases produced before this change carry a closed-generic FQN; the new code supplies the open-generic managed type as `testClassName`. `StartsWith` fails → method name is set to the entire FQN, breaking re-execution.	MAJOR
3	API Contract	`ManagedTypeName` is now always non-null. `HasManagedMethodAndTypeProperties` / `[MemberNotNullWhen]` implicitly relied on either half being nullable to gate `ManagedTypeProperty` writes. The widened write-set is undocumented.	MODERATE
4	Test Completeness	New generic-type test only validates property values; it does not round-trip through `ToTestCase` + `TypeCache` resolution, which is the only path where the bug would surface.	MODERATE
5	Assertion Quality	`GetPropertyValue` without type arg returns `object?`; inconsistent with the rest of the file.	NIT
6	Threading/Concurrency	N/A — no shared mutable state introduced.	—
7	Security/IPC	N/A — no wire-format changes.	—
8	Public API	All changed members are `internal`; no `PublicAPI.Unshipped.txt` impact.	—
9	Performance	N/A — `ManagedTypeName => FullClassName` is a single property read, no allocation.	—
10	Cross-TFM	N/A — no TFM-specific APIs introduced.	—
11–21	Remaining dimensions	No findings.	—

Recommended fixes

TypeEnumerator.cs: Keep fullClassName = _type.FullName! (the exact runtime type name for closed generics) and pass managedType only where a managed-type identifier is expected (i.e. ManagedMethodName's counterpart). If the intent is truly to unify them, add a test proving that TypeCache can resolve and instantiate the resulting type.
TestCaseExtensions.cs: After changing testClassName to prefer the managed type, add a test that re-hydrates a TestCase whose FQN was generated under the old convention (closed-generic form) and verifies the method name is parsed correctly.
TestMethod.cs (HasManagedMethodAndTypeProperties): Add a comment or rename the property to reflect that it now only guards the ManagedMethodName being present (since ManagedTypeName can never be null).

Generated by Expert Code Review (command) for issue #8213 · ● 10.7M

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot · 2026-05-14T14:52:05Z

Review Summary

This PR has a clear goal — eliminate the redundant managedTypeName constructor parameter and derive ManagedTypeName from FullClassName — and the non-generic path is clean....

Addressed in cb20a82. The fix restores closed-generic FullClassName for type loading, derives open managed type metadata from it, covers legacy generic TestCase rehydration, and clarifies the managed metadata gate.

- TypeEnumerator: discard the unused managedType out variable (use 'out _') and add a comment explaining why FullClassName uses _type.FullName! (closed-generic CLR name needed for TypeCache.LoadType) while ManagedTypeName is derived from it. - TestMethod: drop StringComparison overload of String.IndexOf for net462/uap10.0.16299 compatibility. - TestCaseExtensionsTests: use unbound generic type for nameof to satisfy IDE0340. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot's findings

Files reviewed: 11/11 changed files
Comments generated: 0 new

Initial plan

49da6e0

Copilot AI assigned Copilot and Evangelink May 14, 2026

Copilot AI requested review from Copilot and removed request for Copilot May 14, 2026 09:24

Copilot started work on behalf of Evangelink May 14, 2026 09:25 View session

Copilot AI linked an issue May 14, 2026 that may be closed by this pull request

Refactor and cleanup how we store type information for test methods #7032

Closed

Refactor TestMethod to avoid duplicate semantic type storage

3a48b49

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot AI requested review from Copilot and removed request for Copilot May 14, 2026 09:34

Copilot AI changed the title ~~[WIP] Refactor and cleanup how we store type information for test methods~~ Refactor TestMethod type metadata to a single semantic class identity May 14, 2026

Copilot AI requested a review from Evangelink May 14, 2026 09:36

Copilot finished work on behalf of Evangelink May 14, 2026 09:36

Evangelink marked this pull request as ready for review May 14, 2026 10:27

Copilot AI review requested due to automatic review settings May 14, 2026 10:27

Copilot started reviewing on behalf of Evangelink May 14, 2026 10:28 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

Youssef1313 reviewed May 14, 2026

View reviewed changes

Comment thread src/Adapter/MSTestAdapter.PlatformServices/Extensions/TestCaseExtensions.cs Outdated

Copilot started work on behalf of Evangelink May 14, 2026 12:25 View session

Address review feedback for TestMethod type metadata

f1c37dd

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot finished work on behalf of Evangelink May 14, 2026 12:33

Amaury Levé and others added 2 commits May 14, 2026 15:15

Merge remote-tracking branch 'origin/main' into pr-8213

8d69692

Copilot AI review requested due to automatic review settings May 14, 2026 13:55

Copilot started reviewing on behalf of Evangelink May 14, 2026 13:57 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

Copilot started work on behalf of Evangelink May 14, 2026 14:27 View session

Copilot finished work on behalf of Evangelink May 14, 2026 14:31

Evangelink requested changes May 14, 2026

View reviewed changes

microsoft-github-policy-service Bot added the Needs: Author Feedback label May 14, 2026

Copilot started work on behalf of Evangelink May 14, 2026 14:39 View session

Copilot AI and others added 2 commits May 14, 2026 14:49

Fix generic TestMethod metadata rehydration

8c0145c

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Address validation review nits

cb20a82

Co-authored-by: Evangelink <11340282+Evangelink@users.noreply.github.com>

Copilot AI requested review from Copilot and removed request for Copilot May 14, 2026 14:51

microsoft-github-policy-service Bot added Needs: Attention 👋 and removed Needs: Author Feedback labels May 14, 2026

Copilot finished work on behalf of Evangelink May 14, 2026 14:52

Copilot AI requested a review from Evangelink May 14, 2026 14:53

Copilot AI review requested due to automatic review settings May 14, 2026 17:01

Copilot started reviewing on behalf of Evangelink May 14, 2026 17:06 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

Evangelink merged commit a466400 into main May 14, 2026
19 of 21 checks passed

Evangelink deleted the copilot/refactor-test-method-type-storage branch May 14, 2026 18:14

microsoft-github-policy-service Bot removed the Needs: Attention 👋 label May 14, 2026

Conversation

Copilot AI commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✨ Enhancement

Uh oh!

Evangelink commented May 14, 2026

Uh oh!

github-actions Bot commented May 14, 2026 • edited by Evangelink Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Youssef1313 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Youssef1313 May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Evangelink May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Evangelink May 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Evangelink commented May 14, 2026

Uh oh!

Copilot AI commented May 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Evangelink commented May 14, 2026

Uh oh!

github-actions Bot commented May 14, 2026 • edited by Evangelink Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Review Summary

Verdict table

Recommended fixes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented May 14, 2026

Review Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot's findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Copilot AI commented May 14, 2026 •

edited

Loading

github-actions Bot commented May 14, 2026 •

edited by Evangelink

Loading

github-actions Bot commented May 14, 2026 •

edited by Evangelink

Loading