Abstract VSTest test filtering in PlatformServices (Phase 2 of platform-agnostic effort) (#9567)

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: Evangelink

src/Adapter/MSTestAdapter.PlatformServices/Discovery/UnitTestDiscoverer.cs

@@ -110,24 +110,22 @@ internal virtual void DiscoverTestsInSource(
 
     internal void SendTestCases(IEnumerable<UnitTestElement> testElements, ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext, IAdapterMessageLogger logger)
     {
-        // Get filter expression and skip discovery in case filter expression has parsing error.
-        ITestCaseFilterExpression? filterExpression = _testMethodFilter.GetFilterExpression(discoveryContext, logger, out bool filterHasError);
+        // Get filter and skip discovery in case filter expression has parsing error.
+        ITestElementFilter? filter = _testMethodFilter.GetTestElementFilter(discoveryContext, logger, out bool filterHasError);
         if (filterHasError)
         {
             return;
         }
 
         foreach (UnitTestElement testElement in testElements)
         {
-            var testCase = testElement.ToTestCase();
-
-            // Filter tests based on test case filters
-            if (filterExpression != null && !filterExpression.MatchTestCase(testCase, p => _testMethodFilter.PropertyValueProvider(testCase, p)))
+            // Filter tests based on test case filters.
+            if (filter is not null && !filter.Matches(testElement))
             {
                 continue;
             }
 
-            discoverySink.SendTestCase(testCase);
+            discoverySink.SendTestCase(testElement.ToTestCase());
         }
     }
 }

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Parallelization.cs

@@ -73,7 +73,7 @@ private async Task ExecuteTestsInSourceAsync(IEnumerable<TestCase> tests, IRunCo
         }
 
         // Default test set is filtered tests based on user provided filter criteria
-        ITestCaseFilterExpression? filterExpression = _testMethodFilter.GetFilterExpression(runContext, adapterMessageLogger, out bool filterHasError);
+        ITestElementFilter? filter = _testMethodFilter.GetTestElementFilter(runContext, adapterMessageLogger, out bool filterHasError);
         if (filterHasError)
         {
             // Bail out without processing everything else below.
@@ -113,13 +113,19 @@ private async Task ExecuteTestsInSourceAsync(IEnumerable<TestCase> tests, IRunCo
 
         int parallelWorkers = sourceSettings.Workers;
         ExecutionScope parallelScope = sourceSettings.Scope;
-        TestCase[] testsToRun = [.. tests.Where(t => MatchTestFilter(filterExpression, t, _testMethodFilter))];
+        // Convert each test to its UnitTestElement exactly once and carry the pair through filtering and
+        // shuffling, so the surviving tests aren't converted a second time when building unitTestElements below.
+        (TestCase TestCase, UnitTestElement Element)[] testsToRunPairs =
+            [.. tests
+                .Select(test => (TestCase: test, Element: test.ToUnitTestElementWithUpdatedSource(source)))
+                .Where(pair => MatchTestFilter(filter, pair.Element))];
         if (_testOrderRandom is { } sourceRandom)
         {
-            Shuffle(sourceRandom, testsToRun);
+            Shuffle(sourceRandom, testsToRunPairs);
         }
 
-        UnitTestElement[] unitTestElements = [.. testsToRun.Select(e => e.ToUnitTestElementWithUpdatedSource(source))];
+        TestCase[] testsToRun = [.. testsToRunPairs.Select(pair => pair.TestCase)];
+        UnitTestElement[] unitTestElements = [.. testsToRunPairs.Select(pair => pair.Element)];
         // Create an instance of a type defined in adapter so that adapter gets loaded in the child app domain
         var testRunner = (UnitTestRunner)isolationHost.CreateInstanceForType(
             typeof(UnitTestRunner),

src/Adapter/MSTestAdapter.PlatformServices/Execution/TestExecutionManager.Runner.cs

@@ -41,17 +41,11 @@ internal void SendTestResults(
         }
     }
 
-    private static bool MatchTestFilter(ITestCaseFilterExpression? filterExpression, TestCase test, TestMethodFilter testMethodFilter)
-    {
-        if (filterExpression != null
-            && !filterExpression.MatchTestCase(test, p => testMethodFilter.PropertyValueProvider(test, p)))
-        {
-            // Skip test if not fitting filter criteria.
-            return false;
-        }
-
-        return true;
-    }
+    // Takes the already-converted UnitTestElement (rather than reconstructing one from the TestCase) so the
+    // caller can convert each test exactly once and reuse that element both for filtering and downstream.
+    private static bool MatchTestFilter(ITestElementFilter? filter, UnitTestElement testElement)
+        // Keep the test when there is no filter or when the element satisfies the filter criteria.
+        => filter is null || filter.Matches(testElement);
 
     private async Task ExecuteTestsWithTestRunnerAsync(
         IEnumerable<TestCase> tests,

src/Adapter/MSTestAdapter.PlatformServices/Interfaces/ITestElementFilter.cs

@@ -0,0 +1,29 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel;
+
+namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Interface;
+
+/// <summary>
+/// Platform-agnostic predicate that decides whether a discovered or executed <see cref="UnitTestElement"/>
+/// should be included in the current test run, based on the user-provided test-case filter.
+/// </summary>
+/// <remarks>
+/// This abstraction lets the platform services discovery and execution pipelines apply filtering over the
+/// neutral <see cref="UnitTestElement"/> model without taking a dependency on a specific test platform's
+/// filter object model (for example the VSTest <c>ITestCaseFilterExpression</c>, <c>TestProperty</c> and
+/// <c>IRunContext.GetTestCaseFilter</c> types). The concrete filter is produced at the platform boundary by a
+/// wrapper that parses the host's filter (currently <c>TestMethodFilter</c>, which wraps the VSTest filter
+/// expression), and is expected to move fully out of the platform services layer in a later phase.
+/// A <see langword="null"/> filter means "no filter" and every element is included.
+/// </remarks>
+internal interface ITestElementFilter
+{
+    /// <summary>
+    /// Determines whether the given <paramref name="testElement"/> matches the filter and should be included.
+    /// </summary>
+    /// <param name="testElement">The test element to evaluate.</param>
+    /// <returns><see langword="true"/> if the element matches the filter; otherwise, <see langword="false"/>.</returns>
+    bool Matches(UnitTestElement testElement);
+}

src/Adapter/MSTestAdapter.PlatformServices/TestMethodFilter.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Interface;
 using Microsoft.VisualStudio.TestPlatform.ObjectModel;
@@ -30,7 +31,7 @@ internal sealed class TestMethodFilter
     /// Returns ITestCaseFilterExpression for TestProperties supported by adapter.
     /// </summary>
     /// <param name="context">The current context of the run.</param>
-    /// <param name="logger">Handler to report test messages/start/end and results.</param>
+    /// <param name="logger">Logger used to report diagnostic messages (for example, filter parse errors).</param>
     /// <param name="filterHasError">Indicates that the filter is unsupported/has an error.</param>
     /// <returns>A filter expression.</returns>
     internal ITestCaseFilterExpression? GetFilterExpression(IDiscoveryContext? context, IAdapterMessageLogger logger, out bool filterHasError)
@@ -57,6 +58,24 @@ internal sealed class TestMethodFilter
         return filter;
     }
 
+    /// <summary>
+    /// Builds a platform-agnostic <see cref="ITestElementFilter"/> for the properties supported by the adapter.
+    /// </summary>
+    /// <param name="context">The current context of the run.</param>
+    /// <param name="logger">Logger used to report diagnostic messages (for example, filter parse errors).</param>
+    /// <param name="filterHasError">Indicates that the filter is unsupported/has an error.</param>
+    /// <returns>
+    /// A filter that evaluates <see cref="UnitTestElement"/> instances, or <see langword="null"/> when no
+    /// filter applies (in which case every element is included).
+    /// </returns>
+    internal ITestElementFilter? GetTestElementFilter(IDiscoveryContext? context, IAdapterMessageLogger logger, out bool filterHasError)
+    {
+        ITestCaseFilterExpression? filterExpression = GetFilterExpression(context, logger, out filterHasError);
+        return filterExpression is null
+            ? null
+            : new TestElementFilter(this, filterExpression);
+    }
+
     /// <summary>
     /// Provides TestProperty for property name 'propertyName' as used in filter.
     /// </summary>
@@ -139,4 +158,27 @@ internal TestProperty PropertyProvider(string propertyName)
 
         return null;
     }
+
+    /// <summary>
+    /// Platform-agnostic filter that evaluates a <see cref="UnitTestElement"/> against a VSTest
+    /// <see cref="ITestCaseFilterExpression"/>. This is the single point where the neutral element model is
+    /// translated to the VSTest test case in order to reuse the host's filter matching.
+    /// </summary>
+    private sealed class TestElementFilter : ITestElementFilter
+    {
+        private readonly TestMethodFilter _testMethodFilter;
+        private readonly ITestCaseFilterExpression _filterExpression;
+
+        public TestElementFilter(TestMethodFilter testMethodFilter, ITestCaseFilterExpression filterExpression)
+        {
+            _testMethodFilter = testMethodFilter;
+            _filterExpression = filterExpression;
+        }
+
+        public bool Matches(UnitTestElement testElement)
+        {
+            var testCase = testElement.ToTestCase();
+            return _filterExpression.MatchTestCase(testCase, propertyName => _testMethodFilter.PropertyValueProvider(testCase, propertyName));
+        }
+    }
 }

test/IntegrationTests/MSTest.IntegrationTests/TestCaseFilteringTests.cs

@@ -0,0 +1,107 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.Collections.Immutable;
+
+using Microsoft.MSTestV2.CLIAutomation;
+using Microsoft.VisualStudio.TestPlatform.ObjectModel;
+
+using TestResult = Microsoft.VisualStudio.TestPlatform.ObjectModel.TestResult;
+
+namespace MSTest.IntegrationTests;
+
+/// <summary>
+/// Regression guard for the platform-agnostic filtering refactor (neutral <c>ITestElementFilter</c>).
+/// Filtering is now expressed over the neutral <c>UnitTestElement</c>: discovery matches
+/// <c>element.ToTestCase()</c>, and execution matches
+/// <c>incomingTestCase.ToUnitTestElementWithUpdatedSource(source).ToTestCase()</c>. These tests lock the
+/// end-to-end discover-then-run behavior for <c>FullyQualifiedName</c> and <c>Id</c> filters — the two
+/// properties most sensitive to the <c>TestCase</c> ↔ <c>UnitTestElement</c> round-trip that the refactor
+/// re-routes — so a future change cannot silently break <c>--filter</c>.
+/// </summary>
+[TestClass]
+public class TestCaseFilteringTests : CLITestBase
+{
+    private const string TestAsset = "DiscoverInternalsProject";
+
+    // A non-parameterized test with a stable, filter-safe fully qualified name (no filter operator chars).
+    private const string TargetDisplayName = "TopLevelInternalClass_TestMethod1";
+
+    [TestMethod]
+    public void FilterByFullyQualifiedNameSelectsExactlyTheMatchingTestDuringDiscovery()
+    {
+        string assemblyPath = GetAssetFullPath(TestAsset);
+        TestCase target = GetTargetTestCase(assemblyPath);
+
+        ImmutableArray<TestCase> filtered = DiscoverTests(assemblyPath, $"FullyQualifiedName={target.FullyQualifiedName}");
+
+        Assert.HasCount(1, filtered);
+        Assert.AreEqual(target.FullyQualifiedName, filtered[0].FullyQualifiedName);
+        Assert.AreEqual(target.Id, filtered[0].Id);
+    }
+
+    [TestMethod]
+    public void FilterByIdSelectsExactlyTheMatchingTestDuringDiscovery()
+    {
+        string assemblyPath = GetAssetFullPath(TestAsset);
+        TestCase target = GetTargetTestCase(assemblyPath);
+
+        ImmutableArray<TestCase> filtered = DiscoverTests(assemblyPath, $"Id={target.Id}");
+
+        Assert.HasCount(1, filtered);
+        Assert.AreEqual(target.FullyQualifiedName, filtered[0].FullyQualifiedName);
+        Assert.AreEqual(target.Id, filtered[0].Id);
+    }
+
+    [TestMethod]
+    public async Task FilterByFullyQualifiedNameSelectsExactlyTheMatchingTestDuringExecution()
+    {
+        string assemblyPath = GetAssetFullPath(TestAsset);
+        ImmutableArray<TestCase> allTests = DiscoverTests(assemblyPath);
+        TestCase target = allTests.First(t => t.DisplayName == TargetDisplayName);
+
+        SimulateCrossProcessTestCases(allTests);
+
+        ImmutableArray<TestResult> results = await RunTestsAsync(allTests, $"FullyQualifiedName={target.FullyQualifiedName}");
+
+        // HasCount(1) is the load-bearing assertion: it fails if the filter is ignored (all tests run) or if
+        // the FQN no longer matches after reconstruction (no test runs). The outcome check proves the target
+        // was actually executed via the reconstructed element, not merely selected.
+        Assert.HasCount(1, results);
+        Assert.AreEqual(target.FullyQualifiedName, results[0].TestCase.FullyQualifiedName);
+        Assert.AreEqual(TestOutcome.Passed, results[0].Outcome);
+    }
+
+    [TestMethod]
+    public async Task FilterByIdSelectsExactlyTheMatchingTestDuringExecution()
+    {
+        string assemblyPath = GetAssetFullPath(TestAsset);
+        ImmutableArray<TestCase> allTests = DiscoverTests(assemblyPath);
+        Guid targetId = allTests.First(t => t.DisplayName == TargetDisplayName).Id;
+
+        SimulateCrossProcessTestCases(allTests);
+
+        ImmutableArray<TestResult> results = await RunTestsAsync(allTests, $"Id={targetId}");
+
+        // HasCount(1) is the load-bearing assertion: filtering by Id after the TestCase -> UnitTestElement ->
+        // TestCase reconstruction must still recompute the same Id and select exactly the target. The outcome
+        // check proves the target was actually executed via the reconstructed element.
+        Assert.HasCount(1, results);
+        Assert.AreEqual(targetId, results[0].TestCase.Id);
+        Assert.AreEqual(TestOutcome.Passed, results[0].Outcome);
+    }
+
+    private static TestCase GetTargetTestCase(string assemblyPath)
+        => DiscoverTests(assemblyPath).First(t => t.DisplayName == TargetDisplayName);
+
+    // Clears LocalExtensionData so the execution pipeline rebuilds the UnitTestElement (and its regenerated
+    // TestCase/Id) from the test-case properties, mirroring the out-of-proc VSTest path where deserialized
+    // test cases carry no LocalExtensionData. This is the round-trip the filtering refactor must preserve.
+    private static void SimulateCrossProcessTestCases(ImmutableArray<TestCase> testCases)
+    {
+        foreach (TestCase testCase in testCases)
+        {
+            testCase.LocalExtensionData = null;
+        }
+    }
+}

test/IntegrationTests/MSTest.IntegrationTests/Utilities/CLITestBase.discovery.cs

@@ -41,6 +41,18 @@ internal static async Task<ImmutableArray<TestResult>> RunTestsAsync(IEnumerable
         return frameworkHandle.GetFlattenedTestResults();
     }
 
+    internal static async Task<ImmutableArray<TestResult>> RunTestsAsync(IEnumerable<TestCase> testCases, string? testCaseFilter)
+    {
+        var testExecutionManager = new TestExecutionManager();
+        var frameworkHandle = new InternalFrameworkHandle();
+
+        string runSettingsXml = GetRunSettingsXml(string.Empty);
+        var runContext = new InternalRunContext(runSettingsXml, testCaseFilter);
+
+        await testExecutionManager.ExecuteTestsAsync(testCases, runContext, frameworkHandle, false);
+        return frameworkHandle.GetFlattenedTestResults();
+    }
+
     #region Helper classes
     private class InternalLogger : IMessageLogger
     {
@@ -73,15 +85,46 @@ public InternalDiscoveryContext(string runSettings, string? testCaseFilter)
         public IRunSettings? RunSettings { get; }
 
         public ITestCaseFilterExpression? GetTestCaseFilter(IEnumerable<string> supportedProperties, Func<string, TestProperty> propertyProvider) => _filter;
+    }
 
-        private class InternalRunSettings : IRunSettings
-        {
-            public InternalRunSettings(string runSettings) => SettingsXml = runSettings;
+    private sealed class InternalRunContext : IRunContext
+    {
+        private readonly ITestCaseFilterExpression? _filter;
 
-            public string SettingsXml { get; }
+        public InternalRunContext(string runSettings, string? testCaseFilter)
+        {
+            RunSettings = new InternalRunSettings(runSettings);
 
-            public ISettingsProvider? GetSettings(string? settingsName) => throw new NotImplementedException();
+            if (testCaseFilter != null)
+            {
+                _filter = TestCaseFilterFactory.ParseTestFilter(testCaseFilter);
+            }
         }
+
+        public IRunSettings? RunSettings { get; }
+
+        public bool KeepAlive => false;
+
+        public bool InIsolation => false;
+
+        public bool IsDataCollectionEnabled => false;
+
+        public bool IsBeingDebugged => false;
+
+        public string? TestRunDirectory => null;
+
+        public string? SolutionDirectory => null;
+
+        public ITestCaseFilterExpression? GetTestCaseFilter(IEnumerable<string>? supportedProperties, Func<string, TestProperty?> propertyProvider) => _filter;
+    }
+
+    private sealed class InternalRunSettings : IRunSettings
+    {
+        public InternalRunSettings(string runSettings) => SettingsXml = runSettings;
+
+        public string SettingsXml { get; }
+
+        public ISettingsProvider? GetSettings(string? settingsName) => throw new NotImplementedException();
     }
 
     private sealed class InternalFrameworkHandle : IFrameworkHandle