Refactor unnecessary try/catch (#4057)

Co-authored-by: Amaury Levé <[email protected]>
microsoft · Nov 19, 2024 · bde4e5a · bde4e5a
1 parent c203c46
commit bde4e5a
Show file tree

Hide file tree

Showing 9 changed files with 159 additions and 41 deletions.
diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs
@@ -414,23 +414,22 @@ private static bool ProcessITestDataSourceTests(UnitTestElement test, Reflection
 
             // This code is to discover tests. To run the tests code is in TestMethodRunner.ExecuteDataSourceBasedTests.
             // Any change made here should be reflected in TestMethodRunner.ExecuteDataSourceBasedTests as well.
-            try
-            {
-                data = dataSource.GetData(methodInfo);
+            data = dataSource.GetData(methodInfo);
 
-                if (!data.Any())
+            if (!data.Any())
+            {
+                if (!MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive)
                 {
-                    throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, FrameworkMessages.DynamicDataIEnumerableEmpty, "GetData", dataSource.GetType().Name));
+                    throw dataSource.GetExceptionForEmptyDataSource(methodInfo);
                 }
-            }
-            catch (ArgumentException) when (MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive)
-            {
+
                 UnitTestElement discoveredTest = test.Clone();
                 // Make the test not data driven, because it had no data.
                 discoveredTest.TestMethod.DataType = DynamicDataType.None;
                 discoveredTest.DisplayName = dataSource.GetDisplayName(methodInfo, null) ?? discoveredTest.DisplayName;
 
                 tests.Add(discoveredTest);
+
                 continue;
             }
 

diff --git a/src/Adapter/MSTest.TestAdapter/DynamicDataOperations.cs b/src/Adapter/MSTest.TestAdapter/DynamicDataOperations.cs
@@ -78,16 +78,6 @@ public IEnumerable<object[]> GetData(Type? _dynamicDataDeclaringType, DynamicDat
                     _dynamicDataDeclaringType.FullName));
         }
 
-        if (!data.Any())
-        {
-            throw new ArgumentException(
-                string.Format(
-                    CultureInfo.InvariantCulture,
-                    FrameworkMessages.DynamicDataIEnumerableEmpty,
-                    _dynamicDataSourceName,
-                    _dynamicDataDeclaringType.FullName));
-        }
-
         // Data is valid, return it.
         return data;
     }

diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TestMethodRunner.cs b/src/Adapter/MSTest.TestAdapter/Execution/TestMethodRunner.cs
@@ -311,19 +311,18 @@ private bool ExecuteDataSourceBasedTests(List<TestResult> results)
                 {
                     isDataDriven = true;
                     IEnumerable<object?[]>? dataSource;
-                    try
-                    {
-                        // This code is to execute tests. To discover the tests code is in AssemblyEnumerator.ProcessTestDataSourceTests.
-                        // Any change made here should be reflected in AssemblyEnumerator.ProcessTestDataSourceTests as well.
-                        dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo);
 
-                        if (!dataSource.Any())
+                    // This code is to execute tests. To discover the tests code is in AssemblyEnumerator.ProcessTestDataSourceTests.
+                    // Any change made here should be reflected in AssemblyEnumerator.ProcessTestDataSourceTests as well.
+                    dataSource = testDataSource.GetData(_testMethodInfo.MethodInfo);
+
+                    if (!dataSource.Any())
+                    {
+                        if (!MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive)
                         {
-                            throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, FrameworkMessages.DynamicDataIEnumerableEmpty, "GetData", testDataSource.GetType().Name));
+                            throw testDataSource.GetExceptionForEmptyDataSource(_testMethodInfo.MethodInfo);
                         }
-                    }
-                    catch (Exception ex) when (ex is ArgumentException && MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive)
-                    {
+
                         var inconclusiveResult = new TestResult
                         {
                             Outcome = UTF.UnitTestOutcome.Inconclusive,

diff --git a/src/TestFramework/TestFramework/Attributes/DataSource/DynamicDataAttribute.cs b/src/TestFramework/TestFramework/Attributes/DataSource/DynamicDataAttribute.cs
@@ -33,7 +33,7 @@ public enum DynamicDataSourceType
 /// Attribute to define dynamic data for a test method.
 /// </summary>
 [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
-public sealed class DynamicDataAttribute : Attribute, ITestDataSource
+public sealed class DynamicDataAttribute : Attribute, ITestDataSource, ITestDataSourceEmptyDataSourceExceptionInfo
 {
     private readonly string _dynamicDataSourceName;
     private readonly DynamicDataSourceType _dynamicDataSourceType;
@@ -155,4 +155,14 @@ private static bool TryGetData(object dataSource, [NotNullWhen(true)] out IEnume
         data = null;
         return false;
     }
+
+    string? ITestDataSourceEmptyDataSourceExceptionInfo.GetPropertyOrMethodNameForEmptyDataSourceException()
+        => _dynamicDataSourceName;
+
+    string? ITestDataSourceEmptyDataSourceExceptionInfo.GetPropertyOrMethodContainerTypeNameForEmptyDataSourceException(MethodInfo testMethodInfo)
+    {
+        Type? type = _dynamicDataDeclaringType ?? testMethodInfo.DeclaringType;
+        DebugEx.Assert(type is not null, "Declaring type of test data cannot be null.");
+        return type.FullName;
+    }
 }
diff --git a/src/TestFramework/TestFramework/Interfaces/EmptyDataSourceExceptionInfoExtensions.cs b/src/TestFramework/TestFramework/Interfaces/EmptyDataSourceExceptionInfoExtensions.cs
@@ -0,0 +1,28 @@
+// 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.Globalization;
+using System.Reflection;
+
+namespace Microsoft.VisualStudio.TestTools.UnitTesting;
+
+internal static class EmptyDataSourceExceptionInfoExtensions
+{
+    internal static ArgumentException GetExceptionForEmptyDataSource(this ITestDataSource dataSource, MethodInfo testMethodInfo)
+        => dataSource is ITestDataSourceEmptyDataSourceExceptionInfo info
+            ? info.GetExceptionForEmptyDataSource(testMethodInfo)
+            : new ArgumentException(
+                string.Format(
+                    CultureInfo.InvariantCulture,
+                    FrameworkMessages.DynamicDataIEnumerableEmpty,
+                    "GetData",
+                    dataSource.GetType().Name));
+
+    private static ArgumentException GetExceptionForEmptyDataSource(this ITestDataSourceEmptyDataSourceExceptionInfo info, MethodInfo testMethodInfo)
+        => new(
+            string.Format(
+                CultureInfo.InvariantCulture,
+                FrameworkMessages.DynamicDataIEnumerableEmpty,
+                info.GetPropertyOrMethodNameForEmptyDataSourceException(),
+                info.GetPropertyOrMethodContainerTypeNameForEmptyDataSourceException(testMethodInfo)));
+}
diff --git a/src/TestFramework/TestFramework/Interfaces/ITestDataSourceEmptyDataSourceExceptionInfo.cs b/src/TestFramework/TestFramework/Interfaces/ITestDataSourceEmptyDataSourceExceptionInfo.cs
@@ -0,0 +1,20 @@
+// 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.Reflection;
+
+namespace Microsoft.VisualStudio.TestTools.UnitTesting;
+
+internal interface ITestDataSourceEmptyDataSourceExceptionInfo
+{
+    /// <summary>
+    /// Returns the method/property name accessed by this data source.
+    /// For example, for DynamicDataAttribute, that will be attribute argument.
+    /// </summary>
+    string? GetPropertyOrMethodNameForEmptyDataSourceException();
+
+    /// <summary>
+    /// Returns the type name on which the method/property accessed by this data source exists.
+    /// </summary>
+    string? GetPropertyOrMethodContainerTypeNameForEmptyDataSourceException(MethodInfo testMethodInfo);
+}
diff --git a/test/UnitTests/MSTestAdapter.UnitTests/DynamicDataAttributeTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/DynamicDataAttributeTests.cs
@@ -80,13 +80,15 @@ public void GetDataShouldThrowExceptionIfPropertyReturnsNull() =>
             _dynamicDataAttribute.GetData(methodInfo);
         });
 
-    public void GetDataShouldThrowExceptionIfPropertyReturnsEmpty() =>
-        VerifyThrows<ArgumentException>(() =>
-        {
-            MethodInfo methodInfo = _dummyTestClass.GetType().GetTypeInfo().GetDeclaredMethod("TestMethod5");
-            _dynamicDataAttribute = new DynamicDataAttribute("EmptyProperty", typeof(DummyTestClass));
-            _dynamicDataAttribute.GetData(methodInfo);
-        });
+    public void GetDataShouldNotThrowExceptionIfPropertyReturnsEmpty()
+    {
+        MethodInfo methodInfo = _dummyTestClass.GetType().GetTypeInfo().GetDeclaredMethod("TestMethod5");
+        _dynamicDataAttribute = new DynamicDataAttribute("EmptyProperty", typeof(DummyTestClass));
+        IEnumerable<object[]> data = _dynamicDataAttribute.GetData(methodInfo);
+        // The callers in AssemblyEnumerator and TestMethodRunner are responsible
+        // for throwing an exception if data is empty and ConsiderEmptyDataSourceAsInconclusive is false.
+        Verify(!data.Any());
+    }
 
     public void GetDataShouldThrowExceptionIfPropertyDoesNotReturnCorrectType() =>
         VerifyThrows<ArgumentNullException>(() =>

diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System.Diagnostics.CodeAnalysis;
+using System.Globalization;
 using System.Reflection;
 using System.Text;
 
@@ -11,6 +12,7 @@
 using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices;
 using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.UnitTests.TestableImplementations;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 using Moq;
 
@@ -40,16 +42,12 @@ public class TestMethodRunnerTests : TestContainer
 
     public TestMethodRunnerTests()
     {
-        ConstructorInfo constructorInfo = typeof(DummyTestClass).GetConstructor([])!;
         _methodInfo = typeof(DummyTestClass).GetMethods().Single(m => m.Name.Equals("DummyTestMethod", StringComparison.Ordinal));
-        var classAttribute = new UTF.TestClassAttribute();
         _testMethodAttribute = new UTF.TestMethodAttribute();
-        PropertyInfo testContextProperty = typeof(DummyTestClass).GetProperty("TestContext");
 
-        var testAssemblyInfo = new TestAssemblyInfo(typeof(DummyTestClass).Assembly);
         _testMethod = new TestMethod("dummyTestName", "dummyClassName", "dummyAssemblyName", false);
         _testContextImplementation = new TestContextImplementation(_testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary<string, object>());
-        _testClassInfo = new TestClassInfo(typeof(DummyTestClass), constructorInfo, true, testContextProperty, classAttribute, testAssemblyInfo);
+        _testClassInfo = GetTestClassInfo<DummyTestClass>();
 
         _testMethodOptions = new TestMethodOptions(TimeoutInfo.FromTimeout(200), null, _testContextImplementation, false, _testMethodAttribute);
 
@@ -68,6 +66,15 @@ public TestMethodRunnerTests()
         ReflectHelper.Instance.ClearCache();
     }
 
+    private static TestClassInfo GetTestClassInfo<T>()
+    {
+        ConstructorInfo constructorInfo = typeof(T).GetConstructor([])!;
+        PropertyInfo testContextProperty = typeof(T).GetProperty("TestContext");
+        var classAttribute = new UTF.TestClassAttribute();
+        var testAssemblyInfo = new TestAssemblyInfo(typeof(T).Assembly);
+        return new TestClassInfo(typeof(T), constructorInfo, isParameterlessConstructor: true, testContextProperty, classAttribute, testAssemblyInfo);
+    }
+
     protected override void Dispose(bool disposing)
     {
         if (!IsDisposed)
@@ -369,6 +376,56 @@ public void RunTestMethodShouldSetResultFilesIfPresentForDataDrivenTests()
         Verify(results[1].ResultFiles.ToList().Contains("C:\\temp.txt"));
     }
 
+    public void RunTestMethodWithEmptyDataSourceShouldFailBecauseConsiderEmptyDataSourceAsInconclusiveIsFalse()
+        => RunTestMethodWithEmptyDataSourceShouldFailIfConsiderEmptyDataSourceAsInconclusiveIsNotTrueHelper(false);
+
+    public void RunTestMethodWithEmptyDataSourceShouldNotFailBecauseConsiderEmptyDataSourceAsInconclusiveIsTrue()
+        => RunTestMethodWithEmptyDataSourceShouldFailIfConsiderEmptyDataSourceAsInconclusiveIsNotTrueHelper(true);
+
+    private void RunTestMethodWithEmptyDataSourceShouldFailIfConsiderEmptyDataSourceAsInconclusiveIsNotTrueHelper(bool considerEmptyAsInconclusive)
+    {
+        Mock<PlatformServices.Interface.IReflectionOperations2> existingMock = _testablePlatformServiceProvider.MockReflectionOperations;
+        try
+        {
+            // We want this test to go through the "real" reflection to hit the product code path relevant for the test.
+            _testablePlatformServiceProvider.MockReflectionOperations = null;
+
+            string xml =
+                $$"""
+                <RunSettings>
+                    <MSTestV2>
+                        <ConsiderEmptyDataSourceAsInconclusive>{{considerEmptyAsInconclusive}}</ConsiderEmptyDataSourceAsInconclusive>
+                    </MSTestV2>
+                </RunSettings>
+                """;
+
+            var settings = MSTestSettings.GetSettings(xml, MSTestSettings.SettingsNameAlias, null);
+            MSTestSettings.PopulateSettings(settings);
+
+            var testMethodInfo = new TestableTestMethodInfo(
+                typeof(DummyTestClassEmptyDataSource).GetMethod(nameof(DummyTestClassEmptyDataSource.TestMethod)),
+                GetTestClassInfo<DummyTestClassEmptyDataSource>(),
+                _testMethodOptions,
+                () => throw ApplicationStateGuard.Unreachable());
+            var testMethodRunner = new TestMethodRunner(testMethodInfo, _testMethod, _testContextImplementation);
+
+            if (considerEmptyAsInconclusive)
+            {
+                UnitTestResult[] results = testMethodRunner.RunTestMethod();
+                Verify(results[0].Outcome == AdapterTestOutcome.Inconclusive);
+            }
+            else
+            {
+                ArgumentException thrownException = VerifyThrows<ArgumentException>(() => testMethodRunner.RunTestMethod());
+                Verify(thrownException.Message == string.Format(CultureInfo.InvariantCulture, FrameworkMessages.DynamicDataIEnumerableEmpty, nameof(DummyTestClassEmptyDataSource.EmptyProperty), typeof(DummyTestClassEmptyDataSource).FullName));
+            }
+        }
+        finally
+        {
+            _testablePlatformServiceProvider.MockReflectionOperations = existingMock;
+        }
+    }
+
     #region Test data
 
     [SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "Use through reflection")]
@@ -451,5 +508,15 @@ public class DummyTestClassWithTestContextWithoutSetter
         public UTFExtension.TestContext TestContext { get; }
     }
 
+    public class DummyTestClassEmptyDataSource
+    {
+        public static IEnumerable<object[]> EmptyProperty => Array.Empty<object[]>();
+
+        [DynamicData("EmptyProperty")]
+        public void TestMethod(int x)
+        {
+        }
+    }
+
     #endregion
 }
diff --git a/...tTests/MSTestAdapter.UnitTests/TestableImplementations/TestablePlatformServiceProvider.cs b/...tTests/MSTestAdapter.UnitTests/TestableImplementations/TestablePlatformServiceProvider.cs
@@ -26,6 +26,9 @@ public TestablePlatformServiceProvider()
         MockTraceListener = new Mock<ITraceListener>();
         MockTraceListenerManager = new Mock<ITraceListenerManager>();
         MockThreadOperations = new Mock<IThreadOperations>();
+        TestTools.UnitTesting.DynamicDataProvider.Instance = SourceGeneratorToggle.UseSourceGenerator
+            ? new SourceGeneratedDynamicDataOperations()
+            : new DynamicDataOperations();
     }
 
     #region Mock Implementations