Introduce RetryAttribute for test methods

src/Adapter/MSTest.TestAdapter/Execution/TestExecutionManager.cs

@@ -39,7 +39,7 @@ public void SendMessage(TestMessageLevel testMessageLevel, string message)
     /// </summary>
     private readonly IDictionary<string, object> _sessionParameters;
     private readonly IEnvironment _environment;
-    private readonly Func<Action, Task> _taskFactory;
+    private readonly Func<Func<Task>, Task> _taskFactory;
 
     /// <summary>
     /// Specifies whether the test run is canceled or not.
@@ -51,15 +51,15 @@ public TestExecutionManager()
     {
     }
 
-    internal TestExecutionManager(IEnvironment environment, Func<Action, Task>? taskFactory = null)
+    internal TestExecutionManager(IEnvironment environment, Func<Func<Task>, Task>? taskFactory = null)
     {
         _testMethodFilter = new TestMethodFilter();
         _sessionParameters = new Dictionary<string, object>();
         _environment = environment;
         _taskFactory = taskFactory ?? DefaultFactoryAsync;
     }
 
-    private static Task DefaultFactoryAsync(Action action)
+    private static Task DefaultFactoryAsync(Func<Task> taskGetter)
     {
         if (MSTestSettings.RunConfigurationSettings.ExecutionApartmentState == ApartmentState.STA
             && RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
@@ -69,7 +69,9 @@ private static Task DefaultFactoryAsync(Action action)
             {
                 try
                 {
-                    action();
+                    // This is best we can do to execute in STA thread.
+                    Task task = taskGetter();
+                    task.GetAwaiter().GetResult();
                     tcs.SetResult(0);
                 }
                 catch (Exception ex)
@@ -84,7 +86,8 @@ private static Task DefaultFactoryAsync(Action action)
         }
         else
         {
-            return Task.Run(action);
+            // NOTE: If you replace this with `return taskGetter()`, you will break parallel tests.
+            return Task.Run(taskGetter);
         }
     }
 
@@ -121,7 +124,9 @@ public void RunTests(IEnumerable<TestCase> tests, IRunContext? runContext, IFram
         CacheSessionParameters(runContext, frameworkHandle);
 
         // Execute the tests
-        ExecuteTests(tests, runContext, frameworkHandle, isDeploymentDone);
+        // This is a public API, so we can't change it to be async.
+        // Consider not using this API internally, and introduce an async version, and mark this as obsolete.
+        ExecuteTestsAsync(tests, runContext, frameworkHandle, isDeploymentDone).GetAwaiter().GetResult();
 
         if (!_hasAnyTestFailed)
         {
@@ -159,7 +164,9 @@ public void RunTests(IEnumerable<string> sources, IRunContext? runContext, IFram
         CacheSessionParameters(runContext, frameworkHandle);
 
         // Run tests.
-        ExecuteTests(tests, runContext, frameworkHandle, isDeploymentDone);
+        // This is a public API, so we can't change it to be async.
+        // Consider not using this API internally, and introduce an async version, and mark this as obsolete.
+        ExecuteTestsAsync(tests, runContext, frameworkHandle, isDeploymentDone).GetAwaiter().GetResult();
 
         if (!_hasAnyTestFailed)
         {
@@ -174,7 +181,7 @@ public void RunTests(IEnumerable<string> sources, IRunContext? runContext, IFram
     /// <param name="runContext">The run context.</param>
     /// <param name="frameworkHandle">Handle to record test start/end/results.</param>
     /// <param name="isDeploymentDone">Indicates if deployment is done.</param>
-    internal virtual void ExecuteTests(IEnumerable<TestCase> tests, IRunContext? runContext, IFrameworkHandle frameworkHandle, bool isDeploymentDone)
+    internal virtual async Task ExecuteTestsAsync(IEnumerable<TestCase> tests, IRunContext? runContext, IFrameworkHandle frameworkHandle, bool isDeploymentDone)
     {
         var testsBySource = from test in tests
                             group test by test.Source into testGroup
@@ -183,7 +190,7 @@ group test by test.Source into testGroup
         foreach (var group in testsBySource)
         {
             _testRunCancellationToken?.ThrowIfCancellationRequested();
-            ExecuteTestsInSource(group.Tests, runContext, frameworkHandle, group.Source, isDeploymentDone);
+            await ExecuteTestsInSourceAsync(group.Tests, runContext, frameworkHandle, group.Source, isDeploymentDone);
         }
     }
 
@@ -257,7 +264,7 @@ private static bool MatchTestFilter(ITestCaseFilterExpression? filterExpression,
     /// <param name="frameworkHandle">Handle to record test start/end/results.</param>
     /// <param name="source">The test container for the tests.</param>
     /// <param name="isDeploymentDone">Indicates if deployment is done.</param>
-    private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runContext, IFrameworkHandle frameworkHandle, string source, bool isDeploymentDone)
+    private async Task ExecuteTestsInSourceAsync(IEnumerable<TestCase> tests, IRunContext? runContext, IFrameworkHandle frameworkHandle, string source, bool isDeploymentDone)
     {
         DebugEx.Assert(!StringEx.IsNullOrEmpty(source), "Source cannot be empty");
 
@@ -267,7 +274,7 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
         }
 
         using MSTestAdapter.PlatformServices.Interface.ITestSourceHost isolationHost = PlatformServiceProvider.Instance.CreateTestSourceHost(source, runContext?.RunSettings, frameworkHandle);
-
+        bool usesAppDomains = isolationHost is MSTestAdapter.PlatformServices.TestSourceHost { UsesAppDomain: true };
         PlatformServiceProvider.Instance.AdapterTraceLogger.LogInfo("Created unit-test runner {0}", source);
 
         // Default test set is filtered tests based on user provided filter criteria
@@ -363,7 +370,7 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
                 {
                     _testRunCancellationToken?.ThrowIfCancellationRequested();
 
-                    tasks.Add(_taskFactory(() =>
+                    tasks.Add(_taskFactory(async () =>
                     {
                         try
                         {
@@ -373,7 +380,7 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
 
                                 if (queue.TryDequeue(out IEnumerable<TestCase>? testSet))
                                 {
-                                    ExecuteTestsWithTestRunner(testSet, frameworkHandle, source, sourceLevelParameters, testRunner);
+                                    await ExecuteTestsWithTestRunnerAsync(testSet, frameworkHandle, source, sourceLevelParameters, testRunner, usesAppDomains);
                                 }
                             }
                         }
@@ -385,7 +392,7 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
 
                 try
                 {
-                    Task.WaitAll(tasks.ToArray());
+                    await Task.WhenAll(tasks);
                 }
                 catch (Exception ex)
                 {
@@ -399,12 +406,12 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
             // Queue the non parallel set
             if (nonParallelizableTestSet != null)
             {
-                ExecuteTestsWithTestRunner(nonParallelizableTestSet, frameworkHandle, source, sourceLevelParameters, testRunner);
+                await ExecuteTestsWithTestRunnerAsync(nonParallelizableTestSet, frameworkHandle, source, sourceLevelParameters, testRunner, usesAppDomains);
             }
         }
         else
         {
-            ExecuteTestsWithTestRunner(testsToRun, frameworkHandle, source, sourceLevelParameters, testRunner);
+            await ExecuteTestsWithTestRunnerAsync(testsToRun, frameworkHandle, source, sourceLevelParameters, testRunner, usesAppDomains);
         }
 
         if (PlatformServiceProvider.Instance.IsGracefulStopRequested)
@@ -415,12 +422,13 @@ private void ExecuteTestsInSource(IEnumerable<TestCase> tests, IRunContext? runC
         PlatformServiceProvider.Instance.AdapterTraceLogger.LogInfo("Executed tests belonging to source {0}", source);
     }
 
-    private void ExecuteTestsWithTestRunner(
+    private async Task ExecuteTestsWithTestRunnerAsync(
         IEnumerable<TestCase> tests,
         ITestExecutionRecorder testExecutionRecorder,
         string source,
         IDictionary<string, object> sourceLevelParameters,
-        UnitTestRunner testRunner)
+        UnitTestRunner testRunner,
+        bool usesAppDomains)
     {
         bool hasAnyRunnableTests = false;
         var fixtureTests = new List<TestCase>();
@@ -429,7 +437,11 @@ private void ExecuteTestsWithTestRunner(
             ? tests.OrderBy(t => t.GetManagedType()).ThenBy(t => t.GetManagedMethod())
             : tests;
 
-        var remotingMessageLogger = new RemotingMessageLogger(testExecutionRecorder);
+        // If testRunner is in a different AppDomain, we cannot pass the testExecutionRecorder directly.
+        // Instead, we pass a proxy (remoting object) that is marshallable by ref.
+        IMessageLogger remotingMessageLogger = usesAppDomains
+            ? new RemotingMessageLogger(testExecutionRecorder)
+            : testExecutionRecorder;
 
         foreach (TestCase currentTest in orderedTests)
         {
@@ -460,9 +472,18 @@ private void ExecuteTestsWithTestRunner(
             IDictionary<TestProperty, object?> tcmProperties = TcmTestPropertiesProvider.GetTcmProperties(currentTest);
             Dictionary<string, object?> testContextProperties = GetTestContextProperties(tcmProperties, sourceLevelParameters);
 
-            // testRunner could be in a different AppDomain. We cannot pass the testExecutionRecorder directly.
-            // Instead, we pass a proxy (remoting object) that is marshallable by ref.
-            UnitTestResult[] unitTestResult = testRunner.RunSingleTest(unitTestElement.TestMethod, testContextProperties, remotingMessageLogger);
+            UnitTestResult[] unitTestResult;
+            if (usesAppDomains)
+            {
+#pragma warning disable VSTHRD103 // Call async methods when in an async method - We cannot do right now because we are crossing app domains.
+                // TODO: When app domains support is dropped, we can finally always be calling the async version.
+                unitTestResult = testRunner.RunSingleTest(unitTestElement.TestMethod, testContextProperties, remotingMessageLogger);
+#pragma warning restore VSTHRD103 // Call async methods when in an async method
+            }
+            else
+            {
+                unitTestResult = await testRunner.RunSingleTestAsync(unitTestElement.TestMethod, testContextProperties, remotingMessageLogger);
+            }
 
             PlatformServiceProvider.Instance.AdapterTraceLogger.LogInfo("Executed test {0}", unitTestElement.TestMethod.Name);
 

src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs

@@ -46,6 +46,7 @@ internal TestMethodInfo(
         Parent = parent;
         TestMethodOptions = testMethodOptions;
         ExpectedException = ResolveExpectedException();
+        RetryAttribute = GetRetryAttribute();
     }
 
     /// <summary>
@@ -92,6 +93,8 @@ internal TestMethodInfo(
 
     internal ExpectedExceptionBaseAttribute? ExpectedException { get; set; /*set for testing only*/ }
 
+    internal RetryBaseAttribute? RetryAttribute { get; }
+
     public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.Instance.GetDerivedAttributes<Attribute>(TestMethod, inherit).ToArray();
 
     public TAttributeType[] GetAttributes<TAttributeType>(bool inherit)
@@ -249,17 +252,52 @@ public virtual TestResult Invoke(object?[]? arguments)
         // See https://github.com/microsoft/testfx/issues/4331
         if (expectedExceptions.Count() > 1)
         {
-            string errorMessage = string.Format(
-                CultureInfo.CurrentCulture,
-                Resource.UTA_MultipleExpectedExceptionsOnTestMethod,
-                Parent.ClassType.FullName,
-                TestMethod.Name);
-            throw new TypeInspectionException(errorMessage);
+            ThrowMultipleAttributesException(nameof(ExpectedExceptionBaseAttribute));
         }
 
         return expectedExceptions.FirstOrDefault();
     }
 
+    /// <summary>
+    /// Gets the number of retries this test method should make in case of failure.
+    /// </summary>
+    /// <returns>
+    /// The number of retries, which is always greater than or equal to 1.
+    /// If RetryAttribute is not present, returns 1.
+    /// </returns>
+    private RetryBaseAttribute? GetRetryAttribute()
+    {
+        IEnumerable<RetryBaseAttribute> attributes = ReflectHelper.Instance.GetDerivedAttributes<RetryBaseAttribute>(TestMethod, inherit: true);
+        using IEnumerator<RetryBaseAttribute> enumerator = attributes.GetEnumerator();
+        if (!enumerator.MoveNext())
+        {
+            return null;
+        }
+
+        RetryBaseAttribute attribute = enumerator.Current;
+
+        if (enumerator.MoveNext())
+        {
+            ThrowMultipleAttributesException(nameof(RetryBaseAttribute));
+        }
+
+        return attribute;
+    }
+
+    [DoesNotReturn]
+    private void ThrowMultipleAttributesException(string attributeName)
+    {
+        // Note: even if the given attribute has AllowMultiple = false, we can
+        // still reach here if a derived attribute authored by the user re-defines AttributeUsage
+        string errorMessage = string.Format(
+            CultureInfo.CurrentCulture,
+            Resource.UTA_MultipleAttributesOnTestMethod,
+            Parent.ClassType.FullName,
+            TestMethod.Name,
+            attributeName);
+        throw new TypeInspectionException(errorMessage);
+    }
+
     /// <summary>
     /// Execute test without timeout.
     /// </summary>

src/Adapter/MSTest.TestAdapter/Execution/TestMethodRunner.cs

@@ -173,6 +173,7 @@ internal List<TestResult> RunTestMethod()
         {
             if (_test.TestDataSourceIgnoreMessage is not null)
             {
+                _testContext.SetOutcome(UTF.UnitTestOutcome.Ignored);
                 return [new() { Outcome = UTF.UnitTestOutcome.Ignored, IgnoreReason = _test.TestDataSourceIgnoreMessage }];
             }
 

src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs

@@ -6,6 +6,7 @@
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Extensions;
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers;
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel;
+using Microsoft.VisualStudio.TestPlatform.MSTestAdapter;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Interface;
 using Microsoft.VisualStudio.TestPlatform.ObjectModel.Logging;
@@ -122,13 +123,19 @@ internal FixtureTestResult GetFixtureTestResult(TestMethod testMethod, string fi
         static UnitTestOutcome GetOutcome(Exception? exception) => exception == null ? UnitTestOutcome.Passed : UnitTestOutcome.Failed;
     }
 
+    // Task cannot cross app domains.
+    // For now, TestExecutionManager will call this sync method which is hacky.
+    // If we removed AppDomains in v4, we should use the async method and remove this one.
+    internal UnitTestResult[] RunSingleTest(TestMethod testMethod, IDictionary<string, object?> testContextProperties, IMessageLogger messageLogger)
+        => RunSingleTestAsync(testMethod, testContextProperties, messageLogger).GetAwaiter().GetResult();
+
     /// <summary>
     /// Runs a single test.
     /// </summary>
     /// <param name="testMethod"> The test Method. </param>
     /// <param name="testContextProperties"> The test context properties. </param>
     /// <returns> The <see cref="UnitTestResult"/>. </returns>
-    internal UnitTestResult[] RunSingleTest(TestMethod testMethod, IDictionary<string, object?> testContextProperties, IMessageLogger messageLogger)
+    internal async Task<UnitTestResult[]> RunSingleTestAsync(TestMethod testMethod, IDictionary<string, object?> testContextProperties, IMessageLogger messageLogger)
     {
         Guard.NotNull(testMethod);
 
@@ -179,8 +186,23 @@ internal UnitTestResult[] RunSingleTest(TestMethod testMethod, IDictionary<strin
                     {
                         // Run the test method
                         testContextForTestExecution.SetOutcome(testContextForClassInit.Context.CurrentTestOutcome);
+                        RetryBaseAttribute? retryAttribute = testMethodInfo.RetryAttribute;
                         var testMethodRunner = new TestMethodRunner(testMethodInfo, testMethod, testContextForTestExecution);
-                        result = testMethodRunner.Execute(classInitializeResult.StandardOut!, classInitializeResult.StandardError!, classInitializeResult.DebugTrace!, classInitializeResult.TestContextMessages!).ToUnitTestResults();
+                        List<TestResult> firstRunResult = testMethodRunner.Execute(classInitializeResult.StandardOut!, classInitializeResult.StandardError!, classInitializeResult.DebugTrace!, classInitializeResult.TestContextMessages!);
+                        result = firstRunResult.ToUnitTestResults();
+                        if (retryAttribute is not null && !RetryBaseAttribute.IsAcceptableResultForRetry(firstRunResult))
+                        {
+                            RetryResult retryResult = await retryAttribute.ExecuteAsync(
+                                new RetryContext(
+                                    () => Task.FromResult(
+                                        testMethodRunner.Execute(
+                                            classInitializeResult.StandardOut!,
+                                            classInitializeResult.StandardError!,
+                                            classInitializeResult.DebugTrace!,
+                                            classInitializeResult.TestContextMessages!).ToArray())));
+
+                            result = retryResult.TryGetLast()?.ToUnitTestResults() ?? throw ApplicationStateGuard.Unreachable();
+                        }
                     }
                 }
             }

src/Adapter/MSTest.TestAdapter/Resources/Resource.resx

@@ -267,8 +267,8 @@ Error: {1}</value>
     <value>The ExpectedException attribute defined on test method {0}.{1} threw an exception during construction.
 {2}</value>
   </data>
-  <data name="UTA_MultipleExpectedExceptionsOnTestMethod" xml:space="preserve">
-    <value>The test method {0}.{1} has multiple attributes derived from ExpectedExceptionBaseAttribute defined on it. Only one such attribute is allowed.</value>
+  <data name="UTA_MultipleAttributesOnTestMethod" xml:space="preserve">
+    <value>The test method {0}.{1} has multiple attributes derived from '{2}' defined on it. Only one such attribute is allowed.</value>
   </data>
   <data name="LegacyScenariosNotSupportedWarning" xml:space="preserve">
     <value>Warning : A testsettings file or a vsmdi file is not supported with the MSTest V2 Adapter.</value>
@@ -414,4 +414,4 @@ but received {4} argument(s), with types '{5}'.</value>
   <data name="GenericParameterConflict" xml:space="preserve">
     <value>Found two conflicting types for generic parameter '{0}'. The conflicting types are '{1}' and '{2}'.</value>
   </data>
-</root>
+</root>