From 78d2ce27136641b029fc2fc4b1e4eecae467f2b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Fri, 3 May 2024 11:27:19 +0200 Subject: [PATCH 01/20] Add timing code to discovery --- .../MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs index 7b51db7095..69685bf473 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs @@ -1,6 +1,7 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Diagnostics; using System.Globalization; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery; @@ -58,7 +59,11 @@ internal UnitTestDiscoverer() ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext) { + TimeSpan getTests; + var sw = Stopwatch.StartNew(); ICollection? testElements = _assemblyEnumeratorWrapper.GetTests(source, discoveryContext?.RunSettings, out ICollection? warnings); + getTests = sw.Elapsed; + sw.Restart(); bool treatDiscoveryWarningsAsErrors = MSTestSettings.CurrentSettings.TreatDiscoveryWarningsAsErrors; @@ -85,6 +90,9 @@ internal UnitTestDiscoverer() source); SendTestCases(source, testElements, discoverySink, discoveryContext, logger); + TimeSpan sendOverhead = sw.Elapsed; + Debug.WriteLine(">>>>>>>>>>>>>>>>>>>>>>>>>>>"); + Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds}"); } internal void SendTestCases(string source, IEnumerable testElements, ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext, IMessageLogger logger) From 04514260b3b2487f322a4b9f4fbae188f3f289ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 9 May 2024 18:05:56 +0200 Subject: [PATCH 02/20] changes so far --- .../MSTest.TestAdapter/BannedSymbols.txt | 1 + .../Discovery/AssemblyEnumerator.cs | 6 +- .../Discovery/TestMethodValidator.cs | 7 +- .../Discovery/TypeEnumerator.cs | 14 +- .../Discovery/TypeValidator.cs | 4 +- .../Discovery/UnitTestDiscoverer.cs | 15 +- .../Execution/TestMethodInfo.cs | 11 +- .../MSTest.TestAdapter/Execution/TypeCache.cs | 63 +- .../Execution/UnitTestRunner.cs | 10 +- .../Extensions/MethodInfoExtensions.cs | 15 +- .../Helpers/ReflectHelper.cs | 586 +++++++++++------- .../MSTest.TestAdapter.csproj | 6 +- .../Interfaces/IReflectionOperations.cs | 2 + .../Services/ReflectionOperations.cs | 3 + .../Utilities/DeploymentItemUtility.cs | 3 +- .../TestMethod/ExpectedExceptionAttribute.cs | 2 +- .../Discovery/TestMethodValidatorTests.cs | 4 +- .../Discovery/TypeEnumeratorTests.cs | 192 +++--- .../Discovery/TypeValidatorTests.cs | 8 +- .../Execution/TypeCacheTests.cs | 140 ++--- .../Execution/UnitTestRunnerTests.cs | 2 +- .../Helpers/ReflectHelperTests.cs | 56 +- .../TestableReflectHelper.cs | 29 +- 23 files changed, 656 insertions(+), 523 deletions(-) create mode 100644 src/Adapter/MSTest.TestAdapter/BannedSymbols.txt diff --git a/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt new file mode 100644 index 0000000000..e0d4601a41 --- /dev/null +++ b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt @@ -0,0 +1 @@ +T:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.NotCachedReflectHelper; This is allowed only for special cases that need to scan all types or methods to see if they are potentially a test class or a test method. Any use on a test class or method should use methods on ReflectHelper because it has cache. diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index bf0ec5d305..a53329eeee 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -219,7 +219,7 @@ internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFile typeFullName = type.FullName; TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals, testIdGenerationStrategy); ICollection? unitTestCases = testTypeEnumerator.Enumerate(out ICollection? warningsFromTypeEnumerator); - bool typeIgnored = ReflectHelper.IsAttributeDefined(type, false); + bool typeIgnored = ReflectHelper.IsNonDerivedAttributeDefined(type, false); if (warningsFromTypeEnumerator != null) { @@ -275,7 +275,9 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMethodInfo testMethodInfo, List tests) { MethodInfo methodInfo = testMethodInfo.MethodInfo; - IEnumerable? testDataSources = ReflectHelper.GetAttributes(methodInfo, false)?.OfType(); + // TODO: this needs a special method where we grab all attributes and compare them to a type to see if the attribute inherits from additional interface. + // We want to do first or default here, to see if we should go to the expensive path. + IEnumerable? testDataSources = ReflectHelper.Instance.GetNonDerivedAttributes(methodInfo, inherit: false)?.OfType(); if (testDataSources == null || !testDataSources.Any()) { return false; diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs index eac9692d4f..10be10ad99 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -48,8 +48,9 @@ internal TestMethodValidator(ReflectHelper reflectHelper, bool discoverInternals /// Return true if a method is a valid test method. internal virtual bool IsValidTestMethod(MethodInfo testMethodInfo, Type type, ICollection warnings) { - if (!_reflectHelper.IsAttributeDefined(testMethodInfo, false) - && !_reflectHelper.HasAttributeDerivedFrom(testMethodInfo, false)) + // Use non-caching reflection helper to check if a method is a valid test method. We don't want to retrieve + // all attributes for every single method, and we also don't want to cache them for methods that we won't look at again. + if (!_reflectHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(testMethodInfo, inherit: false)) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 0a1097105f..4b8b0ea35c 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -151,10 +151,10 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT { // Get compiler generated type name for async test method (either void returning or task returning). AsyncTypeName = method.GetAsyncTypeName(), - TestCategory = _reflectHelper.GetCategories(method, _type), + TestCategory = _reflectHelper.GetTestCategories(method, _type), DoNotParallelize = _reflectHelper.IsDoNotParallelizeSet(method, _type), Priority = _reflectHelper.GetPriority(method), - Ignored = _reflectHelper.IsAttributeDefined(method, false), + Ignored = _reflectHelper.IsNonDerivedAttributeDefined(method, inherit: false), DeploymentItems = PlatformServiceProvider.Instance.TestDeployment.GetDeploymentItems(method, _type, warnings), }; @@ -174,29 +174,29 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT testElement.Traits = traits.ToArray(); - if (_reflectHelper.GetCustomAttribute(method) is CssIterationAttribute cssIteration) + if (_reflectHelper.GetFirstDerivedAttributeOrDefault(method, inherit: true) is CssIterationAttribute cssIteration) { testElement.CssIteration = cssIteration.CssIteration; } - if (_reflectHelper.GetCustomAttribute(method) is CssProjectStructureAttribute cssProjectStructure) + if (_reflectHelper.GetFirstDerivedAttributeOrDefault(method, inherit: true) is CssProjectStructureAttribute cssProjectStructure) { testElement.CssProjectStructure = cssProjectStructure.CssProjectStructure; } - if (_reflectHelper.GetCustomAttribute(method) is DescriptionAttribute descriptionAttribute) + if (_reflectHelper.GetFirstDerivedAttributeOrDefault(method, inherit: true) is DescriptionAttribute descriptionAttribute) { testElement.Description = descriptionAttribute.Description; } - WorkItemAttribute[] workItemAttributes = _reflectHelper.GetCustomAttributes(method); + WorkItemAttribute[] workItemAttributes = _reflectHelper.GetDerivedAttributes(method, inherit: true).ToArray(); if (workItemAttributes.Length != 0) { testElement.WorkItemIds = workItemAttributes.Select(x => x.Id.ToString(CultureInfo.InvariantCulture)).ToArray(); } // get DisplayName from TestMethodAttribute (or any inherited attribute) - TestMethodAttribute? testMethodAttribute = _reflectHelper.GetCustomAttribute(method); + TestMethodAttribute? testMethodAttribute = _reflectHelper.GetFirstDerivedAttributeOrDefault(method, inherit: true); testElement.DisplayName = testMethodAttribute?.DisplayName ?? method.Name; return testElement; diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs index 991d6ade55..04e5d60332 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs @@ -51,9 +51,7 @@ internal virtual bool IsValidTestClass(Type type, ICollection warnings) { TypeInfo typeInfo = type.GetTypeInfo(); - if (!typeInfo.IsClass - || (!_reflectHelper.IsAttributeDefined(typeInfo, false) - && !_reflectHelper.HasAttributeDerivedFrom(typeInfo, false))) + if (!typeInfo.IsClass || !_reflectHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(typeInfo, false)) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs index 69685bf473..e325d3846d 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs @@ -61,8 +61,14 @@ internal UnitTestDiscoverer() { TimeSpan getTests; var sw = Stopwatch.StartNew(); +#if NET6_0_OR_GREATER + long beforeDiscovery = GC.GetTotalAllocatedBytes(); +#endif ICollection? testElements = _assemblyEnumeratorWrapper.GetTests(source, discoveryContext?.RunSettings, out ICollection? warnings); getTests = sw.Elapsed; +#if NET6_0_OR_GREATER + long afterDiscovery = GC.GetTotalAllocatedBytes(); +#endif sw.Restart(); bool treatDiscoveryWarningsAsErrors = MSTestSettings.CurrentSettings.TreatDiscoveryWarningsAsErrors; @@ -91,8 +97,13 @@ internal UnitTestDiscoverer() SendTestCases(source, testElements, discoverySink, discoveryContext, logger); TimeSpan sendOverhead = sw.Elapsed; - Debug.WriteLine(">>>>>>>>>>>>>>>>>>>>>>>>>>>"); - Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds}"); +#if NET6_0_OR_GREATER + long afterSend = GC.GetTotalAllocatedBytes(); +#endif + Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds} <<<"); +#if NET6_0_OR_GREATER + Console.WriteLine($"discovered: discovery alloc: {(afterDiscovery - beforeDiscovery) / 1024 * 1024} MB send alloc: {(afterSend- afterDiscovery) / 1024 * 1024} MB"); +#endif } internal void SendTestCases(string source, IEnumerable testElements, ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext, IMessageLogger logger) diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs b/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs index 1be63d983e..4ff5bdefb9 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Diagnostics; @@ -82,11 +82,16 @@ public class TestMethodInfo : ITestMethod /// internal TestMethodOptions TestMethodOptions { get; } - public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.GetCustomAttributes(TestMethod, inherit) as Attribute[]; + // This is allowed because we don't know the usage. +#pragma warning disable RS0030 // Do not use banned APIs + [Obsolete("don't use, but cannot remove")] + public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached(TestMethod, inherit) as Attribute[]; +#pragma warning restore RS0030 // Do not use banned APIs + [Obsolete("don't use, but cannot remove")] public TAttributeType[] GetAttributes(bool inherit) where TAttributeType : Attribute - => ReflectHelper.GetAttributes(TestMethod, inherit) + => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached(TestMethod, inherit) ?? Array.Empty(); /// diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index fd8e9b575f..a998477920 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -290,7 +290,7 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) TestAssemblyInfo assemblyInfo = GetAssemblyInfo(classType); - TestClassAttribute? testClassAttribute = ReflectHelper.GetDerivedAttribute(classType, false); + TestClassAttribute? testClassAttribute = ReflectHelper.Instance.GetDerivedAttribute(classType, false); DebugEx.Assert(testClassAttribute is not null, "testClassAttribute is null"); var classInfo = new TestClassInfo(classType, constructor, testContextProperty, testClassAttribute, assemblyInfo); @@ -402,8 +402,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) { // Only examine classes which are TestClass or derives from TestClass attribute TypeInfo typeInfo = t.GetTypeInfo(); - if (!_reflectionHelper.IsAttributeDefined(typeInfo, inherit: true) && - !_reflectionHelper.HasAttributeDerivedFrom(typeInfo, true)) + if (!_reflectionHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(typeInfo, inherit: true)) { continue; } @@ -426,16 +425,15 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) { assemblyInfo.AssemblyInitializeMethod = methodInfo; - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); assemblyInfo.AssemblyInitializeMethodTimeoutMilliseconds = timeoutAttribute.Timeout; } else if (MSTestSettings.CurrentSettings.AssemblyInitializeTimeout > 0) @@ -446,16 +444,15 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) else if (IsAssemblyOrClassCleanupMethod(methodInfo)) { assemblyInfo.AssemblyCleanupMethod = methodInfo; - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); assemblyInfo.AssemblyCleanupMethodTimeoutMilliseconds = timeoutAttribute.Timeout; } else if (MSTestSettings.CurrentSettings.AssemblyCleanupTimeout > 0) @@ -480,7 +477,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) private bool IsAssemblyOrClassInitializeMethod(MethodInfo methodInfo) where TInitializeAttribute : Attribute { - if (!_reflectionHelper.IsAttributeDefined(methodInfo, false)) + if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; } @@ -503,7 +500,7 @@ private bool IsAssemblyOrClassInitializeMethod(MethodInfo private bool IsAssemblyOrClassCleanupMethod(MethodInfo methodInfo) where TCleanupAttribute : Attribute { - if (!_reflectionHelper.IsAttributeDefined(methodInfo, false)) + if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; } @@ -557,16 +554,15 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isInitializeMethod) { - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); classInfo.ClassInitializeMethodTimeoutMilliseconds.Add(methodInfo, timeoutAttribute.Timeout); } else if (MSTestSettings.CurrentSettings.ClassInitializeTimeout > 0) @@ -576,7 +572,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isBase) { - if (_reflectionHelper.GetCustomAttribute(methodInfo)! + if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)! .InheritanceBehavior == InheritanceBehavior.BeforeEachDerivedClass) { initAndCleanupMethods[0] = methodInfo; @@ -591,16 +587,15 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isCleanupMethod) { - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); classInfo.ClassCleanupMethodTimeoutMilliseconds.Add(methodInfo, timeoutAttribute.Timeout); } else if (MSTestSettings.CurrentSettings.ClassCleanupTimeout > 0) @@ -610,7 +605,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isBase) { - if (_reflectionHelper.GetCustomAttribute(methodInfo)! + if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)! .InheritanceBehavior == InheritanceBehavior.BeforeEachDerivedClass) { initAndCleanupMethods[1] = methodInfo; @@ -637,8 +632,8 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method bool isBase, Dictionary instanceMethods) { - bool hasTestInitialize = _reflectionHelper.IsAttributeDefined(methodInfo, inherit: false); - bool hasTestCleanup = _reflectionHelper.IsAttributeDefined(methodInfo, inherit: false); + bool hasTestInitialize = _reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, inherit: false); + bool hasTestCleanup = _reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, inherit: false); if (!hasTestCleanup && !hasTestInitialize) { @@ -658,16 +653,15 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (hasTestInitialize) { - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); classInfo.TestInitializeMethodTimeoutMilliseconds.Add(methodInfo, timeoutAttribute.Timeout); } else if (MSTestSettings.CurrentSettings.TestInitializeTimeout > 0) @@ -690,16 +684,15 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (hasTestCleanup) { - if (_reflectionHelper.IsAttributeDefined(methodInfo, false)) + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, methodInfo.DeclaringType!.FullName, methodInfo.Name); throw new TypeInspectionException(message); } - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); - DebugEx.Assert(timeoutAttribute != null, "TimeoutAttribute cannot be null"); classInfo.TestCleanupMethodTimeoutMilliseconds.Add(methodInfo, timeoutAttribute.Timeout); } else if (MSTestSettings.CurrentSettings.TestCleanupTimeout > 0) @@ -878,12 +871,12 @@ private MethodInfo GetMethodInfoForTestMethod(TestMethod testMethod, TestClassIn private int GetTestTimeout(MethodInfo methodInfo, TestMethod testMethod) { DebugEx.Assert(methodInfo != null, "TestMethod should be non-null"); - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetAttribute(methodInfo); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); int globalTimeout = MSTestSettings.CurrentSettings.TestTimeout; if (timeoutAttribute != null) { - if (!methodInfo.HasCorrectTimeout()) + if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) { string message = string.Format(CultureInfo.CurrentCulture, Resource.UTA_ErrorInvalidTimeout, testMethod.FullClassName, testMethod.Name); throw new TypeInspectionException(message); diff --git a/src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs b/src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs index 2bae363395..c2505875ed 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/UnitTestRunner.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Concurrent; @@ -254,9 +254,9 @@ private void RunRequiredCleanups(ITestContext testContext, TestMethodInfo? testM string? ignoreMessage = null; bool isIgnoreAttributeOnClass = - _reflectHelper.IsAttributeDefined(testMethodInfo.Parent.ClassType, false); + _reflectHelper.IsNonDerivedAttributeDefined(testMethodInfo.Parent.ClassType, false); bool isIgnoreAttributeOnMethod = - _reflectHelper.IsAttributeDefined(testMethodInfo.TestMethod, false); + _reflectHelper.IsNonDerivedAttributeDefined(testMethodInfo.TestMethod, false); if (isIgnoreAttributeOnClass) { @@ -291,7 +291,7 @@ private class ClassCleanupManager IEnumerable testsToRun, ClassCleanupBehavior? lifecycleFromMsTest, ClassCleanupBehavior lifecycleFromAssembly, - ReflectHelper? reflectHelper = null) + ReflectHelper reflectHelper) { _remainingTestsByClass = new(testsToRun.GroupBy(t => t.TestMethod.FullClassName) @@ -300,7 +300,7 @@ private class ClassCleanupManager g => new HashSet(g.Select(t => t.TestMethod.UniqueName)))); _lifecycleFromMsTest = lifecycleFromMsTest; _lifecycleFromAssembly = lifecycleFromAssembly; - _reflectHelper = reflectHelper ?? new ReflectHelper(); + _reflectHelper = reflectHelper; } public void MarkTestComplete(TestMethodInfo testMethodInfo, TestMethod testMethod, out bool shouldRunEndOfClassCleanup, diff --git a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs index e240c1e6db..2f9ece4e34 100644 --- a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs +++ b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs @@ -89,20 +89,17 @@ internal static bool HasCorrectTestMethodSignature(this MethodInfo method, bool /// Checks whether test method has correct Timeout attribute. /// /// The method to verify. + /// The timeout attribute when we already have it. /// True if the method has the right test timeout signature. - internal static bool HasCorrectTimeout(this MethodInfo method) + internal static bool HasCorrectTimeout(this MethodInfo method, TimeoutAttribute? timeoutAttribute = null) { DebugEx.Assert(method != null, "method should not be null."); - // There should be one and only one TimeoutAttribute. - TimeoutAttribute[] attributes = ReflectHelper.GetCustomAttributes(method, false); - if (attributes?.Length != 1) - { - return false; - } + // TODO: redesign this, probably change this to GetTimeout? so we don't have to do this weird dance? + timeoutAttribute ??= ReflectHelper.Instance.GetSingleNonDerivedAttributeOrDefault(method, inherit: false, nullOnMultiple: true); // Timeout cannot be less than 0. - return !(attributes[0]?.Timeout < 0); + return !(timeoutAttribute?.Timeout < 0); } /// @@ -125,7 +122,7 @@ internal static bool IsValidReturnType(this MethodInfo method) /// Compiler generated type name for given async test method.. internal static string? GetAsyncTypeName(this MethodInfo method) { - AsyncStateMachineAttribute? asyncStateMachineAttribute = ReflectHelper.GetCustomAttributes(method, false).FirstOrDefault(); + AsyncStateMachineAttribute? asyncStateMachineAttribute = ReflectHelper.Instance.GetFirstNonDerivedAttributeOrDefault(method, inherit: false); return asyncStateMachineAttribute?.StateMachineType?.FullName; } diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index ac1a2be06e..adb0a2d3ca 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Diagnostics.CodeAnalysis; @@ -15,27 +15,32 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; internal class ReflectHelper : MarshalByRefObject { - private static readonly Lazy InstanceValue = new(() => new ReflectHelper()); + // It is okay to use this api here, because we need to access the real reflection. +#pragma warning disable RS0030 // Do not use banned APIs + private static readonly Lazy InstanceValue = new(() => new ReflectHelper(new NotCachedReflectHelper())); +#pragma warning restore RS0030 // Do not use banned APIs - /// - /// Contains the memberInfo Vs the name/type of the attributes defined on that member. (FYI: - MemberInfo denotes properties, fields, methods, events). - /// - private readonly Dictionary> _attributeCache = []; + // Caches below could be unified by using ICustomAttributeProvider. But the underlying IReflectionOperations is public and we would have to change or duplicate it. + private readonly Dictionary> _inheritedAttributeCache = []; + private readonly Dictionary> _nonInheritedAttributeCache = []; - internal ReflectHelper() - { - } + internal /* for tests only */ ReflectHelper(INotCachedReflectHelper notCachedReflectHelper) => + NotCachedReflectHelper = notCachedReflectHelper; + + private readonly AttributeComparer _attributeComparer = new(); public static ReflectHelper Instance => InstanceValue.Value; + public INotCachedReflectHelper NotCachedReflectHelper { get; } + /// - /// Checks to see if the parameter memberInfo contains the parameter attribute or not. + /// Checks to see if a member or type is decorated with the given attribute. The type is checked exactly. If attribute is derived (inherits from) a class, e.g. [MyTestClass] from [TestClass] it won't match if you look for [TestClass]. The inherit parameter does not impact this checking. /// - /// Attribute to search for. + /// Attribute to search for by fully qualified name. /// Member/Type to test. - /// Look through inheritance or not. - /// True if the attribute of the specified type is defined. - public virtual bool IsAttributeDefined(MemberInfo memberInfo, bool inherit) + /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. + /// True if the attribute of the specified type is defined on this member or a class. + public virtual bool IsNonDerivedAttributeDefined(MemberInfo memberInfo, bool inherit) where TAttribute : Attribute { if (memberInfo == null) @@ -44,12 +49,19 @@ public virtual bool IsAttributeDefined(MemberInfo memberInfo, bool i } // Get attributes defined on the member from the cache. - Dictionary? attributes = GetAttributes(memberInfo, inherit); + Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); string requiredAttributeQualifiedName = typeof(TAttribute).AssemblyQualifiedName!; if (attributes == null) { + // TODO: + bool a = true; + if (a) + { + throw new NotSupportedException("THIS FALLBACK!"); + } + // If we could not obtain all attributes from cache, just get the one we need. - TAttribute[] specificAttributes = GetCustomAttributes(memberInfo, inherit); + TAttribute[] specificAttributes = GetCustomAttributesOfTypeNotCached(memberInfo, inherit); return specificAttributes.Any(a => string.Equals(a.GetType().AssemblyQualifiedName, requiredAttributeQualifiedName, StringComparison.Ordinal)); } @@ -57,57 +69,35 @@ public virtual bool IsAttributeDefined(MemberInfo memberInfo, bool i } /// - /// Checks to see if the parameter memberInfo contains the parameter attribute or not. + /// Checks to see if a member or type is decorated with the given attribute. The type is checked exactly. If attribute is derived (inherits from) a class, e.g. [MyTestClass] from [TestClass] it won't match if you look for [TestClass]. The inherit parameter does not impact this checking. /// - /// Attribute to search for. - /// Member/Type to test. - /// Look through inheritance or not. - /// True if the specified attribute is defined on the type. - public virtual bool IsAttributeDefined(Type type, bool inherit) + /// Attribute to search for by fully qualified name. + /// Type to test. + /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. + /// True if the attribute of the specified type is defined on this member or a class. + public virtual bool IsNonDerivedAttributeDefined(Type type, bool inherit) where TAttribute : Attribute - => IsAttributeDefined((MemberInfo)type.GetTypeInfo(), inherit); + => IsNonDerivedAttributeDefined((MemberInfo)type, inherit); /// - /// Checks to see if the parameter memberInfo contains the parameter attribute or not. + /// Checks to see if a member or type is decorated with the given attribute, or an attribute that derives from it. e.g. [MyTestClass] from [TestClass] will match if you look for [TestClass]. The inherit parameter does not impact this checking. /// /// Attribute to search for. - /// Type info to test. - /// Look through inheritance or not. - /// True if the specified attribute is defined on the type. - public virtual bool IsAttributeDefined(TypeInfo typeInfo, bool inherit) - where TAttribute : Attribute - => IsAttributeDefined((MemberInfo)typeInfo, inherit); - - /// - /// Returns true when specified class/member has attribute derived from specific attribute. - /// - /// The base attribute type. - /// The type. - /// Should look at inheritance tree. - /// An object derived from Attribute that corresponds to the instance of found attribute. - public virtual bool HasAttributeDerivedFrom(Type type, bool inherit) + /// Type to test. + /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. + /// True if the attribute of the specified type is defined on this member or a class. + public virtual bool IsDerivedAttributeDefined(Type type, bool inherit) where TAttribute : Attribute - => HasAttributeDerivedFrom((MemberInfo)type.GetTypeInfo(), inherit); + => IsDerivedAttributeDefined((MemberInfo)type, inherit); /// - /// Returns true when specified class/member has attribute derived from specific attribute. + /// Checks to see if a member or type is decorated with the given attribute, or an attribute that derives from it. e.g. [MyTestClass] from [TestClass] will match if you look for [TestClass]. The inherit parameter does not impact this checking. /// - /// The base attribute type. - /// The type info. - /// Should look at inheritance tree. - /// An object derived from Attribute that corresponds to the instance of found attribute. - public virtual bool HasAttributeDerivedFrom(TypeInfo typeInfo, bool inherit) - where TAttribute : Attribute - => HasAttributeDerivedFrom((MemberInfo)typeInfo, inherit); - - /// - /// Returns true when specified class/member has attribute derived from specific attribute. - /// - /// The base attribute type. - /// The member info. - /// Should look at inheritance tree. - /// An object derived from Attribute that corresponds to the instance of found attribute. - public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inherit) + /// Attribute to search for. + /// Type to test. + /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. + /// True if the attribute of the specified type is defined on this member or a class. + public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool inherit) where TAttribute : Attribute { if (memberInfo == null) @@ -116,21 +106,27 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe } // Get all attributes on the member. - Dictionary? attributes = GetAttributes(memberInfo, inherit); + Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); if (attributes == null) { - PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning($"{nameof(ReflectHelper)}.{nameof(GetAttributes)}: {Resource.FailedFetchAttributeCache}"); + // TODO: + bool a = true; + if (a) + { + throw new NotSupportedException("THIS FALLBACK!"); + } + + PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning($"{nameof(ReflectHelper)}.{nameof(GetCustomAttributesCached)}: {Resource.FailedFetchAttributeCache}"); - return IsAttributeDefined(memberInfo, inherit); + return IsNonDerivedAttributeDefined(memberInfo, inherit); } // Try to find the attribute that is derived from baseAttrType. - foreach (object attribute in attributes.Values) + foreach (Attribute attribute in attributes.Values) { - DebugEx.Assert(attribute != null, $"{nameof(ReflectHelper)}.{nameof(GetAttributes)}: internal error: wrong value in the attributes dictionary."); + DebugEx.Assert(attribute != null, $"{nameof(ReflectHelper)}.{nameof(GetCustomAttributesCached)}: internal error: wrong value in the attributes dictionary."); - Type attributeType = attribute.GetType(); - if (attributeType.GetTypeInfo().IsSubclassOf(typeof(TAttribute))) + if (AttributeComparer.IsDerived(attribute)) { return true; } @@ -153,10 +149,10 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe DebugEx.Assert(methodInfo != null, "MethodInfo should be non-null"); // Get the expected exception attribute - ExpectedExceptionBaseAttribute[]? expectedExceptions; + IEnumerable expectedExceptions; try { - expectedExceptions = GetCustomAttributes(methodInfo, true); + expectedExceptions = GetDerivedAttributes(methodInfo, inherit: true); } catch (Exception ex) { @@ -171,14 +167,15 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe throw new TypeInspectionException(errorMessage); } - if (expectedExceptions == null || expectedExceptions.Length == 0) + // TODO: we can probably do better if we grab the enumerator? + if (!expectedExceptions.Any()) { return null; } // Verify that there is only one attribute (multiple attributes derived from // ExpectedExceptionBaseAttribute are not allowed on a test method) - if (expectedExceptions.Length > 1) + if (expectedExceptions.Count() > 1) { string errorMessage = string.Format( CultureInfo.CurrentCulture, @@ -189,7 +186,7 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe } // Set the expected exception attribute to use for this test - ExpectedExceptionBaseAttribute expectedException = expectedExceptions[0]; + ExpectedExceptionBaseAttribute expectedException = expectedExceptions.First(); return expectedException; } @@ -206,13 +203,100 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe #endif public override object InitializeLifetimeService() => null!; - internal static TAttribute[]? GetAttributes(MethodBase methodBase, bool inherit) + public TAttribute? GetSingleNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) where TAttribute : Attribute { - TAttribute[]? attributeArray = GetCustomAttributes(methodBase, inherit); - return attributeArray == null || attributeArray.Length == 0 + Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + + int count = 0; + TAttribute? foundAttribute = default; + foreach (KeyValuePair cachedAttribute in cachedAttributes) + { + if (AttributeComparer.IsNonDerived(cachedAttribute)) + { + foundAttribute = (TAttribute)cachedAttribute.Value; + count++; + } + } + + if (count == 0) + { + return null; + } + + // We found what we were looking for. + if (count == 1) + { + return foundAttribute; + } + + return nullOnMultiple + ? null + : throw new InvalidOperationException($"Found {count} instances of attribute {typeof(TAttribute)} on class, but only single one was expected."); + } + + public TAttribute? GetFirstNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) + where TAttribute : Attribute + { + Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + + foreach (KeyValuePair cachedAttribute in cachedAttributes) + { + if (AttributeComparer.IsNonDerived(cachedAttribute)) + { + return (TAttribute)cachedAttribute.Value; + } + } + + return null; + } + + public TAttribute? GetSingleDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) + where TAttribute : Attribute + { + Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + + int count = 0; + TAttribute? foundAttribute = default; + foreach (KeyValuePair cachedAttribute in cachedAttributes) + { + if (AttributeComparer.IsDerived(cachedAttribute)) + { + foundAttribute = (TAttribute)cachedAttribute.Value; + count++; + } + } + + if (count == 0) + { + return null; + } + + // We found what we were looking for. + if (count == 1) + { + return foundAttribute; + } + + return nullOnMultiple ? null - : attributeArray; + : throw new InvalidOperationException($"Found {count} instances of attribute {typeof(TAttribute)} on class, but only single one was expected."); + } + + public TAttribute? GetFirstDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) + where TAttribute : Attribute + { + Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + + foreach (KeyValuePair cachedAttribute in cachedAttributes) + { + if (AttributeComparer.IsDerived(cachedAttribute)) + { + return (TAttribute)cachedAttribute.Value; + } + } + + return null; } /// @@ -233,7 +317,7 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe /// If inherited type of attribute. /// All attributes of give type on member. [return: NotNullIfNotNull(nameof(memberInfo))] - internal static TAttribute[]? GetCustomAttributes(MemberInfo? memberInfo, bool inherit) + internal static TAttribute[]? GetCustomAttributesOfTypeNotCached(MemberInfo? memberInfo, bool inherit) where TAttribute : Attribute { if (memberInfo == null) @@ -249,47 +333,6 @@ public bool HasAttributeDerivedFrom(MemberInfo memberInfo, bool inhe return attributesArray!.OfType().ToArray(); // TODO: Investigate if we rely on NRE } - /// - /// Get custom attributes on a member for both normal and reflection only load. - /// - /// Member for which attributes needs to be retrieved. - /// If inherited type of attribute. - /// All attributes of give type on member. - [return: NotNullIfNotNull(nameof(memberInfo))] - internal static object[]? GetCustomAttributes(MemberInfo memberInfo, bool inherit) - { - if (memberInfo == null) - { - return null; - } - - object[] attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes( - memberInfo, - inherit); - - return attributesArray!.ToArray(); // TODO: Investigate if we rely on NRE - } - - /// - /// Returns the first attribute of the specified type or null if no attribute - /// of the specified type is set on the method. - /// - /// The type of attribute to return. - /// The method on which the attribute is defined. - /// The attribute or null if none exists. - internal TAttribute? GetAttribute(MethodInfo method) - where TAttribute : Attribute - { - if (IsAttributeDefined(method, false)) - { - TAttribute[] attributes = GetCustomAttributes(method, false); - DebugEx.Assert(attributes.Length == 1, "Should only be one attribute."); - return attributes[0]; - } - - return null; - } - /// /// Returns true when the method is declared in the assembly where the type is declared. /// @@ -305,20 +348,13 @@ internal virtual bool IsMethodDeclaredInSameAssemblyAsType(MethodInfo method, Ty /// The member to inspect. /// The reflected type that owns . /// Categories defined. - internal virtual string[] GetCategories(MemberInfo categoryAttributeProvider, Type owningType) + internal virtual string[] GetTestCategories(MemberInfo categoryAttributeProvider, Type owningType) { - IEnumerable categories = GetCustomAttributesRecursively(categoryAttributeProvider, owningType); - List testCategories = []; + IEnumerable? methodCategories = GetDerivedAttributes(categoryAttributeProvider, inherit: true); + IEnumerable? typeCategories = GetDerivedAttributes(owningType, inherit: true); + IEnumerable? assemblyCategories = GetDerivedAttributes(owningType.Assembly, inherit: true); - if (categories != null) - { - foreach (TestCategoryBaseAttribute category in categories.Cast()) - { - testCategories.AddRange(category.TestCategories); - } - } - - return testCategories.ToArray(); + return methodCategories.Concat(typeCategories).Concat(assemblyCategories).SelectMany(c => c.TestCategories).ToArray(); } /// @@ -338,8 +374,8 @@ internal virtual string[] GetCategories(MemberInfo categoryAttributeProvider, Ty /// The type that owns . /// True if test method should not run in parallel. internal bool IsDoNotParallelizeSet(MemberInfo testMethod, Type owningType) - => GetCustomAttributes(testMethod).Length != 0 - || GetCustomAttributes(owningType.GetTypeInfo()).Length != 0; + => IsDerivedAttributeDefined(testMethod, inherit: false) + || IsDerivedAttributeDefined(owningType, inherit: false); /// /// Get the parallelization behavior for a test assembly. @@ -360,68 +396,6 @@ internal static bool IsDoNotParallelizeSet(Assembly assembly) !.OfType() // TODO: Investigate if we rely on NRE .FirstOrDefault(); - /// - /// Gets custom attributes at the class and assembly for a method. - /// - /// Method Info or Member Info or a Type. - /// The type that owns . - /// The categories of the specified type on the method. - internal IEnumerable GetCustomAttributesRecursively(MemberInfo attributeProvider, Type owningType) - { - TestCategoryBaseAttribute[]? categories = GetCustomAttributes(attributeProvider); - if (categories != null) - { - categories = categories.Concat(GetCustomAttributes(owningType.GetTypeInfo())).ToArray(); - } - - if (categories != null) - { - categories = categories.Concat(GetCustomAttributeForAssembly(owningType.GetTypeInfo())).ToArray(); - } - - return categories ?? Enumerable.Empty(); - } - - /// - /// Gets the custom attributes on the assembly of a member info - /// NOTE: having it as separate virtual method, so that we can extend it for testing. - /// - /// The attribute type to find. - /// The member to inspect. - /// Custom attributes defined. - internal virtual TAttribute[] GetCustomAttributeForAssembly(MemberInfo memberInfo) - where TAttribute : Attribute - => PlatformServiceProvider.Instance.ReflectionOperations - .GetCustomAttributes(memberInfo.Module.Assembly, typeof(TAttribute)) - !.OfType() - .ToArray(); - - /// - /// Gets the custom attributes of the provided type on a memberInfo. - /// - /// The attribute type. - /// The member to reflect on. - /// Attributes defined. - internal virtual TAttribute[] GetCustomAttributes(MemberInfo attributeProvider) - where TAttribute : Attribute - => GetCustomAttributes(attributeProvider, true); - - /// - /// Gets the first custom attribute of the provided type on a memberInfo. - /// - /// The attribute type. - /// The member to reflect on. - /// Attribute defined. - internal virtual TAttribute? GetCustomAttribute(MemberInfo attributeProvider) - where TAttribute : Attribute - { - TAttribute[] attribute = GetCustomAttributes(attributeProvider, true); - - return attribute == null || attribute.Length != 1 - ? null - : attribute[0]; - } - /// /// KeyValue pairs that are provided by TestOwnerAttribute of the given test method. /// @@ -451,14 +425,8 @@ internal virtual TAttribute[] GetCustomAttributes(MemberInfo attribu /// /// The member to inspect. /// Priority value if defined. Null otherwise. - internal virtual int? GetPriority(MemberInfo priorityAttributeProvider) - { - PriorityAttribute[] priorityAttribute = GetCustomAttributes(priorityAttributeProvider, true); - - return priorityAttribute == null || priorityAttribute.Length != 1 - ? null - : priorityAttribute[0].Priority; - } + internal virtual int? GetPriority(MemberInfo priorityAttributeProvider) => + GetSingleDerivedAttributeOrDefault(priorityAttributeProvider, inherit: true, nullOnMultiple: true)?.Priority; /// /// Priority if any set for test method. Will return priority if attribute is applied to TestMethod @@ -466,14 +434,8 @@ internal virtual TAttribute[] GetCustomAttributes(MemberInfo attribu /// /// The member to inspect. /// Priority value if defined. Null otherwise. - internal virtual string? GetIgnoreMessage(MemberInfo ignoreAttributeProvider) - { - IgnoreAttribute[]? ignoreAttribute = GetCustomAttributes(ignoreAttributeProvider, true); - - return ignoreAttribute is null || ignoreAttribute.Length == 0 - ? null - : ignoreAttribute[0].IgnoreMessage; - } + internal virtual string? GetIgnoreMessage(MemberInfo ignoreAttributeProvider) => + GetSingleDerivedAttributeOrDefault(ignoreAttributeProvider, inherit: true, nullOnMultiple: true)?.IgnoreMessage; /// /// Gets the class cleanup lifecycle for the class, if set. @@ -482,6 +444,7 @@ internal virtual TAttribute[] GetCustomAttributes(MemberInfo attribu /// Returns if provided, otherwise null. internal virtual ClassCleanupBehavior? GetClassCleanupBehavior(TestClassInfo classInfo) { + // TODO: not discovery related but seems expensive and unnecessary, because we do inheritance lookup, and to put the method into the stack we've already did this lookup before? if (!classInfo.HasExecutableCleanupMethod) { return null; @@ -507,7 +470,7 @@ internal virtual TAttribute[] GetCustomAttributes(MemberInfo attribu /// List of traits. internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPropertyProvider) { - TestPropertyAttribute[] testPropertyAttributes = GetCustomAttributes(testPropertyProvider, true); + IEnumerable testPropertyAttributes = GetDerivedAttributes(testPropertyProvider, inherit: true); foreach (TestPropertyAttribute testProperty in testPropertyAttributes) { @@ -528,19 +491,18 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro internal TAttributeType? GetDerivedAttribute(MemberInfo memberInfo, bool inherit) where TAttributeType : Attribute { - Dictionary? attributes = GetAttributes(memberInfo, inherit); + Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); if (attributes == null) { return null; } // Try to find the attribute that is derived from baseAttrType. - foreach (object attribute in attributes.Values) + foreach (Attribute attribute in attributes.Values) { DebugEx.Assert(attribute != null, "ReflectHelper.DefinesAttributeDerivedFrom: internal error: wrong value in the attributes dictionary."); - Type attributeType = attribute.GetType(); - if (attributeType.Equals(typeof(TAttributeType)) || attributeType.GetTypeInfo().IsSubclassOf(typeof(TAttributeType))) + if (AttributeComparer.IsDerived(attribute)) { return attribute as TAttributeType; } @@ -553,31 +515,28 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro /// Get attribute defined on a method which is of given type of subtype of given type. /// /// The attribute type. - /// The type to inspect. + /// The member to inspect. /// Look at inheritance chain. /// An instance of the attribute. - internal static TAttributeType? GetDerivedAttribute(Type type, bool inherit) + internal IEnumerable GetDerivedAttributes(ICustomAttributeProvider attributeProvider, bool inherit) where TAttributeType : Attribute { - object[] attributes = GetCustomAttributes(type.GetTypeInfo(), inherit); + Dictionary? attributes = GetCustomAttributesCached(attributeProvider, inherit); if (attributes == null) { - return null; + yield break; } // Try to find the attribute that is derived from baseAttrType. - foreach (object attribute in attributes) + foreach (Attribute attribute in attributes.Values) { DebugEx.Assert(attribute != null, "ReflectHelper.DefinesAttributeDerivedFrom: internal error: wrong value in the attributes dictionary."); - Type attributeType = attribute.GetType(); - if (attributeType.Equals(typeof(TAttributeType)) || attributeType.GetTypeInfo().IsSubclassOf(typeof(TAttributeType))) + if (AttributeComparer.IsDerived(attribute)) { - return attribute as TAttributeType; + yield return attribute as TAttributeType; } } - - return null; } /// @@ -585,28 +544,43 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro /// /// The member to inspect. /// owner if attribute is applied to TestMethod, else null. - private static string? GetOwner(MemberInfo ownerAttributeProvider) + private string? GetOwner(MemberInfo ownerAttributeProvider) { - OwnerAttribute[] ownerAttribute = GetCustomAttributes(ownerAttributeProvider, true); + OwnerAttribute? ownerAttribute = GetSingleDerivedAttributeOrDefault(ownerAttributeProvider, inherit: true, nullOnMultiple: true); - return ownerAttribute == null || ownerAttribute.Length != 1 - ? null - : ownerAttribute[0].Owner; + return ownerAttribute?.Owner; } + + /// - /// Get the Attributes (TypeName/TypeObject) for a given member. + /// Gets and caches the attributes for the given type, or method. /// - /// The member to inspect. + /// The member to inspect. /// Look at inheritance chain. /// attributes defined. - private Dictionary? GetAttributes(MemberInfo memberInfo, bool inherit) + private Dictionary GetCustomAttributesCached(ICustomAttributeProvider attributeProvider, bool inherit) { + if (inherit) + { + lock (_inheritedAttributeCache) + { + return GetOrAddAttributes(_inheritedAttributeCache, attributeProvider, inherit: true); + } + } + else + { + lock (_nonInheritedAttributeCache) + { + return GetOrAddAttributes(_nonInheritedAttributeCache, attributeProvider, inherit: false); + } + } + // If the information is cached, then use it otherwise populate the cache using // the reflection APIs. - lock (_attributeCache) + Dictionary GetOrAddAttributes(Dictionary> cache, ICustomAttributeProvider attributeProvider, bool inherit) { - if (_attributeCache.TryGetValue(memberInfo, out Dictionary? attributes)) + if (cache.TryGetValue(attributeProvider, out Dictionary? attributes)) { return attributes; } @@ -617,7 +591,10 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro object[]? customAttributesArray = null; try { - customAttributesArray = GetCustomAttributes(memberInfo, inherit); + // This is where we get the data for our cache. It required to use call to Reflection here. +#pragma warning disable RS0030 // Do not use banned APIs + customAttributesArray = NotCachedReflectHelper.GetCustomAttributesNotCached(attributeProvider, inherit); +#pragma warning restore RS0030 // Do not use banned APIs } catch (Exception ex) { @@ -633,11 +610,13 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro description = string.Format(CultureInfo.CurrentCulture, Resource.ExceptionOccuredWhileGettingTheExceptionDescription, ex.GetType().FullName, ex2.GetType().FullName); // ex.GetType().FullName + } - PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning(Resource.FailedToGetCustomAttribute, memberInfo.GetType().FullName!, description); + PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning(Resource.FailedToGetCustomAttribute, attributeProvider.GetType().FullName!, description); // Since we cannot check by attribute names, do it in reflection way. // Note 1: this will not work for different version of assembly but it is better than nothing. // Note 2: we cannot cache this because we don't know if there are other attributes defined. + + // TODO: handle this instead polluting the api with null check, that we don't handle correctly in most places. This path is already expensive and this way we can at least keep it unified. return null; } @@ -646,12 +625,147 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro foreach (object customAttribute in customAttributesArray) { Type attributeType = customAttribute.GetType(); - attributes[attributeType.AssemblyQualifiedName!] = customAttribute; + // TODO: this overwrites any multiple attribute entry. + attributes[attributeType.AssemblyQualifiedName!] = (Attribute)customAttribute; } - _attributeCache.Add(memberInfo, attributes); + cache.Add(attributeProvider, attributes); return attributes; } } + + internal IEnumerable? GetNonDerivedAttributes(MethodInfo methodInfo, bool inherit) + where TAttribute : Attribute + { + Dictionary cachedAttributes = GetCustomAttributesCached(methodInfo, inherit); + + foreach (KeyValuePair cachedAttribute in cachedAttributes) + { + if (AttributeComparer.IsNonDerived(cachedAttribute)) + { + yield return (TAttribute)cachedAttribute.Value; + } + } + } +} + +internal class AttributeComparer +{ + public static bool IsNonDerived(KeyValuePair cachedAttribute) => + typeof(TAttribute).AssemblyQualifiedName == cachedAttribute.Key; + + public static bool IsDerived(KeyValuePair cachedAttribute) => + IsDerived(cachedAttribute.Value); + + public static bool IsDerived(Attribute attribute) + { + Type attributeType = attribute.GetType(); + // IsSubclassOf returns false when the types are equal. + return attributeType == typeof(TAttribute) + || attributeType.IsSubclassOf(typeof(TAttribute)); + } +} + +internal interface INotCachedReflectHelper +{ + object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); + TAttribute[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); + bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit); +} + +/// +/// Reflection helper that is accessing Reflection directly, and won't cache the results. This is used internally by ReflectHelper. +/// Outside of ReflectHelper this should be used only to do the most basic checks on classes, and types, that will determine that a class or a method is NOT +/// part of the discovery or execution. E.g. checking that a class has TestClass attribute. +/// +internal class NotCachedReflectHelper : INotCachedReflectHelper +{ + /// + /// Checks if an attribute of the given type, or and attribute inheriting from that type, is defined on the class or a method. + /// Use this to check single attribute on a class or method to see if it is eligible for being a test. + /// DO NOT use this repeatedly on a type or method that you already know is a test. + /// + /// Type of the attribute to check. + public virtual bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + { + if (attributeProvider is MemberInfo memberInfo) + { + // This is cheaper than getting all the attributes and filtering them. This will return true for + // classes that have [TestClass], and for methods that have [TestMethod]. + if (PlatformServiceProvider.Instance.ReflectionOperations.IsAttributeDefined(memberInfo, typeof(TAttribute), inherit)) + { + return true; + } + } + + // This tries to find an attribute that derives from the given attribute e.g. [TestMethod]. + // This is more expensive than the check above. + foreach (object attribute in GetCustomAttributesNotCached(attributeProvider, inherit)) + { + if (AttributeComparer.IsDerived((Attribute)attribute)) + { + return true; + } + } + + return false; + } + + /// + /// Get custom attributes on a member without cache. + /// + /// Member for which attributes needs to be retrieved. + /// If inherited type of attribute. + /// All attributes of give type on member. + [return: NotNullIfNotNull(nameof(attributeProvider))] + public object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + { + if (attributeProvider == null) + { + return null; + } + + object[] attributesArray; + + if (attributeProvider is MemberInfo memberInfo) + { + attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit); + } + else + { + attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); + } + + return attributesArray; // TODO: Investigate if we rely on NRE + } + + /// + /// Get custom attributes on a member without cache. + /// + /// Member for which attributes needs to be retrieved. + /// If inherited type of attribute. + /// All attributes of give type on member. + [return: NotNullIfNotNull(nameof(attributeProvider))] + public TAttribute[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + { + if (attributeProvider == null) + { + return null; + } + + object[] attributesArray; + + if (attributeProvider is MemberInfo memberInfo) + { + attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit); + } + else + { + attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); + } + + return attributesArray!.OfType().ToArray(); + } + } diff --git a/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj b/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj index 8c03e9fb72..b1e0abc4c0 100644 --- a/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj +++ b/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj @@ -1,4 +1,4 @@ - + @@ -54,6 +54,7 @@ + @@ -96,6 +97,9 @@ + + + diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs index 1da84b921c..0c71a005cb 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs @@ -37,4 +37,6 @@ public interface IReflectionOperations /// The attribute type. /// The list of attributes of the given type on the member. Empty list if none found. object[] GetCustomAttributes(Assembly assembly, Type type); + + public bool IsAttributeDefined(MemberInfo memberInfo, Type type, bool inherit); } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs index 4dfbe0b35f..58df9ebab3 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs @@ -57,4 +57,7 @@ public class ReflectionOperations : IReflectionOperations #else assembly.GetCustomAttributes(type).ToArray(); #endif + + public bool IsAttributeDefined(MemberInfo memberInfo, Type type, bool inherit) => + memberInfo.IsDefined(type, inherit); } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs b/src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs index 3a88898732..2920cb13f2 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. #if !WINDOWS_UWP @@ -19,6 +19,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Uti /// internal class DeploymentItemUtility { + // REVIEW: it would be better if this was a ReflectionHelper, because helper is able to cache. But we don't have reflection helper here, because this is platform services dll. private readonly ReflectionUtility _reflectionUtility; /// diff --git a/src/TestFramework/TestFramework/Attributes/TestMethod/ExpectedExceptionAttribute.cs b/src/TestFramework/TestFramework/Attributes/TestMethod/ExpectedExceptionAttribute.cs index ca877ed410..a61bf7094a 100644 --- a/src/TestFramework/TestFramework/Attributes/TestMethod/ExpectedExceptionAttribute.cs +++ b/src/TestFramework/TestFramework/Attributes/TestMethod/ExpectedExceptionAttribute.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs index dafafa633f..fe4fe9cac4 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs @@ -39,7 +39,7 @@ public TestMethodValidatorTests() public void IsValidTestMethodShouldReturnFalseForMethodsWithoutATestMethodAttributeOrItsDerivedAttributes() { _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(It.IsAny(), false)).Returns(false); + rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(false); Verify(!_testMethodValidator.IsValidTestMethod(_mockMethodInfo.Object, _type, _warnings)); } @@ -183,7 +183,7 @@ public void WhenDiscoveryOfInternalsIsEnabledIsValidTestMethodShouldReturnFalseF #endregion private void SetupTestMethod() => _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(It.IsAny(), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(true); } #region Dummy types diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs index c8761125ba..92fe44cebe 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -277,9 +277,9 @@ public void GetTestFromMethodShouldSetIgnoredPropertyToFalseIfNotSetOnTestClassA // Setup mocks _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(typeof(DummyTestClass), false)).Returns(false); + rh => rh.IsNonDerivedAttributeDefined(typeof(DummyTestClass), false)).Returns(false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(methodInfo, false)).Returns(false); + rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)).Returns(false); MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); @@ -295,7 +295,7 @@ public void GetTestFromMethodShouldSetTestCategory() string[] testCategories = new string[] { "foo", "bar" }; // Setup mocks - _mockReflectHelper.Setup(rh => rh.GetCategories(methodInfo, typeof(DummyTestClass))).Returns(testCategories); + _mockReflectHelper.Setup(rh => rh.GetTestCategories(methodInfo, typeof(DummyTestClass))).Returns(testCategories); MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); @@ -303,20 +303,20 @@ public void GetTestFromMethodShouldSetTestCategory() Verify(testCategories.SequenceEqual(testElement.TestCategory)); } - public void GetTestFromMethodShouldSetDoNotParallelize() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + //public void GetTestFromMethodShouldSetDoNotParallelize() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // Setup mocks - _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(It.IsAny())).Returns([new DoNotParallelizeAttribute()]); + // // Setup mocks + // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(It.IsAny())).Returns([new DoNotParallelizeAttribute()]); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement is not null); - Verify(testElement.DoNotParallelize); - } + // Verify(testElement is not null); + // Verify(testElement.DoNotParallelize); + //} public void GetTestFromMethodShouldFillTraitsWithTestProperties() { @@ -388,65 +388,65 @@ public void GetTestFromMethodShouldSetPriority() Verify(testElement.Priority == 1); } - public void GetTestFromMethodShouldSetDescription() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new DescriptionAttribute("Dummy description")); + //public void GetTestFromMethodShouldSetDescription() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new DescriptionAttribute("Dummy description")); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement.Description == "Dummy description"); - } + // Verify(testElement.Description == "Dummy description"); + //} - public void GetTestFromMethodShouldSetWorkItemIds() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns([new(123), new(345)]); + //public void GetTestFromMethodShouldSetWorkItemIds() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns([new(123), new(345)]); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(new string[] { "123", "345" }.SequenceEqual(testElement.WorkItemIds)); - } + // Verify(new string[] { "123", "345" }.SequenceEqual(testElement.WorkItemIds)); + //} - public void GetTestFromMethodShouldSetWorkItemIdsToNullIfNotAny() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns(Array.Empty()); + //public void GetTestFromMethodShouldSetWorkItemIdsToNullIfNotAny() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns(Array.Empty()); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement.WorkItemIds is null); - } + // Verify(testElement.WorkItemIds is null); + //} - public void GetTestFromMethodShouldSetCssIteration() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssIterationAttribute("234")); + //public void GetTestFromMethodShouldSetCssIteration() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssIterationAttribute("234")); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement.CssIteration == "234"); - } + // Verify(testElement.CssIteration == "234"); + //} - public void GetTestFromMethodShouldSetCssProjectStructure() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssProjectStructureAttribute("ProjectStructure123")); + //public void GetTestFromMethodShouldSetCssProjectStructure() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssProjectStructureAttribute("ProjectStructure123")); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement.CssProjectStructure == "ProjectStructure123"); - } + // Verify(testElement.CssProjectStructure == "ProjectStructure123"); + //} public void GetTestFromMethodShouldSetDeploymentItemsToNullIfNotPresent() { @@ -501,53 +501,53 @@ public void GetTestFromMethodShouldSetDeclaringAssemblyName() Verify(otherAssemblyName == testElement.TestMethod.DeclaringAssemblyName); } - public void GetTestFromMethodShouldSetDisplayNameToTestMethodNameIfDisplayNameIsNotPresent() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + //public void GetTestFromMethodShouldSetDisplayNameToTestMethodNameIfDisplayNameIsNotPresent() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // Setup mocks to behave like we have [TestMethod] attribute on the method - _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(It.IsAny())).Returns(new TestMethodAttribute()); + // // Setup mocks to behave like we have [TestMethod] attribute on the method + // _mockReflectHelper.Setup( + // rh => rh.GetCustomAttribute(It.IsAny())).Returns(new TestMethodAttribute()); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement is not null); - Verify(testElement.DisplayName == "MethodWithVoidReturnType"); - } + // Verify(testElement is not null); + // Verify(testElement.DisplayName == "MethodWithVoidReturnType"); + //} - public void GetTestFromMethodShouldSetDisplayNameFromTestMethodAttribute() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + //public void GetTestFromMethodShouldSetDisplayNameFromTestMethodAttribute() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // Setup mocks to behave like we have [TestMethod("Test method display name.")] attribute on the method - _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(methodInfo)).Returns(new TestMethodAttribute("Test method display name.")); + // // Setup mocks to behave like we have [TestMethod("Test method display name.")] attribute on the method + // _mockReflectHelper.Setup( + // rh => rh.GetCustomAttribute(methodInfo)).Returns(new TestMethodAttribute("Test method display name.")); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement is not null); - Verify(testElement.DisplayName == "Test method display name."); - } + // Verify(testElement is not null); + // Verify(testElement.DisplayName == "Test method display name."); + //} - public void GetTestFromMethodShouldSetDisplayNameFromDataTestMethodAttribute() - { - SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + //public void GetTestFromMethodShouldSetDisplayNameFromDataTestMethodAttribute() + //{ + // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // Setup mocks to behave like we have [DataTestMethod("Test method display name.")] attribute on the method - _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(methodInfo)).Returns(new DataTestMethodAttribute("Test method display name.")); + // // Setup mocks to behave like we have [DataTestMethod("Test method display name.")] attribute on the method + // _mockReflectHelper.Setup( + // rh => rh.GetCustomAttribute(methodInfo)).Returns(new DataTestMethodAttribute("Test method display name.")); - MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - Verify(testElement is not null); - Verify(testElement.DisplayName == "Test method display name."); - } + // Verify(testElement is not null); + // Verify(testElement.DisplayName == "Test method display name."); + //} #endregion diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs index 8b32e05d13..baeca09614 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs @@ -44,15 +44,15 @@ public TypeValidatorTests() public void IsValidTestClassShouldReturnFalseForClassesNotHavingTestClassAttributeOrDerivedAttributeTypes() { - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(It.IsAny(), false)).Returns(false); + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(false); Verify(!_typeValidator.IsValidTestClass(typeof(TypeValidatorTests), _warnings)); } public void IsValidTestClassShouldReturnTrueForClassesMarkedByAnAttributeDerivedFromTestClass() { - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(It.IsAny(), false)).Returns(false); + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(false); _mockReflectHelper.Setup( - rh => rh.HasAttributeDerivedFrom(It.IsAny(), false)).Returns(true); + rh => rh.IsDerivedAttributeDefined(It.IsAny(), false)).Returns(true); Verify(_typeValidator.IsValidTestClass(typeof(TypeValidatorTests), _warnings)); } @@ -403,7 +403,7 @@ private static Type[] GetAllTestTypes() #region private methods - private void SetupTestClass() => _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(It.IsAny(), false)).Returns(true); + private void SetupTestClass() => _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(true); #endregion } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs index b143c20a61..bb7e77157a 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs @@ -177,7 +177,7 @@ public void GetTestMethodInfoShouldSetTestContextIfPresent() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, @@ -195,7 +195,7 @@ public void GetTestMethodInfoShouldSetTestContextToNullIfNotPresent() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, @@ -215,7 +215,7 @@ public void GetTestMethodInfoShouldAddAssemblyInfoToTheCache() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -232,10 +232,10 @@ public void GetTestMethodInfoShouldNotThrowIfWeFailToDiscoverTypeFromAnAssembly( var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(It.IsAny(), true)).Throws(new Exception()); + rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), true)).Throws(new Exception()); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(typeof(DummyTestClassWithTestMethods), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(typeof(DummyTestClassWithTestMethods), true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -251,10 +251,10 @@ public void GetTestMethodInfoShouldCacheAssemblyInitializeAttribute() var testMethod = new TestMethod("TestInit", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -271,10 +271,10 @@ public void GetTestMethodInfoShouldCacheAssemblyCleanupAttribute() var testMethod = new TestMethod("TestCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -291,12 +291,12 @@ public void GetTestMethodInfoShouldCacheAssemblyInitAndCleanupAttribute() var testMethod = new TestMethod("TestInitOrCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -314,10 +314,10 @@ public void GetTestMethodInfoShouldThrowIfAssemblyInitHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -346,10 +346,10 @@ public void GetTestMethodInfoShouldThrowIfAssemblyCleanupHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -379,7 +379,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInfoInstanceAndReuseTheCache() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -391,7 +391,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInfoInstanceAndReuseTheCache() new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), false); - _mockReflectHelper.Verify(rh => rh.IsAttributeDefined(type.GetTypeInfo(), true), Times.Once); + _mockReflectHelper.Verify(rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true), Times.Once); Verify(_typeCache.AssemblyInfoCache.Count == 1); } @@ -406,7 +406,7 @@ public void GetTestMethodInfoShouldAddClassInfoToTheCache() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -424,10 +424,10 @@ public void GetTestMethodInfoShouldCacheClassInitializeAttribute() var testMethod = new TestMethod("TestInit", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -447,16 +447,16 @@ public void GetTestMethodInfoShouldCacheBaseClassInitializeAttributes() var testMethod = new TestMethod("TestMethod", type.FullName, "A", false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseType.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("AssemblyInit"), false)).Returns(true); _mockReflectHelper.Setup( rh => rh.GetCustomAttribute(baseType.GetMethod("AssemblyInit"))) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("ClassInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("ClassInit"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -475,10 +475,10 @@ public void GetTestMethodInfoShouldCacheClassCleanupAttribute() var testMethod = new TestMethod("TestCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -497,9 +497,9 @@ public void GetTestMethodInfoShouldCacheBaseClassCleanupAttributes() var testMethod = new TestMethod("TestMethod", type.FullName, "A", false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseType.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("AssemblyCleanup"), false)).Returns(true); _mockReflectHelper.Setup( rh => rh.GetCustomAttribute(baseType.GetMethod("AssemblyCleanup"))) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); @@ -521,11 +521,11 @@ public void GetTestMethodInfoShouldCacheClassInitAndCleanupAttribute() var testMethod = new TestMethod("TestInitOrCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -547,23 +547,23 @@ public void GetTestMethodInfoShouldCacheBaseClassInitAndCleanupAttributes() MethodInfo baseCleanupMethod = baseType.GetMethod("ClassCleanup"); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseInitializeMethod, false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseInitializeMethod, false)).Returns(true); _mockReflectHelper.Setup( rh => rh.GetCustomAttribute(baseInitializeMethod)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseCleanupMethod, false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseCleanupMethod, false)).Returns(true); _mockReflectHelper.Setup( rh => rh.GetCustomAttribute(baseCleanupMethod)) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -591,18 +591,18 @@ public void GetTestMethodInfoShouldCacheParentAndGrandparentClassInitAndCleanupA MethodInfo parentCleanupMethod = parentType.GetMethod("ChildClassCleanup"); _mockReflectHelper - .Setup(rh => rh.IsAttributeDefined(type, true)) + .Setup(rh => rh.IsNonDerivedAttributeDefined(type, true)) .Returns(true); // Setup grandparent class init/cleanup methods _mockReflectHelper - .Setup(rh => rh.IsAttributeDefined(grandparentInitMethod, false)) + .Setup(rh => rh.IsNonDerivedAttributeDefined(grandparentInitMethod, false)) .Returns(true); _mockReflectHelper .Setup(rh => rh.GetCustomAttribute(grandparentInitMethod)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper - .Setup(rh => rh.IsAttributeDefined(grandparentCleanupMethod, false)) + .Setup(rh => rh.IsNonDerivedAttributeDefined(grandparentCleanupMethod, false)) .Returns(true); _mockReflectHelper .Setup(rh => rh.GetCustomAttribute(grandparentCleanupMethod)) @@ -610,13 +610,13 @@ public void GetTestMethodInfoShouldCacheParentAndGrandparentClassInitAndCleanupA // Setup parent class init/cleanup methods _mockReflectHelper - .Setup(rh => rh.IsAttributeDefined(parentInitMethod, false)) + .Setup(rh => rh.IsNonDerivedAttributeDefined(parentInitMethod, false)) .Returns(true); _mockReflectHelper .Setup(rh => rh.GetCustomAttribute(parentInitMethod)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper - .Setup(rh => rh.IsAttributeDefined(parentCleanupMethod, false)) + .Setup(rh => rh.IsNonDerivedAttributeDefined(parentCleanupMethod, false)) .Returns(true); _mockReflectHelper .Setup(rh => rh.GetCustomAttribute(parentCleanupMethod)) @@ -650,10 +650,10 @@ public void GetTestMethodInfoShouldThrowIfClassInitHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -682,10 +682,10 @@ public void GetTestMethodInfoShouldThrowIfClassCleanupHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -714,10 +714,10 @@ public void GetTestMethodInfoShouldCacheTestInitializeAttribute() var testMethod = new TestMethod("TestInit", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("TestInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("TestInit"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -734,10 +734,10 @@ public void GetTestMethodInfoShouldCacheTestCleanupAttribute() var testMethod = new TestMethod("TestCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("TestCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("TestCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -754,10 +754,10 @@ public void GetTestMethodInfoShouldThrowIfTestInitOrCleanupHasIncorrectSignature var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("TestInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("TestInit"), false)).Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -788,10 +788,10 @@ public void GetTestMethodInfoShouldCacheTestInitializeAttributeDefinedInBaseClas var testMethod = new TestMethod("TestMethod", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseType.GetMethod("TestInit"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("TestInit"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -809,10 +809,10 @@ public void GetTestMethodInfoShouldCacheTestCleanupAttributeDefinedInBaseClass() var testMethod = new TestMethod("TestMethod", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(baseType.GetMethod("TestCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("TestCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -830,7 +830,7 @@ public void GetTestMethodInfoShouldCacheClassInfoInstanceAndReuseFromCache() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -899,7 +899,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoWithTimeout() MethodInfo methodInfo = type.GetMethod("TestMethodWithTimeout"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( @@ -919,7 +919,7 @@ public void GetTestMethodInfoShouldThrowWhenTimeoutIsIncorrect() MethodInfo methodInfo = type.GetMethod("TestMethodWithIncorrectTimeout"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); void A() => _typeCache.GetTestMethodInfo( @@ -980,7 +980,7 @@ public void GetTestMethodInfoWhenTimeoutAttributeSetShouldReturnTimeoutBasedOnAt MethodInfo methodInfo = type.GetMethod("TestMethodWithTimeout"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( @@ -1209,10 +1209,10 @@ public void ClassInfoListWithExecutableCleanupMethodsShouldReturnEmptyListWhenCl var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("TestCleanup"), false)).Returns(false); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("TestCleanup"), false)).Returns(false); _typeCache.GetTestMethodInfo( testMethod, @@ -1231,10 +1231,10 @@ public void ClassInfoListWithExecutableCleanupMethodsShouldReturnClassInfosWithE var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -1265,10 +1265,10 @@ public void AssemblyInfoListWithExecutableCleanupMethodsShouldReturnEmptyListWhe var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type, true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(false); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(false); _typeCache.GetTestMethodInfo( testMethod, @@ -1287,10 +1287,10 @@ public void AssemblyInfoListWithExecutableCleanupMethodsShouldReturnAssemblyInfo var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -1314,7 +1314,7 @@ public void ResolveExpectedExceptionHelperShouldReturnExpectedExceptionAttribute var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); UTF.ExpectedExceptionAttribute expectedException = new(typeof(DivideByZeroException)); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); _mockReflectHelper.Setup(rh => rh.ResolveExpectedExceptionHelper(methodInfo, testMethod)).Returns(expectedException); @@ -1332,7 +1332,7 @@ public void ResolveExpectedExceptionHelperShouldReturnNullIfExpectedExceptionAtt MethodInfo methodInfo = type.GetMethod("TestMethod"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( @@ -1351,7 +1351,7 @@ public void ResolveExpectedExceptionHelperShouldThrowIfMultipleExpectedException MethodInfo methodInfo = type.GetMethod("TestMethodWithMultipleExpectedException"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); - _mockReflectHelper.Setup(rh => rh.IsAttributeDefined(methodInfo, false)) + _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); try diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/UnitTestRunnerTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/UnitTestRunnerTests.cs index 0ed6117ec1..b02c849bde 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/UnitTestRunnerTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/UnitTestRunnerTests.cs @@ -304,7 +304,7 @@ public void RunSingleTestShouldCallAssemblyInitializeAndClassInitializeMethodsIn _testablePlatformServiceProvider.MockFileOperations.Setup(fo => fo.LoadAssembly("A", It.IsAny())) .Returns(Assembly.GetExecutingAssembly()); mockReflectHelper.Setup( - rh => rh.IsAttributeDefined(type.GetMethod("AssemblyInitialize"), It.IsAny())) + rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInitialize"), It.IsAny())) .Returns(true); int validator = 1; diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs index d39dd6d883..1935004c12 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -49,7 +49,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtClassLevel() _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel") }, MemberTypes.TypeInfo); string[] expected = ["ClassLevel"]; - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); Verify(expected.SequenceEqual(actual)); } @@ -64,7 +64,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtAllLevels() _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel") }, MemberTypes.TypeInfo); _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel") }, MemberTypes.Method); - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); string[] expected = ["MethodLevel", "ClassLevel", "AsmLevel1", "AsmLevel2", "AsmLevel3"]; Verify(expected.SequenceEqual(actual)); @@ -82,7 +82,7 @@ public void GetTestCategoryAttributeShouldConcatCustomAttributeOfSameType() _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel1") }, MemberTypes.Method); _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel2") }, MemberTypes.Method); - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); string[] expected = ["MethodLevel1", "MethodLevel2", "ClassLevel1", "ClassLevel2", "AsmLevel1", "AsmLevel2"]; Verify(expected.SequenceEqual(actual)); @@ -97,7 +97,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtAssemblyLevel() string[] expected = ["AsmLevel"]; - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); Verify(expected.SequenceEqual(actual)); } @@ -110,7 +110,7 @@ public void GetTestCategoryAttributeShouldIncludeMultipleTestCategoriesAtClassLe _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("ClassLevel"), new UTF.TestCategoryAttribute("ClassLevel1") }, MemberTypes.TypeInfo); string[] expected = ["ClassLevel", "ClassLevel1"]; - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); Verify(expected.SequenceEqual(actual)); } @@ -123,7 +123,7 @@ public void GetTestCategoryAttributeShouldIncludeMultipleTestCategoriesAtAssembl _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("AsmLevel"), new UTF.TestCategoryAttribute("AsmLevel1") }, MemberTypes.All); string[] expected = ["AsmLevel", "AsmLevel1"]; - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); Verify(expected.SequenceEqual(actual)); } @@ -135,14 +135,14 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtMethodLevel() _reflectHelper.SetCustomAttribute(typeof(UTF.TestCategoryBaseAttribute), new[] { new UTF.TestCategoryAttribute("MethodLevel") }, MemberTypes.Method); string[] expected = ["MethodLevel"]; - string[] actual = _reflectHelper.GetCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); + string[] actual = _reflectHelper.GetTestCategories(_method.Object, typeof(ReflectHelperTests)).ToArray(); Verify(expected.SequenceEqual(actual)); } public void IsAttributeDefinedShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMember() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new UTF.TestMethodAttribute() }; @@ -150,12 +150,12 @@ public void IsAttributeDefinedShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMe Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). Returns(attributes); - Verify(rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void IsAttributeDefinedShouldReturnFalseIfSpecifiedAttributeIsNotDefinedOnAMember() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new UTF.TestClassAttribute() }; @@ -163,12 +163,12 @@ public void IsAttributeDefinedShouldReturnFalseIfSpecifiedAttributeIsNotDefinedO Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). Returns(attributes); - Verify(!rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(!rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void IsAttributeDefinedShouldReturnFromCache() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); // Not using mocks here because for some reason a dictionary match of the mock is not returning true in the product code. MethodInfo memberInfo = typeof(ReflectHelperTests).GetMethod("IsAttributeDefinedShouldReturnFromCache"); @@ -180,10 +180,10 @@ public void IsAttributeDefinedShouldReturnFromCache() Setup(ro => ro.GetCustomAttributes(memberInfo, true)). Returns(attributes); - Verify(rh.IsAttributeDefined(memberInfo, true)); + Verify(rh.IsNonDerivedAttributeDefined(memberInfo, true)); // Validate that reflection APIs are not called again. - Verify(rh.IsAttributeDefined(memberInfo, true)); + Verify(rh.IsNonDerivedAttributeDefined(memberInfo, true)); _testablePlatformServiceProvider.MockReflectionOperations.Verify(ro => ro.GetCustomAttributes(memberInfo, true), Times.Once); // Also validate that reflection APIs for an individual type is not called since the cache gives us what we need already. @@ -192,7 +192,7 @@ public void IsAttributeDefinedShouldReturnFromCache() public void IsAttributeDefinedShouldReturnTrueQueryingASpecificAttributesExistenceEvenIfGettingAllAttributesFail() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new UTF.TestMethodAttribute() }; @@ -204,12 +204,12 @@ public void IsAttributeDefinedShouldReturnTrueQueryingASpecificAttributesExisten Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, typeof(UTF.TestMethodAttribute), true)). Returns(attributes); - Verify(rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void HasAttributeDerivedFromShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMember() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -217,12 +217,12 @@ public void HasAttributeDerivedFromShouldReturnTrueIfSpecifiedAttributeIsDefined Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). Returns(attributes); - Verify(rh.HasAttributeDerivedFrom(mockMemberInfo.Object, true)); + Verify(rh.IsDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void HasAttributeDerivedFromShouldReturnFalseIfSpecifiedAttributeIsNotDefinedOnAMember() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -230,12 +230,12 @@ public void HasAttributeDerivedFromShouldReturnFalseIfSpecifiedAttributeIsNotDef Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). Returns(attributes); - Verify(!rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(!rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void HasAttributeDerivedFromShouldReturnFromCache() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); // Not using mocks here because for some reason a dictionary match of the mock is not returning true in the product code. MethodInfo memberInfo = typeof(ReflectHelperTests).GetMethod("HasAttributeDerivedFromShouldReturnFromCache"); @@ -247,10 +247,10 @@ public void HasAttributeDerivedFromShouldReturnFromCache() Setup(ro => ro.GetCustomAttributes(memberInfo, true)). Returns(attributes); - Verify(rh.HasAttributeDerivedFrom(memberInfo, true)); + Verify(rh.IsDerivedAttributeDefined(memberInfo, true)); // Validate that reflection APIs are not called again. - Verify(rh.HasAttributeDerivedFrom(memberInfo, true)); + Verify(rh.IsDerivedAttributeDefined(memberInfo, true)); _testablePlatformServiceProvider.MockReflectionOperations.Verify(ro => ro.GetCustomAttributes(memberInfo, true), Times.Once); // Also validate that reflection APIs for an individual type is not called since the cache gives us what we need already. @@ -259,7 +259,7 @@ public void HasAttributeDerivedFromShouldReturnFromCache() public void HasAttributeDerivedFromShouldReturnFalseQueryingProvidedAttributesExistenceIfGettingAllAttributesFail() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -271,12 +271,12 @@ public void HasAttributeDerivedFromShouldReturnFalseQueryingProvidedAttributesEx Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, typeof(UTF.TestMethodAttribute), true)). Returns(attributes); - Verify(!rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(!rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } public void HasAttributeDerivedFromShouldReturnTrueQueryingProvidedAttributesExistenceIfGettingAllAttributesFail() { - var rh = new ReflectHelper(); + var rh = new ReflectHelper(new NotCachedReflectHelper()); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -288,7 +288,7 @@ public void HasAttributeDerivedFromShouldReturnTrueQueryingProvidedAttributesExi Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, typeof(TestableExtendedTestMethod), true)). Returns(attributes); - Verify(rh.IsAttributeDefined(mockMemberInfo.Object, true)); + Verify(rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs index 42811bd3de..30d9b8d174 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -22,33 +22,34 @@ internal sealed class TestableReflectHelper : ReflectHelper private readonly Dictionary _customAttributes; public TestableReflectHelper() + : base(new TestableReflectionAccessor()) { _customAttributes = []; } public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes memberTypes) { + // tODO: Add the info to ours; + var ours = (TestableReflectionAccessor)this.NotCachedReflectHelper; + + int hashCode = type.FullName.GetHashCode() + memberTypes.GetHashCode(); _customAttributes[hashCode] = _customAttributes.TryGetValue(hashCode, out Attribute[] value) ? value.Concat(values).ToArray() : values; } +} - internal override TAttribute[] GetCustomAttributeForAssembly(MemberInfo memberInfo) +internal class TestableReflectionAccessor : INotCachedReflectHelper +{ + public TestableReflectionAccessor() { - int hashCode = MemberTypes.All.GetHashCode() + typeof(TAttribute).FullName.GetHashCode(); - - return _customAttributes.TryGetValue(hashCode, out Attribute[] value) - ? value.OfType().ToArray() - : Array.Empty(); } - internal override TAttribute[] GetCustomAttributes(MemberInfo memberInfo) - { - int hashCode = memberInfo.MemberType.GetHashCode() + typeof(TAttribute).FullName.GetHashCode(); + // TODO: fix to fix tests. + public object[] GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); - return _customAttributes.TryGetValue(hashCode, out Attribute[] value) - ? value.OfType().ToArray() - : Array.Empty(); - } + public TAttribute[] GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); + + public bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); } From 0c8f86f15034abf3f94788bdc240172d6d683dd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 9 May 2024 18:36:08 +0200 Subject: [PATCH 03/20] Skip over test method info when not needed --- .../Discovery/AssemblyEnumerator.cs | 7 ++++++- .../Discovery/TypeEnumerator.cs | 16 ++++++++++++++++ .../Discovery/UnitTestDiscoverer.cs | 2 +- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index a53329eeee..ac37c583bf 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -263,6 +263,11 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet return false; } + if (test.TestMethod.DataType != DynamicDataType.ITestDataSource) + { + return false; + } + // NOTE: From this place we don't have any path that would let the user write a message on the TestContext and we don't do // anything with what would be printed anyway so we can simply use a simple StringWriter. using var writer = new StringWriter(); @@ -277,7 +282,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth MethodInfo methodInfo = testMethodInfo.MethodInfo; // TODO: this needs a special method where we grab all attributes and compare them to a type to see if the attribute inherits from additional interface. // We want to do first or default here, to see if we should go to the expensive path. - IEnumerable? testDataSources = ReflectHelper.Instance.GetNonDerivedAttributes(methodInfo, inherit: false)?.OfType(); + IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false)?.OfType(); if (testDataSources == null || !testDataSources.Any()) { return false; diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 4b8b0ea35c..6c04c60af3 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -147,6 +147,22 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT method.DeclaringType.GetTypeInfo().Assembly); } + // TODO: this needs a special method where we grab all attributes and compare them to a type to see if the attribute inherits from additional interface. + // We want to do first or default here, to see if we should go to the expensive path. + bool isDataDriven = false; + foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes(method, inherit: true)) + { + if (AttributeComparer.IsDerived(attribute)) + { + isDataDriven = true; + break; + } + } + + testMethod.DataType = isDataDriven ? DynamicDataType.ITestDataSource : DynamicDataType.None; + + // TODO end + var testElement = new UnitTestElement(testMethod) { // Get compiler generated type name for async test method (either void returning or task returning). diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs index e325d3846d..8308f203f8 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs @@ -102,7 +102,7 @@ internal UnitTestDiscoverer() #endif Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds} <<<"); #if NET6_0_OR_GREATER - Console.WriteLine($"discovered: discovery alloc: {(afterDiscovery - beforeDiscovery) / 1024 * 1024} MB send alloc: {(afterSend- afterDiscovery) / 1024 * 1024} MB"); + Console.WriteLine($"discovered: discovery alloc: {(afterDiscovery - beforeDiscovery) / (1024 * 1024)} MB send alloc: {(afterSend - afterDiscovery) / (1024 * 1024)} MB"); #endif } From 93bdf38f7705154cc8df17aaacdd3858bc9f14cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 9 May 2024 21:58:14 +0200 Subject: [PATCH 04/20] cache all --- .../Discovery/TestMethodValidator.cs | 2 +- .../Discovery/TypeValidator.cs | 2 +- .../Execution/KnownNonTestMethods.cs | 9 ++ .../MSTest.TestAdapter/Execution/TypeCache.cs | 26 ++++- .../Helpers/ReflectHelper.cs | 95 ++++++++----------- 5 files changed, 74 insertions(+), 60 deletions(-) create mode 100644 src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs index 10be10ad99..5d850697b2 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs @@ -50,7 +50,7 @@ internal virtual bool IsValidTestMethod(MethodInfo testMethodInfo, Type type, IC { // Use non-caching reflection helper to check if a method is a valid test method. We don't want to retrieve // all attributes for every single method, and we also don't want to cache them for methods that we won't look at again. - if (!_reflectHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(testMethodInfo, inherit: false)) + if (!_reflectHelper.IsDerivedAttributeDefined(testMethodInfo, inherit: false)) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs index 04e5d60332..bfa6b97382 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs @@ -51,7 +51,7 @@ internal virtual bool IsValidTestClass(Type type, ICollection warnings) { TypeInfo typeInfo = type.GetTypeInfo(); - if (!typeInfo.IsClass || !_reflectHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(typeInfo, false)) + if (!typeInfo.IsClass || !_reflectHelper.IsDerivedAttributeDefined(typeInfo, inherit: false)) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs b/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs new file mode 100644 index 0000000000..2942c345b5 --- /dev/null +++ b/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs @@ -0,0 +1,9 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; + +internal static class KnownNonTestMethods +{ + public static HashSet Methods { get; } = [nameof(object.Equals), nameof(object.GetHashCode), nameof(object.GetType), nameof(object.ReferenceEquals), nameof(object.ToString)]; +} diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index a998477920..0359106887 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -306,11 +306,22 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) foreach (MethodInfo methodInfo in classType.GetTypeInfo().DeclaredMethods) { - // Update test initialize/cleanup method - UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, false, instanceMethods); + if (KnownNonTestMethods.Methods.Contains(methodInfo.Name)) + { + continue; + } + + if (methodInfo.IsPublic && !methodInfo.IsStatic) + { + // Update test initialize/cleanup method + UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, false, instanceMethods); + } - // Update class initialize/cleanup method - UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); + if (methodInfo.IsPublic && methodInfo.IsStatic) + { + // Update class initialize/cleanup method + UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); + } } Type? baseType = classType.GetTypeInfo().BaseType; @@ -318,6 +329,11 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) { foreach (MethodInfo methodInfo in baseType.GetTypeInfo().DeclaredMethods) { + if (KnownNonTestMethods.Methods.Contains(methodInfo.Name)) + { + continue; + } + if (methodInfo.IsPublic && !methodInfo.IsStatic) { // Update test initialize/cleanup method from base type. @@ -402,7 +418,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) { // Only examine classes which are TestClass or derives from TestClass attribute TypeInfo typeInfo = t.GetTypeInfo(); - if (!_reflectionHelper.NotCachedReflectHelper.IsDerivedAttributeDefinedNotCached(typeInfo, inherit: true)) + if (!_reflectionHelper.IsDerivedAttributeDefined(typeInfo, inherit: true)) { continue; } diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index adb0a2d3ca..8e90e8a400 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -21,8 +21,8 @@ internal class ReflectHelper : MarshalByRefObject #pragma warning restore RS0030 // Do not use banned APIs // Caches below could be unified by using ICustomAttributeProvider. But the underlying IReflectionOperations is public and we would have to change or duplicate it. - private readonly Dictionary> _inheritedAttributeCache = []; - private readonly Dictionary> _nonInheritedAttributeCache = []; + private readonly Dictionary _inheritedAttributeCache = []; + private readonly Dictionary _nonInheritedAttributeCache = []; internal /* for tests only */ ReflectHelper(INotCachedReflectHelper notCachedReflectHelper) => NotCachedReflectHelper = notCachedReflectHelper; @@ -49,7 +49,7 @@ public virtual bool IsNonDerivedAttributeDefined(MemberInfo memberIn } // Get attributes defined on the member from the cache. - Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); + Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); string requiredAttributeQualifiedName = typeof(TAttribute).AssemblyQualifiedName!; if (attributes == null) { @@ -65,7 +65,15 @@ public virtual bool IsNonDerivedAttributeDefined(MemberInfo memberIn return specificAttributes.Any(a => string.Equals(a.GetType().AssemblyQualifiedName, requiredAttributeQualifiedName, StringComparison.Ordinal)); } - return attributes.ContainsKey(requiredAttributeQualifiedName); + foreach (Attribute attribute in attributes) + { + if (AttributeComparer.IsNonDerived(attribute)) + { + return true; + } + } + + return false; } /// @@ -106,7 +114,7 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in } // Get all attributes on the member. - Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); + Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); if (attributes == null) { // TODO: @@ -122,7 +130,7 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in } // Try to find the attribute that is derived from baseAttrType. - foreach (Attribute attribute in attributes.Values) + foreach (Attribute attribute in attributes) { DebugEx.Assert(attribute != null, $"{nameof(ReflectHelper)}.{nameof(GetCustomAttributesCached)}: internal error: wrong value in the attributes dictionary."); @@ -206,15 +214,15 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in public TAttribute? GetSingleNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) where TAttribute : Attribute { - Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); int count = 0; TAttribute? foundAttribute = default; - foreach (KeyValuePair cachedAttribute in cachedAttributes) + foreach (Attribute cachedAttribute in cachedAttributes) { if (AttributeComparer.IsNonDerived(cachedAttribute)) { - foundAttribute = (TAttribute)cachedAttribute.Value; + foundAttribute = (TAttribute)cachedAttribute; count++; } } @@ -238,13 +246,13 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in public TAttribute? GetFirstNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) where TAttribute : Attribute { - Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); - foreach (KeyValuePair cachedAttribute in cachedAttributes) + foreach (Attribute cachedAttribute in cachedAttributes) { if (AttributeComparer.IsNonDerived(cachedAttribute)) { - return (TAttribute)cachedAttribute.Value; + return (TAttribute)cachedAttribute; } } @@ -254,15 +262,15 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in public TAttribute? GetSingleDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) where TAttribute : Attribute { - Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); int count = 0; TAttribute? foundAttribute = default; - foreach (KeyValuePair cachedAttribute in cachedAttributes) + foreach (Attribute cachedAttribute in cachedAttributes) { if (AttributeComparer.IsDerived(cachedAttribute)) { - foundAttribute = (TAttribute)cachedAttribute.Value; + foundAttribute = (TAttribute)cachedAttribute; count++; } } @@ -286,13 +294,13 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in public TAttribute? GetFirstDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) where TAttribute : Attribute { - Dictionary cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); + Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); - foreach (KeyValuePair cachedAttribute in cachedAttributes) + foreach (Attribute cachedAttribute in cachedAttributes) { if (AttributeComparer.IsDerived(cachedAttribute)) { - return (TAttribute)cachedAttribute.Value; + return (TAttribute)cachedAttribute; } } @@ -491,20 +499,15 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro internal TAttributeType? GetDerivedAttribute(MemberInfo memberInfo, bool inherit) where TAttributeType : Attribute { - Dictionary? attributes = GetCustomAttributesCached(memberInfo, inherit); - if (attributes == null) - { - return null; - } + Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); - // Try to find the attribute that is derived from baseAttrType. - foreach (Attribute attribute in attributes.Values) + foreach (Attribute attribute in attributes) { DebugEx.Assert(attribute != null, "ReflectHelper.DefinesAttributeDerivedFrom: internal error: wrong value in the attributes dictionary."); if (AttributeComparer.IsDerived(attribute)) { - return attribute as TAttributeType; + return (TAttributeType)attribute; } } @@ -521,20 +524,16 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro internal IEnumerable GetDerivedAttributes(ICustomAttributeProvider attributeProvider, bool inherit) where TAttributeType : Attribute { - Dictionary? attributes = GetCustomAttributesCached(attributeProvider, inherit); - if (attributes == null) - { - yield break; - } + Attribute[] attributes = GetCustomAttributesCached(attributeProvider, inherit); // Try to find the attribute that is derived from baseAttrType. - foreach (Attribute attribute in attributes.Values) + foreach (Attribute attribute in attributes) { DebugEx.Assert(attribute != null, "ReflectHelper.DefinesAttributeDerivedFrom: internal error: wrong value in the attributes dictionary."); if (AttributeComparer.IsDerived(attribute)) { - yield return attribute as TAttributeType; + yield return (TAttributeType)attribute; } } } @@ -559,7 +558,7 @@ internal IEnumerable GetDerivedAttributes(ICusto /// The member to inspect. /// Look at inheritance chain. /// attributes defined. - private Dictionary GetCustomAttributesCached(ICustomAttributeProvider attributeProvider, bool inherit) + private Attribute[] GetCustomAttributesCached(ICustomAttributeProvider attributeProvider, bool inherit) { if (inherit) { @@ -578,22 +577,19 @@ internal IEnumerable GetDerivedAttributes(ICusto // If the information is cached, then use it otherwise populate the cache using // the reflection APIs. - Dictionary GetOrAddAttributes(Dictionary> cache, ICustomAttributeProvider attributeProvider, bool inherit) + Attribute[] GetOrAddAttributes(Dictionary cache, ICustomAttributeProvider attributeProvider, bool inherit) { - if (cache.TryGetValue(attributeProvider, out Dictionary? attributes)) + if (cache.TryGetValue(attributeProvider, out Attribute[]? attributes)) { return attributes; } // Populate the cache - attributes = []; - - object[]? customAttributesArray = null; try { // This is where we get the data for our cache. It required to use call to Reflection here. #pragma warning disable RS0030 // Do not use banned APIs - customAttributesArray = NotCachedReflectHelper.GetCustomAttributesNotCached(attributeProvider, inherit); + attributes = NotCachedReflectHelper.GetCustomAttributesNotCached(attributeProvider, inherit)?.Cast().ToArray() ?? Array.Empty(); #pragma warning restore RS0030 // Do not use banned APIs } catch (Exception ex) @@ -620,14 +616,7 @@ internal IEnumerable GetDerivedAttributes(ICusto return null; } - DebugEx.Assert(customAttributesArray != null, "attributes should not be null."); - - foreach (object customAttribute in customAttributesArray) - { - Type attributeType = customAttribute.GetType(); - // TODO: this overwrites any multiple attribute entry. - attributes[attributeType.AssemblyQualifiedName!] = (Attribute)customAttribute; - } + DebugEx.Assert(attributes != null, "attributes should not be null."); cache.Add(attributeProvider, attributes); @@ -638,13 +627,13 @@ internal IEnumerable GetDerivedAttributes(ICusto internal IEnumerable? GetNonDerivedAttributes(MethodInfo methodInfo, bool inherit) where TAttribute : Attribute { - Dictionary cachedAttributes = GetCustomAttributesCached(methodInfo, inherit); + Attribute[] cachedAttributes = GetCustomAttributesCached(methodInfo, inherit); - foreach (KeyValuePair cachedAttribute in cachedAttributes) + foreach (Attribute cachedAttribute in cachedAttributes) { if (AttributeComparer.IsNonDerived(cachedAttribute)) { - yield return (TAttribute)cachedAttribute.Value; + yield return (TAttribute)cachedAttribute; } } } @@ -652,8 +641,8 @@ internal IEnumerable GetDerivedAttributes(ICusto internal class AttributeComparer { - public static bool IsNonDerived(KeyValuePair cachedAttribute) => - typeof(TAttribute).AssemblyQualifiedName == cachedAttribute.Key; + public static bool IsNonDerived(Attribute attribute) => + attribute is TAttribute; public static bool IsDerived(KeyValuePair cachedAttribute) => IsDerived(cachedAttribute.Value); From 1f55e25cb5e543ae53c4eb20fbd0ea29da533e1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 9 May 2024 22:49:54 +0200 Subject: [PATCH 05/20] I am bored --- TestFx.sln | 6 +++ .../MSTest.TestAdapter/Execution/TypeCache.cs | 12 ++++++ .../Helpers/ReflectHelper.cs | 38 ------------------- .../Services/FileOperations.cs | 12 +++++- 4 files changed, 29 insertions(+), 39 deletions(-) diff --git a/TestFx.sln b/TestFx.sln index cc9f95b2d5..988171ac50 100644 --- a/TestFx.sln +++ b/TestFx.sln @@ -201,6 +201,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Playground", "samples\Playg EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MSTest.Acceptance.IntegrationTests", "test\IntegrationTests\MSTest.Acceptance.IntegrationTests\MSTest.Acceptance.IntegrationTests.csproj", "{BCB42780-C559-40B6-8C4A-85EBC464AAA8}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "mstest133", "..\..\t\mstest133\prj\mstest133.csproj", "{DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -427,6 +429,10 @@ Global {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Debug|Any CPU.Build.0 = Debug|Any CPU {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Release|Any CPU.ActiveCfg = Release|Any CPU {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Release|Any CPU.Build.0 = Release|Any CPU + {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Debug|Any CPU.Build.0 = Debug|Any CPU + {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Release|Any CPU.ActiveCfg = Release|Any CPU + {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index 0359106887..e8fb238b81 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -493,6 +493,12 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) private bool IsAssemblyOrClassInitializeMethod(MethodInfo methodInfo) where TInitializeAttribute : Attribute { + // TODO: can we? it will then never throw for invalid, this is inconsitent in the codebase. + //if (!methodInfo.IsStatic) + //{ + // return false; + //} + if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; @@ -516,6 +522,12 @@ private bool IsAssemblyOrClassInitializeMethod(MethodInfo private bool IsAssemblyOrClassCleanupMethod(MethodInfo methodInfo) where TCleanupAttribute : Attribute { + // TODO: can we? it will then never throw for invalid, this is inconsitent in the codebase. + //if (!methodInfo.IsStatic) + //{ + // return false; + //} + if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index 8e90e8a400..8f4a73a498 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -50,20 +50,6 @@ public virtual bool IsNonDerivedAttributeDefined(MemberInfo memberIn // Get attributes defined on the member from the cache. Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); - string requiredAttributeQualifiedName = typeof(TAttribute).AssemblyQualifiedName!; - if (attributes == null) - { - // TODO: - bool a = true; - if (a) - { - throw new NotSupportedException("THIS FALLBACK!"); - } - - // If we could not obtain all attributes from cache, just get the one we need. - TAttribute[] specificAttributes = GetCustomAttributesOfTypeNotCached(memberInfo, inherit); - return specificAttributes.Any(a => string.Equals(a.GetType().AssemblyQualifiedName, requiredAttributeQualifiedName, StringComparison.Ordinal)); - } foreach (Attribute attribute in attributes) { @@ -317,30 +303,6 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in ? throw new ArgumentNullException(nameof(method)) : returnType == null ? throw new ArgumentNullException(nameof(returnType)) : method.ReturnType.Equals(returnType); - /// - /// Get custom attributes on a member for both normal and reflection only load. - /// - /// Type of attribute to retrieve. - /// Member for which attributes needs to be retrieved. - /// If inherited type of attribute. - /// All attributes of give type on member. - [return: NotNullIfNotNull(nameof(memberInfo))] - internal static TAttribute[]? GetCustomAttributesOfTypeNotCached(MemberInfo? memberInfo, bool inherit) - where TAttribute : Attribute - { - if (memberInfo == null) - { - return null; - } - - object[] attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes( - memberInfo, - typeof(TAttribute), - inherit); - - return attributesArray!.OfType().ToArray(); // TODO: Investigate if we rely on NRE - } - /// /// Returns true when the method is declared in the assembly where the type is declared. /// diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs index 65100a103c..34fc9b3ff3 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs @@ -19,6 +19,8 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices; /// public class FileOperations : IFileOperations { + Dictionary _assemblyCache = new(); + #if WIN_UI private readonly bool _isPackaged; @@ -45,7 +47,15 @@ public Assembly LoadAssembly(string assemblyName, bool isReflectionOnly) } #endif string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(assemblyName); - return Assembly.Load(new AssemblyName(fileNameWithoutExtension)); + if (_assemblyCache.TryGetValue(fileNameWithoutExtension, out Assembly? assembly)) + { + return assembly; + } + + var asm = Assembly.Load(new AssemblyName(fileNameWithoutExtension)); + _assemblyCache.Add(fileNameWithoutExtension, asm); + + return asm; #elif NETFRAMEWORK #pragma warning disable IDE0022 // Use expression body for method return isReflectionOnly ? Assembly.ReflectionOnlyLoadFrom(assemblyName) : Assembly.LoadFrom(assemblyName); From 3aa41bab3e9e6b85b41c9335d7a79b070a3ed8e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Fri, 10 May 2024 11:20:40 +0200 Subject: [PATCH 06/20] remove usage of single and skip object type and methods --- TestFx.sln | 6 ---- .../MSTest.TestAdapter/BannedSymbols.txt | 3 +- .../Discovery/AssemblyEnumerator.cs | 12 ++++---- .../Discovery/TestMethodValidator.cs | 12 ++++++-- .../Discovery/TypeEnumerator.cs | 15 +++++++--- .../Discovery/TypeValidator.cs | 5 ++++ .../Execution/KnownNonTestMethods.cs | 26 ++++++++++++++++- .../MSTest.TestAdapter/Execution/TypeCache.cs | 28 +++++++------------ .../Extensions/MethodInfoExtensions.cs | 2 +- .../Helpers/ReflectHelper.cs | 9 ++++++ 10 files changed, 77 insertions(+), 41 deletions(-) diff --git a/TestFx.sln b/TestFx.sln index 988171ac50..cc9f95b2d5 100644 --- a/TestFx.sln +++ b/TestFx.sln @@ -201,8 +201,6 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Playground", "samples\Playg EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MSTest.Acceptance.IntegrationTests", "test\IntegrationTests\MSTest.Acceptance.IntegrationTests\MSTest.Acceptance.IntegrationTests.csproj", "{BCB42780-C559-40B6-8C4A-85EBC464AAA8}" EndProject -Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "mstest133", "..\..\t\mstest133\prj\mstest133.csproj", "{DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}" -EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -429,10 +427,6 @@ Global {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Debug|Any CPU.Build.0 = Debug|Any CPU {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Release|Any CPU.ActiveCfg = Release|Any CPU {BCB42780-C559-40B6-8C4A-85EBC464AAA8}.Release|Any CPU.Build.0 = Release|Any CPU - {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Debug|Any CPU.ActiveCfg = Debug|Any CPU - {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Debug|Any CPU.Build.0 = Debug|Any CPU - {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Release|Any CPU.ActiveCfg = Release|Any CPU - {DD58B3FE-1EC4-47A4-9D81-CE6A66CBF3BF}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt index e0d4601a41..fdab0872bf 100644 --- a/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt +++ b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt @@ -1 +1,2 @@ -T:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.NotCachedReflectHelper; This is allowed only for special cases that need to scan all types or methods to see if they are potentially a test class or a test method. Any use on a test class or method should use methods on ReflectHelper because it has cache. +T:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.NotCachedReflectHelper; This is allowed only for tests. +M:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.ReflectHelper.#ctor; This is allowed only for tests. diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index ac37c583bf..f90926a204 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -280,13 +280,11 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMethodInfo testMethodInfo, List tests) { MethodInfo methodInfo = testMethodInfo.MethodInfo; - // TODO: this needs a special method where we grab all attributes and compare them to a type to see if the attribute inherits from additional interface. - // We want to do first or default here, to see if we should go to the expensive path. - IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false)?.OfType(); - if (testDataSources == null || !testDataSources.Any()) - { - return false; - } + + // We don't have a special method to filter attributes that are not derived from Attribute, so we take all + // attributes and filter them. We don't have to care if there is one, because this method is only entered when + // there is at least one (we determine this in TypeEnumerator.GetTestFromMethod. + IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false).OfType(); try { diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs index 5d850697b2..ecb0641255 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs @@ -4,6 +4,7 @@ using System.Globalization; using System.Reflection; +using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Extensions; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; using Microsoft.VisualStudio.TestTools.UnitTesting; @@ -48,9 +49,14 @@ internal TestMethodValidator(ReflectHelper reflectHelper, bool discoverInternals /// Return true if a method is a valid test method. internal virtual bool IsValidTestMethod(MethodInfo testMethodInfo, Type type, ICollection warnings) { - // Use non-caching reflection helper to check if a method is a valid test method. We don't want to retrieve - // all attributes for every single method, and we also don't want to cache them for methods that we won't look at again. - if (!_reflectHelper.IsDerivedAttributeDefined(testMethodInfo, inherit: false)) + // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every method info in the + // assembly, this is because when we discover and run we will repeatedly inspect all the methods in the assembly, and this + // gives us a better performance. + // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run, + // but the difference is quite small, and we don't expect a huge amount of non-test methods in the assembly. + // + // Also skip all methods coming from object, because they cannot be tests. + if (testMethodInfo.DeclaringType == typeof(object) || !_reflectHelper.IsDerivedAttributeDefined(testMethodInfo, inherit: false)) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 6c04c60af3..2fc6aba3cb 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -73,6 +73,8 @@ internal Collection GetTests(ICollection warnings) var tests = new Collection(); // Test class is already valid. Verify methods. + // PERF: GetRuntimeMethods is used here to get all methods, including non-public, and static methods. + // if we rely on analyzers to identify all invalid methods on build, we can change this to fit the current settings. foreach (MethodInfo method in _type.GetRuntimeMethods()) { bool isMethodDeclaredInTestTypeAssembly = _reflectHelper.IsMethodDeclaredInSameAssemblyAsType(method, _type); @@ -147,8 +149,15 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT method.DeclaringType.GetTypeInfo().Assembly); } - // TODO: this needs a special method where we grab all attributes and compare them to a type to see if the attribute inherits from additional interface. - // We want to do first or default here, to see if we should go to the expensive path. + // PERF: AssemblyEnumerator would later try to figure if this is a data driven test method + // by creating a TestMethodInfo. This is really expensive for discovery, and should be done only + // when we are a data driven test. + // Instead we check if we have any data driven attribute here, and skip creating TestMethod info + // in AssemblyEnumerator. + // + // The foreach below could move to ReflectHelper, but this is only place where we want to check if + // an attribute implements something different than an Attribute derived class. So instead we just + // grab all attributes, and do the check ourselves. bool isDataDriven = false; foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes(method, inherit: true)) { @@ -161,8 +170,6 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT testMethod.DataType = isDataDriven ? DynamicDataType.ITestDataSource : DynamicDataType.None; - // TODO end - var testElement = new UnitTestElement(testMethod) { // Get compiler generated type name for async test method (either void returning or task returning). diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs index bfa6b97382..5b23e68d06 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs @@ -51,6 +51,11 @@ internal virtual bool IsValidTestClass(Type type, ICollection warnings) { TypeInfo typeInfo = type.GetTypeInfo(); + // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every class info in the + // assembly, this is because when we discover and run we will repeatedly inspect all the types in the assembly, and this + // gives us a better performance. + // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run, + // but the difference is quite small, and we don't expect a huge amount of non-test classes in the assembly. if (!typeInfo.IsClass || !_reflectHelper.IsDerivedAttributeDefined(typeInfo, inherit: false)) { return false; diff --git a/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs b/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs index 2942c345b5..25acb413f0 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs @@ -5,5 +5,29 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; internal static class KnownNonTestMethods { - public static HashSet Methods { get; } = [nameof(object.Equals), nameof(object.GetHashCode), nameof(object.GetType), nameof(object.ReferenceEquals), nameof(object.ToString)]; + private static readonly string[] MethodNames = [ + nameof(Equals), + nameof(GetHashCode), + nameof(GetType), + nameof(ReferenceEquals), + nameof(ToString) + ]; + + public static bool Contains(string methodName) + { + if (methodName == "Equals") + { + Console.WriteLine($"Known non test methods called with: {methodName}"); + Console.WriteLine(Environment.StackTrace); + } + foreach (string method in MethodNames) + { + if (string.Equals(method, methodName, StringComparison.Ordinal)) + { + return true; + } + } + + return false; + } } diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index e8fb238b81..b51bd29c72 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -306,11 +306,6 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) foreach (MethodInfo methodInfo in classType.GetTypeInfo().DeclaredMethods) { - if (KnownNonTestMethods.Methods.Contains(methodInfo.Name)) - { - continue; - } - if (methodInfo.IsPublic && !methodInfo.IsStatic) { // Update test initialize/cleanup method @@ -325,15 +320,12 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) } Type? baseType = classType.GetTypeInfo().BaseType; - while (baseType != null) + + // PERF: Don't inspect object, no test methods or setups can be defined on it. + while (baseType != null && baseType != typeof(object)) { foreach (MethodInfo methodInfo in baseType.GetTypeInfo().DeclaredMethods) { - if (KnownNonTestMethods.Methods.Contains(methodInfo.Name)) - { - continue; - } - if (methodInfo.IsPublic && !methodInfo.IsStatic) { // Update test initialize/cleanup method from base type. @@ -441,7 +433,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) { assemblyInfo.AssemblyInitializeMethod = methodInfo; - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -460,7 +452,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) else if (IsAssemblyOrClassCleanupMethod(methodInfo)) { assemblyInfo.AssemblyCleanupMethod = methodInfo; - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -582,7 +574,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isInitializeMethod) { - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -615,7 +607,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isCleanupMethod) { - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -681,7 +673,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (hasTestInitialize) { - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -712,7 +704,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (hasTestCleanup) { - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); if (timeoutAttribute != null) { if (!methodInfo.HasCorrectTimeout(timeoutAttribute)) @@ -899,7 +891,7 @@ private MethodInfo GetMethodInfoForTestMethod(TestMethod testMethod, TestClassIn private int GetTestTimeout(MethodInfo methodInfo, TestMethod testMethod) { DebugEx.Assert(methodInfo != null, "TestMethod should be non-null"); - TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetSingleNonDerivedAttributeOrDefault(methodInfo, inherit: false, nullOnMultiple: true); + TimeoutAttribute? timeoutAttribute = _reflectionHelper.GetFirstNonDerivedAttributeOrDefault(methodInfo, inherit: false); int globalTimeout = MSTestSettings.CurrentSettings.TestTimeout; if (timeoutAttribute != null) diff --git a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs index 2f9ece4e34..acc5e14d6c 100644 --- a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs +++ b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs @@ -96,7 +96,7 @@ internal static bool HasCorrectTimeout(this MethodInfo method, TimeoutAttribute? DebugEx.Assert(method != null, "method should not be null."); // TODO: redesign this, probably change this to GetTimeout? so we don't have to do this weird dance? - timeoutAttribute ??= ReflectHelper.Instance.GetSingleNonDerivedAttributeOrDefault(method, inherit: false, nullOnMultiple: true); + timeoutAttribute ??= ReflectHelper.Instance.GetFirstNonDerivedAttributeOrDefault(method, inherit: false); // Timeout cannot be less than 0. return !(timeoutAttribute?.Timeout < 0); diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index 8f4a73a498..f0933e35b3 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -197,6 +197,15 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in #endif public override object InitializeLifetimeService() => null!; + /// + /// Gets single attribute, DON'T use this together with attribute that does not allow multiple, to avoid eating the cost of checking for more when there cannot be more. + /// + /// Type of the attribute to find. + /// The type, assembly or method. + /// If we should inspect parents of this type. + /// If null will be returned when multiple attributes are found, otherwise we throw. + /// The attribute that is found or null. + /// Throws when multiple attributes are found (the attribute must allow multiple). public TAttribute? GetSingleNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) where TAttribute : Attribute { From dacbe26b60ce4a17516a332de253a7e31999ded4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Fri, 10 May 2024 15:50:46 +0200 Subject: [PATCH 07/20] Fix more tests --- .../MSTest.TestAdapter/BannedSymbols.txt | 3 +- .../Execution/TestMethodInfo.cs | 10 +- .../MSTest.TestAdapter/Execution/TypeCache.cs | 10 +- .../Helpers/AttributeComparer.cs | 18 ++ .../Helpers/INotCachedAttributeHelper.cs | 11 + .../Helpers/ReflectHelper.cs | 296 ++++-------------- .../Interfaces/IReflectionOperations.cs | 2 - .../Services/FileOperations.cs | 21 +- .../Services/ReflectionOperations.cs | 3 - .../Discovery/TestMethodValidatorTests.cs | 8 +- .../Discovery/TypeEnumeratorTests.cs | 184 +++++------ .../Discovery/TypeValidatorTests.cs | 4 +- .../Execution/TestMethodRunnerTests.cs | 6 +- .../Execution/TypeCacheTests.cs | 46 +-- .../Helpers/ReflectHelperTests.cs | 48 +-- .../TestableReflectHelper.cs | 64 ++-- 16 files changed, 283 insertions(+), 451 deletions(-) create mode 100644 src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs create mode 100644 src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs diff --git a/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt index fdab0872bf..91178cbc64 100644 --- a/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt +++ b/src/Adapter/MSTest.TestAdapter/BannedSymbols.txt @@ -1,2 +1 @@ -T:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.NotCachedReflectHelper; This is allowed only for tests. -M:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.ReflectHelper.#ctor; This is allowed only for tests. +M:Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers.ReflectHelper.#ctor; This is allowed only for tests. diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs b/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs index 4ff5bdefb9..ee998c845c 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TestMethodInfo.cs @@ -82,17 +82,11 @@ public class TestMethodInfo : ITestMethod /// internal TestMethodOptions TestMethodOptions { get; } - // This is allowed because we don't know the usage. -#pragma warning disable RS0030 // Do not use banned APIs - [Obsolete("don't use, but cannot remove")] - public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached(TestMethod, inherit) as Attribute[]; -#pragma warning restore RS0030 // Do not use banned APIs + public Attribute[]? GetAllAttributes(bool inherit) => ReflectHelper.Instance.GetDerivedAttributes(TestMethod, inherit).ToArray(); - [Obsolete("don't use, but cannot remove")] public TAttributeType[] GetAttributes(bool inherit) where TAttributeType : Attribute - => ReflectHelper.Instance.NotCachedReflectHelper.GetCustomAttributesNotCached(TestMethod, inherit) - ?? Array.Empty(); + => ReflectHelper.Instance.GetDerivedAttributes(TestMethod, inherit).ToArray(); /// /// Execute test method. Capture failures, handle async and return result. diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index b51bd29c72..60213f6878 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -290,7 +290,7 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) TestAssemblyInfo assemblyInfo = GetAssemblyInfo(classType); - TestClassAttribute? testClassAttribute = ReflectHelper.Instance.GetDerivedAttribute(classType, false); + TestClassAttribute? testClassAttribute = ReflectHelper.Instance.GetFirstDerivedAttributeOrDefault(classType, inherit: false); DebugEx.Assert(testClassAttribute is not null, "testClassAttribute is null"); var classInfo = new TestClassInfo(classType, constructor, testContextProperty, testClassAttribute, assemblyInfo); @@ -485,7 +485,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) private bool IsAssemblyOrClassInitializeMethod(MethodInfo methodInfo) where TInitializeAttribute : Attribute { - // TODO: can we? it will then never throw for invalid, this is inconsitent in the codebase. + // TODO: can we? it will then never throw for invalid, this is inconsistent in the codebase. //if (!methodInfo.IsStatic) //{ // return false; @@ -592,7 +592,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isBase) { - if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)! + if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)? .InheritanceBehavior == InheritanceBehavior.BeforeEachDerivedClass) { initAndCleanupMethods[0] = methodInfo; @@ -625,7 +625,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method if (isBase) { - if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)! + if (_reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true)? .InheritanceBehavior == InheritanceBehavior.BeforeEachDerivedClass) { initAndCleanupMethods[1] = methodInfo; @@ -788,7 +788,7 @@ private bool IsAssemblyOrClassCleanupMethod(MethodInfo method private TestMethodAttribute? GetTestMethodAttribute(MethodInfo methodInfo, TestClassInfo testClassInfo) { // Get the derived TestMethod attribute from reflection - TestMethodAttribute? testMethodAttribute = _reflectionHelper.GetDerivedAttribute(methodInfo, false); + TestMethodAttribute? testMethodAttribute = _reflectionHelper.GetFirstDerivedAttributeOrDefault(methodInfo, inherit: false); // Get the derived TestMethod attribute from Extended TestClass Attribute // If the extended TestClass Attribute doesn't have extended TestMethod attribute then base class returns back the original testMethod Attribute diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs new file mode 100644 index 0000000000..7d364bbabd --- /dev/null +++ b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs @@ -0,0 +1,18 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; + +internal class AttributeComparer +{ + public static bool IsNonDerived(Attribute attribute) => + attribute is TAttribute; + + public static bool IsDerived(Attribute attribute) + { + Type attributeType = attribute.GetType(); + // IsSubclassOf returns false when the types are equal. + return attributeType == typeof(TAttribute) + || attributeType.IsSubclassOf(typeof(TAttribute)); + } +} diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs new file mode 100644 index 0000000000..0795da4019 --- /dev/null +++ b/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs @@ -0,0 +1,11 @@ +// 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.TestPlatform.MSTest.TestAdapter.Helpers; + +internal interface INotCachedAttributeHelper +{ + object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); +} diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index f0933e35b3..bcb9e9ccb9 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -15,23 +15,27 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; internal class ReflectHelper : MarshalByRefObject { - // It is okay to use this api here, because we need to access the real reflection. -#pragma warning disable RS0030 // Do not use banned APIs - private static readonly Lazy InstanceValue = new(() => new ReflectHelper(new NotCachedReflectHelper())); -#pragma warning restore RS0030 // Do not use banned APIs + private static readonly Lazy InstanceValue = new(() => new ReflectHelper(new NotCachedAttributeHelper())); - // Caches below could be unified by using ICustomAttributeProvider. But the underlying IReflectionOperations is public and we would have to change or duplicate it. + // PERF: This was moved from Dictionary> to Dictionary + // this allows us to store multiple attributes of the same type if we find them. It also has lower memory footprint, and is faster + // when we are going through the whole collection. Giving us overall better perf. private readonly Dictionary _inheritedAttributeCache = []; private readonly Dictionary _nonInheritedAttributeCache = []; - internal /* for tests only */ ReflectHelper(INotCachedReflectHelper notCachedReflectHelper) => - NotCachedReflectHelper = notCachedReflectHelper; + internal /* for tests only */ ReflectHelper(INotCachedAttributeHelper notCachedAttributeHelper) => + NotCachedAttributes = notCachedAttributeHelper ?? new NotCachedAttributeHelper(); + + internal /* for tests only, because of Moq */ ReflectHelper() + : this(new NotCachedAttributeHelper()) + { + } private readonly AttributeComparer _attributeComparer = new(); public static ReflectHelper Instance => InstanceValue.Value; - public INotCachedReflectHelper NotCachedReflectHelper { get; } + public INotCachedAttributeHelper NotCachedAttributes { get; } /// /// Checks to see if a member or type is decorated with the given attribute. The type is checked exactly. If attribute is derived (inherits from) a class, e.g. [MyTestClass] from [TestClass] it won't match if you look for [TestClass]. The inherit parameter does not impact this checking. @@ -91,7 +95,7 @@ public virtual bool IsDerivedAttributeDefined(Type type, bool inheri /// Type to test. /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. /// True if the attribute of the specified type is defined on this member or a class. - public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool inherit) + public virtual /* for testing */ bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool inherit) where TAttribute : Attribute { if (memberInfo == null) @@ -143,10 +147,10 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in DebugEx.Assert(methodInfo != null, "MethodInfo should be non-null"); // Get the expected exception attribute - IEnumerable expectedExceptions; + ExpectedExceptionBaseAttribute? expectedException; try { - expectedExceptions = GetDerivedAttributes(methodInfo, inherit: true); + expectedException = GetFirstDerivedAttributeOrDefault(methodInfo, inherit: true); } catch (Exception ex) { @@ -162,26 +166,11 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in } // TODO: we can probably do better if we grab the enumerator? - if (!expectedExceptions.Any()) + if (expectedException == null) { return null; } - // Verify that there is only one attribute (multiple attributes derived from - // ExpectedExceptionBaseAttribute are not allowed on a test method) - if (expectedExceptions.Count() > 1) - { - string errorMessage = string.Format( - CultureInfo.CurrentCulture, - Resource.UTA_MultipleExpectedExceptionsOnTestMethod, - testMethod.FullClassName, - testMethod.Name); - throw new TypeInspectionException(errorMessage); - } - - // Set the expected exception attribute to use for this test - ExpectedExceptionBaseAttribute expectedException = expectedExceptions.First(); - return expectedException; } @@ -198,46 +187,15 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in public override object InitializeLifetimeService() => null!; /// - /// Gets single attribute, DON'T use this together with attribute that does not allow multiple, to avoid eating the cost of checking for more when there cannot be more. + /// Gets first attribute that matches the type (but is not derived from it). Use this together with attribute that does not allow multiple. + /// In such case there cannot be more attributes, and this will avoid the cost of + /// checking for more than one attribute. /// /// Type of the attribute to find. /// The type, assembly or method. /// If we should inspect parents of this type. - /// If null will be returned when multiple attributes are found, otherwise we throw. /// The attribute that is found or null. /// Throws when multiple attributes are found (the attribute must allow multiple). - public TAttribute? GetSingleNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) - where TAttribute : Attribute - { - Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); - - int count = 0; - TAttribute? foundAttribute = default; - foreach (Attribute cachedAttribute in cachedAttributes) - { - if (AttributeComparer.IsNonDerived(cachedAttribute)) - { - foundAttribute = (TAttribute)cachedAttribute; - count++; - } - } - - if (count == 0) - { - return null; - } - - // We found what we were looking for. - if (count == 1) - { - return foundAttribute; - } - - return nullOnMultiple - ? null - : throw new InvalidOperationException($"Found {count} instances of attribute {typeof(TAttribute)} on class, but only single one was expected."); - } - public TAttribute? GetFirstNonDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) where TAttribute : Attribute { @@ -254,39 +212,17 @@ public bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool in return null; } - public TAttribute? GetSingleDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit, bool nullOnMultiple) - where TAttribute : Attribute - { - Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); - - int count = 0; - TAttribute? foundAttribute = default; - foreach (Attribute cachedAttribute in cachedAttributes) - { - if (AttributeComparer.IsDerived(cachedAttribute)) - { - foundAttribute = (TAttribute)cachedAttribute; - count++; - } - } - - if (count == 0) - { - return null; - } - - // We found what we were looking for. - if (count == 1) - { - return foundAttribute; - } - - return nullOnMultiple - ? null - : throw new InvalidOperationException($"Found {count} instances of attribute {typeof(TAttribute)} on class, but only single one was expected."); - } - - public TAttribute? GetFirstDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) + /// + /// Gets first attribute that matches the type or is derived from it. + /// Use this together with attribute that does not allow multiple. In such case there cannot be more attributes, and this will avoid the cost of + /// checking for more than one attribute. + /// + /// Type of the attribute to find. + /// The type, assembly or method. + /// If we should inspect parents of this type. + /// The attribute that is found or null. + /// Throws when multiple attributes are found (the attribute must allow multiple). + public virtual /* for tests, for moq */ TAttribute? GetFirstDerivedAttributeOrDefault(ICustomAttributeProvider attributeProvider, bool inherit) where TAttribute : Attribute { Attribute[] cachedAttributes = GetCustomAttributesCached(attributeProvider, inherit); @@ -353,8 +289,8 @@ internal virtual string[] GetTestCategories(MemberInfo categoryAttributeProvider /// The type that owns . /// True if test method should not run in parallel. internal bool IsDoNotParallelizeSet(MemberInfo testMethod, Type owningType) - => IsDerivedAttributeDefined(testMethod, inherit: false) - || IsDerivedAttributeDefined(owningType, inherit: false); + => IsDerivedAttributeDefined(testMethod, inherit: true) + || IsDerivedAttributeDefined(owningType, inherit: true); /// /// Get the parallelization behavior for a test assembly. @@ -405,7 +341,7 @@ internal static bool IsDoNotParallelizeSet(Assembly assembly) /// The member to inspect. /// Priority value if defined. Null otherwise. internal virtual int? GetPriority(MemberInfo priorityAttributeProvider) => - GetSingleDerivedAttributeOrDefault(priorityAttributeProvider, inherit: true, nullOnMultiple: true)?.Priority; + GetFirstDerivedAttributeOrDefault(priorityAttributeProvider, inherit: true)?.Priority; /// /// Priority if any set for test method. Will return priority if attribute is applied to TestMethod @@ -414,7 +350,7 @@ internal static bool IsDoNotParallelizeSet(Assembly assembly) /// The member to inspect. /// Priority value if defined. Null otherwise. internal virtual string? GetIgnoreMessage(MemberInfo ignoreAttributeProvider) => - GetSingleDerivedAttributeOrDefault(ignoreAttributeProvider, inherit: true, nullOnMultiple: true)?.IgnoreMessage; + GetFirstDerivedAttributeOrDefault(ignoreAttributeProvider, inherit: true)?.IgnoreMessage; /// /// Gets the class cleanup lifecycle for the class, if set. @@ -432,9 +368,9 @@ internal static bool IsDoNotParallelizeSet(Assembly assembly) var cleanupBehaviors = new HashSet( classInfo.BaseClassCleanupMethodsStack - .Select(x => x.GetCustomAttribute(true)?.CleanupBehavior)) + .Select(x => GetFirstDerivedAttributeOrDefault(x, inherit: true)?.CleanupBehavior)) { - classInfo.ClassCleanupMethod?.GetCustomAttribute(true)?.CleanupBehavior, + classInfo.ClassCleanupMethod == null ? null : GetFirstDerivedAttributeOrDefault(classInfo.ClassCleanupMethod, inherit: true)?.CleanupBehavior, }; return cleanupBehaviors.Contains(ClassCleanupBehavior.EndOfClass) @@ -460,30 +396,6 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro } } - /// - /// Get attribute defined on a method which is of given type of subtype of given type. - /// - /// The attribute type. - /// The member to inspect. - /// Look at inheritance chain. - /// An instance of the attribute. - internal TAttributeType? GetDerivedAttribute(MemberInfo memberInfo, bool inherit) - where TAttributeType : Attribute - { - Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); - - foreach (Attribute attribute in attributes) - { - DebugEx.Assert(attribute != null, "ReflectHelper.DefinesAttributeDerivedFrom: internal error: wrong value in the attributes dictionary."); - - if (AttributeComparer.IsDerived(attribute)) - { - return (TAttributeType)attribute; - } - } - - return null; - } /// /// Get attribute defined on a method which is of given type of subtype of given type. @@ -492,7 +404,7 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro /// The member to inspect. /// Look at inheritance chain. /// An instance of the attribute. - internal IEnumerable GetDerivedAttributes(ICustomAttributeProvider attributeProvider, bool inherit) + internal virtual /* for tests, for moq */ IEnumerable GetDerivedAttributes(ICustomAttributeProvider attributeProvider, bool inherit) where TAttributeType : Attribute { Attribute[] attributes = GetCustomAttributesCached(attributeProvider, inherit); @@ -516,7 +428,7 @@ internal IEnumerable GetDerivedAttributes(ICusto /// owner if attribute is applied to TestMethod, else null. private string? GetOwner(MemberInfo ownerAttributeProvider) { - OwnerAttribute? ownerAttribute = GetSingleDerivedAttributeOrDefault(ownerAttributeProvider, inherit: true, nullOnMultiple: true); + OwnerAttribute? ownerAttribute = GetFirstDerivedAttributeOrDefault(ownerAttributeProvider, inherit: true); return ownerAttribute?.Owner; } @@ -558,10 +470,7 @@ Attribute[] GetOrAddAttributes(Dictionary // Populate the cache try { - // This is where we get the data for our cache. It required to use call to Reflection here. -#pragma warning disable RS0030 // Do not use banned APIs - attributes = NotCachedReflectHelper.GetCustomAttributesNotCached(attributeProvider, inherit)?.Cast().ToArray() ?? Array.Empty(); -#pragma warning restore RS0030 // Do not use banned APIs + attributes = NotCachedAttributes.GetCustomAttributesNotCached(attributeProvider, inherit)?.Cast().ToArray() ?? Array.Empty(); } catch (Exception ex) { @@ -608,124 +517,31 @@ Attribute[] GetOrAddAttributes(Dictionary } } } -} - -internal class AttributeComparer -{ - public static bool IsNonDerived(Attribute attribute) => - attribute is TAttribute; - - public static bool IsDerived(KeyValuePair cachedAttribute) => - IsDerived(cachedAttribute.Value); - - public static bool IsDerived(Attribute attribute) - { - Type attributeType = attribute.GetType(); - // IsSubclassOf returns false when the types are equal. - return attributeType == typeof(TAttribute) - || attributeType.IsSubclassOf(typeof(TAttribute)); - } -} -internal interface INotCachedReflectHelper -{ - object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); - TAttribute[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); - bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit); -} - -/// -/// Reflection helper that is accessing Reflection directly, and won't cache the results. This is used internally by ReflectHelper. -/// Outside of ReflectHelper this should be used only to do the most basic checks on classes, and types, that will determine that a class or a method is NOT -/// part of the discovery or execution. E.g. checking that a class has TestClass attribute. -/// -internal class NotCachedReflectHelper : INotCachedReflectHelper -{ /// - /// Checks if an attribute of the given type, or and attribute inheriting from that type, is defined on the class or a method. - /// Use this to check single attribute on a class or method to see if it is eligible for being a test. - /// DO NOT use this repeatedly on a type or method that you already know is a test. + /// Reflection helper that is accessing Reflection directly, and won't cache the results. /// - /// Type of the attribute to check. - public virtual bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + internal class NotCachedAttributeHelper : INotCachedAttributeHelper { - if (attributeProvider is MemberInfo memberInfo) - { - // This is cheaper than getting all the attributes and filtering them. This will return true for - // classes that have [TestClass], and for methods that have [TestMethod]. - if (PlatformServiceProvider.Instance.ReflectionOperations.IsAttributeDefined(memberInfo, typeof(TAttribute), inherit)) + /// + /// Get custom attributes on a member without cache. + /// + /// Member for which attributes needs to be retrieved. + /// If inherited type of attribute. + /// All attributes of give type on member. + [return: NotNullIfNotNull(nameof(attributeProvider))] + public object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + { + if (attributeProvider == null) { - return true; + return null; } - } - // This tries to find an attribute that derives from the given attribute e.g. [TestMethod]. - // This is more expensive than the check above. - foreach (object attribute in GetCustomAttributesNotCached(attributeProvider, inherit)) - { - if (AttributeComparer.IsDerived((Attribute)attribute)) - { - return true; - } - } + object[] attributesArray = attributeProvider is MemberInfo memberInfo + ? PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit) + : PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); - return false; - } - - /// - /// Get custom attributes on a member without cache. - /// - /// Member for which attributes needs to be retrieved. - /// If inherited type of attribute. - /// All attributes of give type on member. - [return: NotNullIfNotNull(nameof(attributeProvider))] - public object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) - { - if (attributeProvider == null) - { - return null; + return attributesArray; // TODO: Investigate if we rely on NRE } - - object[] attributesArray; - - if (attributeProvider is MemberInfo memberInfo) - { - attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit); - } - else - { - attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); - } - - return attributesArray; // TODO: Investigate if we rely on NRE } - - /// - /// Get custom attributes on a member without cache. - /// - /// Member for which attributes needs to be retrieved. - /// If inherited type of attribute. - /// All attributes of give type on member. - [return: NotNullIfNotNull(nameof(attributeProvider))] - public TAttribute[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) - { - if (attributeProvider == null) - { - return null; - } - - object[] attributesArray; - - if (attributeProvider is MemberInfo memberInfo) - { - attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit); - } - else - { - attributesArray = PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); - } - - return attributesArray!.OfType().ToArray(); - } - } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs index 0c71a005cb..1da84b921c 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Interfaces/IReflectionOperations.cs @@ -37,6 +37,4 @@ public interface IReflectionOperations /// The attribute type. /// The list of attributes of the given type on the member. Empty list if none found. object[] GetCustomAttributes(Assembly assembly, Type type); - - public bool IsAttributeDefined(MemberInfo memberInfo, Type type, bool inherit); } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs index 34fc9b3ff3..c4ff1a9b96 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs @@ -19,7 +19,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices; /// public class FileOperations : IFileOperations { - Dictionary _assemblyCache = new(); + private readonly Dictionary _assemblyCache = new(); #if WIN_UI private readonly bool _isPackaged; @@ -57,9 +57,22 @@ public Assembly LoadAssembly(string assemblyName, bool isReflectionOnly) return asm; #elif NETFRAMEWORK -#pragma warning disable IDE0022 // Use expression body for method - return isReflectionOnly ? Assembly.ReflectionOnlyLoadFrom(assemblyName) : Assembly.LoadFrom(assemblyName); -#pragma warning restore IDE0022 // Use expression body for method + if (isReflectionOnly) + { + return Assembly.ReflectionOnlyLoadFrom(assemblyName); + } + else + { + if (_assemblyCache.TryGetValue(assemblyName, out Assembly? assembly)) + { + return assembly; + } + + var asm = Assembly.LoadFrom(assemblyName); + _assemblyCache.Add(assemblyName, asm); + + return asm; + } #endif } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs index 58df9ebab3..4dfbe0b35f 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs @@ -57,7 +57,4 @@ public class ReflectionOperations : IReflectionOperations #else assembly.GetCustomAttributes(type).ToArray(); #endif - - public bool IsAttributeDefined(MemberInfo memberInfo, Type type, bool inherit) => - memberInfo.IsDefined(type, inherit); } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs index fe4fe9cac4..6ab7a4ff1b 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Diagnostics.CodeAnalysis; @@ -39,7 +39,7 @@ public TestMethodValidatorTests() public void IsValidTestMethodShouldReturnFalseForMethodsWithoutATestMethodAttributeOrItsDerivedAttributes() { _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(false); + rh => rh.IsDerivedAttributeDefined(It.IsAny(), false)).Returns(false); Verify(!_testMethodValidator.IsValidTestMethod(_mockMethodInfo.Object, _type, _warnings)); } @@ -56,6 +56,8 @@ public void IsValidTestMethodShouldReturnFalseForGenericTestMethodDefinitions() public void IsValidTestMethodShouldReportWarningsForGenericTestMethodDefinitions() { SetupTestMethod(); + + // _mockMethodInfo.Setup(mi => mi.Module.Assembly.ReflectionOnly).Returns(false); _mockMethodInfo.Setup(mi => mi.IsGenericMethodDefinition).Returns(true); _mockMethodInfo.Setup(mi => mi.DeclaringType.FullName).Returns("DummyTestClass"); _mockMethodInfo.Setup(mi => mi.Name).Returns("DummyTestMethod"); @@ -183,7 +185,7 @@ public void WhenDiscoveryOfInternalsIsEnabledIsValidTestMethodShouldReturnFalseF #endregion private void SetupTestMethod() => _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(true); + rh => rh.IsDerivedAttributeDefined(It.IsAny(), false)).Returns(true); } #region Dummy types diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs index 92fe44cebe..76261ddec2 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs @@ -303,20 +303,20 @@ public void GetTestFromMethodShouldSetTestCategory() Verify(testCategories.SequenceEqual(testElement.TestCategory)); } - //public void GetTestFromMethodShouldSetDoNotParallelize() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + public void GetTestFromMethodShouldSetDoNotParallelize() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // // Setup mocks - // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(It.IsAny())).Returns([new DoNotParallelizeAttribute()]); + // Setup mocks + _mockReflectHelper.Setup(rh => rh.IsDerivedAttributeDefined(It.IsAny(), true)).Returns(true); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement is not null); - // Verify(testElement.DoNotParallelize); - //} + Verify(testElement is not null); + Verify(testElement.DoNotParallelize); + } public void GetTestFromMethodShouldFillTraitsWithTestProperties() { @@ -388,65 +388,65 @@ public void GetTestFromMethodShouldSetPriority() Verify(testElement.Priority == 1); } - //public void GetTestFromMethodShouldSetDescription() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new DescriptionAttribute("Dummy description")); + public void GetTestFromMethodShouldSetDescription() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(methodInfo, true)).Returns(new DescriptionAttribute("Dummy description")); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement.Description == "Dummy description"); - //} + Verify(testElement.Description == "Dummy description"); + } - //public void GetTestFromMethodShouldSetWorkItemIds() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns([new(123), new(345)]); + public void GetTestFromMethodShouldSetWorkItemIds() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + _mockReflectHelper.Setup(rh => rh.GetDerivedAttributes(methodInfo, true)).Returns([new(123), new(345)]); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(new string[] { "123", "345" }.SequenceEqual(testElement.WorkItemIds)); - //} + Verify(new string[] { "123", "345" }.SequenceEqual(testElement.WorkItemIds)); + } - //public void GetTestFromMethodShouldSetWorkItemIdsToNullIfNotAny() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // _mockReflectHelper.Setup(rh => rh.GetCustomAttributes(methodInfo)).Returns(Array.Empty()); + public void GetTestFromMethodShouldSetWorkItemIdsToNullIfNotAny() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + _mockReflectHelper.Setup(rh => rh.GetDerivedAttributes(methodInfo, true)).Returns(Array.Empty()); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement.WorkItemIds is null); - //} + Verify(testElement.WorkItemIds is null); + } - //public void GetTestFromMethodShouldSetCssIteration() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssIterationAttribute("234")); + public void GetTestFromMethodShouldSetCssIteration() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(methodInfo, true)).Returns(new CssIterationAttribute("234")); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement.CssIteration == "234"); - //} + Verify(testElement.CssIteration == "234"); + } - //public void GetTestFromMethodShouldSetCssProjectStructure() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); - // _mockReflectHelper.Setup(rh => rh.GetCustomAttribute(methodInfo)).Returns(new CssProjectStructureAttribute("ProjectStructure123")); + public void GetTestFromMethodShouldSetCssProjectStructure() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod("MethodWithVoidReturnType"); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(methodInfo, true)).Returns(new CssProjectStructureAttribute("ProjectStructure123")); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement.CssProjectStructure == "ProjectStructure123"); - //} + Verify(testElement.CssProjectStructure == "ProjectStructure123"); + } public void GetTestFromMethodShouldSetDeploymentItemsToNullIfNotPresent() { @@ -501,53 +501,53 @@ public void GetTestFromMethodShouldSetDeclaringAssemblyName() Verify(otherAssemblyName == testElement.TestMethod.DeclaringAssemblyName); } - //public void GetTestFromMethodShouldSetDisplayNameToTestMethodNameIfDisplayNameIsNotPresent() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + public void GetTestFromMethodShouldSetDisplayNameToTestMethodNameIfDisplayNameIsNotPresent() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // // Setup mocks to behave like we have [TestMethod] attribute on the method - // _mockReflectHelper.Setup( - // rh => rh.GetCustomAttribute(It.IsAny())).Returns(new TestMethodAttribute()); + // Setup mocks to behave like we have [TestMethod] attribute on the method + _mockReflectHelper.Setup( + rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).Returns(new TestMethodAttribute()); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement is not null); - // Verify(testElement.DisplayName == "MethodWithVoidReturnType"); - //} + Verify(testElement is not null); + Verify(testElement.DisplayName == "MethodWithVoidReturnType"); + } - //public void GetTestFromMethodShouldSetDisplayNameFromTestMethodAttribute() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + public void GetTestFromMethodShouldSetDisplayNameFromTestMethodAttribute() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // // Setup mocks to behave like we have [TestMethod("Test method display name.")] attribute on the method - // _mockReflectHelper.Setup( - // rh => rh.GetCustomAttribute(methodInfo)).Returns(new TestMethodAttribute("Test method display name.")); + // Setup mocks to behave like we have [TestMethod("Test method display name.")] attribute on the method + _mockReflectHelper.Setup( + rh => rh.GetFirstDerivedAttributeOrDefault(methodInfo, true)).Returns(new TestMethodAttribute("Test method display name.")); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement is not null); - // Verify(testElement.DisplayName == "Test method display name."); - //} + Verify(testElement is not null); + Verify(testElement.DisplayName == "Test method display name."); + } - //public void GetTestFromMethodShouldSetDisplayNameFromDataTestMethodAttribute() - //{ - // SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); - // TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); - // MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); + public void GetTestFromMethodShouldSetDisplayNameFromDataTestMethodAttribute() + { + SetupTestClassAndTestMethods(isValidTestClass: true, isValidTestMethod: true, isMethodFromSameAssembly: true); + TypeEnumerator typeEnumerator = GetTypeEnumeratorInstance(typeof(DummyTestClass), "DummyAssemblyName"); + MethodInfo methodInfo = typeof(DummyTestClass).GetMethod(nameof(DummyTestClass.MethodWithVoidReturnType)); - // // Setup mocks to behave like we have [DataTestMethod("Test method display name.")] attribute on the method - // _mockReflectHelper.Setup( - // rh => rh.GetCustomAttribute(methodInfo)).Returns(new DataTestMethodAttribute("Test method display name.")); + // Setup mocks to behave like we have [DataTestMethod("Test method display name.")] attribute on the method + _mockReflectHelper.Setup( + rh => rh.GetFirstDerivedAttributeOrDefault(methodInfo, true)).Returns(new DataTestMethodAttribute("Test method display name.")); - // MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); + MSTest.TestAdapter.ObjectModel.UnitTestElement testElement = typeEnumerator.GetTestFromMethod(methodInfo, true, _warnings); - // Verify(testElement is not null); - // Verify(testElement.DisplayName == "Test method display name."); - //} + Verify(testElement is not null); + Verify(testElement.DisplayName == "Test method display name."); + } #endregion diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs index baeca09614..ca3c51ad81 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeValidatorTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -403,7 +403,7 @@ private static Type[] GetAllTestTypes() #region private methods - private void SetupTestClass() => _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(It.IsAny(), false)).Returns(true); + private void SetupTestClass() => _mockReflectHelper.Setup(rh => rh.IsDerivedAttributeDefined(It.IsAny(), false)).Returns(true); #endregion } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs index 31e659ac41..827563c91f 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs @@ -363,7 +363,7 @@ public void RunTestMethodShouldSetDataRowIndexForDataDrivenTestsWhenDataIsProvid Verify(results[2].DatarowIndex == 2); } - public void RunTestMethodShoudlRunOnlyDataSourceTestsWhenBothDataSourceAndDataRowAreProvided() + public void RunTestMethodShouldRunOnlyDataSourceTestsWhenBothDataSourceAndDataRowAreProvided() { var testMethodInfo = new TestableTestmethodInfo(_methodInfo, _testClassInfo, _testMethodOptions, () => new UTF.TestResult()); var testMethodRunner = new TestMethodRunner(testMethodInfo, _testMethod, _testContextImplementation, false); @@ -405,7 +405,7 @@ public void RunTestMethodShouldFillInDisplayNameWithDataRowDisplayNameIfProvided var attributes = new Attribute[] { dataRowAttribute }; // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(ro => ro.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(ro => ro.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); UnitTestResult[] results = testMethodRunner.RunTestMethod(); @@ -428,7 +428,7 @@ public void RunTestMethodShouldFillInDisplayNameWithDataRowArgumentsIfNoDisplayN var attributes = new Attribute[] { dataRowAttribute }; // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); UnitTestResult[] results = testMethodRunner.RunTestMethod(); diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs index bb7e77157a..ab5dd0dd98 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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; @@ -10,6 +10,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; @@ -251,7 +252,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInitializeAttribute() var testMethod = new TestMethod("TestInit", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); @@ -271,7 +272,7 @@ public void GetTestMethodInfoShouldCacheAssemblyCleanupAttribute() var testMethod = new TestMethod("TestCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); @@ -291,7 +292,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInitAndCleanupAttribute() var testMethod = new TestMethod("TestInitOrCleanup", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); @@ -379,7 +380,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInfoInstanceAndReuseTheCache() var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _typeCache.GetTestMethodInfo( testMethod, @@ -391,7 +392,7 @@ public void GetTestMethodInfoShouldCacheAssemblyInfoInstanceAndReuseTheCache() new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), false); - _mockReflectHelper.Verify(rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true), Times.Once); + _mockReflectHelper.Verify(rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true), Times.Once); Verify(_typeCache.AssemblyInfoCache.Count == 1); } @@ -447,12 +448,12 @@ public void GetTestMethodInfoShouldCacheBaseClassInitializeAttributes() var testMethod = new TestMethod("TestMethod", type.FullName, "A", false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, false)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("AssemblyInit"), false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(baseType.GetMethod("AssemblyInit"))) + rh => rh.GetFirstDerivedAttributeOrDefault(baseType.GetMethod("AssemblyInit"), true)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( @@ -497,11 +498,11 @@ public void GetTestMethodInfoShouldCacheBaseClassCleanupAttributes() var testMethod = new TestMethod("TestMethod", type.FullName, "A", false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(baseType.GetMethod("AssemblyCleanup"), false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(baseType.GetMethod("AssemblyCleanup"))) + rh => rh.GetFirstDerivedAttributeOrDefault(baseType.GetMethod("AssemblyCleanup"), true)) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _typeCache.GetTestMethodInfo( @@ -547,17 +548,17 @@ public void GetTestMethodInfoShouldCacheBaseClassInitAndCleanupAttributes() MethodInfo baseCleanupMethod = baseType.GetMethod("ClassCleanup"); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(baseInitializeMethod, false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(baseInitializeMethod)) + rh => rh.GetFirstDerivedAttributeOrDefault(baseInitializeMethod, true)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(baseCleanupMethod, false)).Returns(true); _mockReflectHelper.Setup( - rh => rh.GetCustomAttribute(baseCleanupMethod)) + rh => rh.GetFirstDerivedAttributeOrDefault(baseCleanupMethod, true)) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper.Setup( @@ -599,13 +600,13 @@ public void GetTestMethodInfoShouldCacheParentAndGrandparentClassInitAndCleanupA .Setup(rh => rh.IsNonDerivedAttributeDefined(grandparentInitMethod, false)) .Returns(true); _mockReflectHelper - .Setup(rh => rh.GetCustomAttribute(grandparentInitMethod)) + .Setup(rh => rh.GetFirstDerivedAttributeOrDefault(grandparentInitMethod, true)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper .Setup(rh => rh.IsNonDerivedAttributeDefined(grandparentCleanupMethod, false)) .Returns(true); _mockReflectHelper - .Setup(rh => rh.GetCustomAttribute(grandparentCleanupMethod)) + .Setup(rh => rh.GetFirstDerivedAttributeOrDefault(grandparentCleanupMethod, true)) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); // Setup parent class init/cleanup methods @@ -613,13 +614,13 @@ public void GetTestMethodInfoShouldCacheParentAndGrandparentClassInitAndCleanupA .Setup(rh => rh.IsNonDerivedAttributeDefined(parentInitMethod, false)) .Returns(true); _mockReflectHelper - .Setup(rh => rh.GetCustomAttribute(parentInitMethod)) + .Setup(rh => rh.GetFirstDerivedAttributeOrDefault(parentInitMethod, true)) .Returns(new UTF.ClassInitializeAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); _mockReflectHelper .Setup(rh => rh.IsNonDerivedAttributeDefined(parentCleanupMethod, false)) .Returns(true); _mockReflectHelper - .Setup(rh => rh.GetCustomAttribute(parentCleanupMethod)) + .Setup(rh => rh.GetFirstDerivedAttributeOrDefault(parentCleanupMethod, true)) .Returns(new UTF.ClassCleanupAttribute(UTF.InheritanceBehavior.BeforeEachDerivedClass)); var testMethod = new TestMethod("TestMethod", type.FullName, "A", isAsync: false); @@ -882,6 +883,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfo() MethodInfo methodInfo = type.GetMethod("TestMethod"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -901,7 +903,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoWithTimeout() _mockReflectHelper.Setup(rh => rh.IsNonDerivedAttributeDefined(methodInfo, false)) .Returns(true); - + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -1020,6 +1022,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoForMethodsAdornedWithADer MethodInfo methodInfo = type.GetMethod("TestMethodWithDerivedTestMethodAttribute"); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -1138,6 +1141,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoForDerivedTestClasses() MethodInfo methodInfo = type.GetRuntimeMethod("DummyTestMethod", Array.Empty()); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -1155,6 +1159,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoForDerivedClassMethodOver MethodInfo methodInfo = type.GetRuntimeMethod("OverloadedTestMethod", Array.Empty()); var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -1176,6 +1181,7 @@ public void GetTestMethodInfoShouldReturnTestMethodInfoForDeclaringTypeMethodOve DeclaringClassFullName = baseType.FullName, }; + _mockReflectHelper.Setup(rh => rh.GetFirstDerivedAttributeOrDefault(It.IsAny(), false)).CallBase(); TestMethodInfo testMethodInfo = _typeCache.GetTestMethodInfo( testMethod, new TestContextImplementation(testMethod, new ThreadSafeStringWriter(null, "test"), new Dictionary()), @@ -1265,7 +1271,7 @@ public void AssemblyInfoListWithExecutableCleanupMethodsShouldReturnEmptyListWhe var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(false); @@ -1287,7 +1293,7 @@ public void AssemblyInfoListWithExecutableCleanupMethodsShouldReturnAssemblyInfo var testMethod = new TestMethod(methodInfo.Name, type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs index 1935004c12..5857d27b05 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Helpers/ReflectHelperTests.cs @@ -142,7 +142,7 @@ public void GetTestCategoryAttributeShouldIncludeTestCategoriesAtMethodLevel() public void IsAttributeDefinedShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMember() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new UTF.TestMethodAttribute() }; @@ -155,7 +155,7 @@ public void IsAttributeDefinedShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMe public void IsAttributeDefinedShouldReturnFalseIfSpecifiedAttributeIsNotDefinedOnAMember() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new UTF.TestClassAttribute() }; @@ -168,7 +168,7 @@ public void IsAttributeDefinedShouldReturnFalseIfSpecifiedAttributeIsNotDefinedO public void IsAttributeDefinedShouldReturnFromCache() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); // Not using mocks here because for some reason a dictionary match of the mock is not returning true in the product code. MethodInfo memberInfo = typeof(ReflectHelperTests).GetMethod("IsAttributeDefinedShouldReturnFromCache"); @@ -190,26 +190,9 @@ public void IsAttributeDefinedShouldReturnFromCache() _testablePlatformServiceProvider.MockReflectionOperations.Verify(ro => ro.GetCustomAttributes(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } - public void IsAttributeDefinedShouldReturnTrueQueryingASpecificAttributesExistenceEvenIfGettingAllAttributesFail() - { - var rh = new ReflectHelper(new NotCachedReflectHelper()); - var mockMemberInfo = new Mock(); - var attributes = new Attribute[] { new UTF.TestMethodAttribute() }; - - _testablePlatformServiceProvider.MockReflectionOperations. - Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). - Returns((object[])null); - - _testablePlatformServiceProvider.MockReflectionOperations. - Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, typeof(UTF.TestMethodAttribute), true)). - Returns(attributes); - - Verify(rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); - } - public void HasAttributeDerivedFromShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMember() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -222,7 +205,7 @@ public void HasAttributeDerivedFromShouldReturnTrueIfSpecifiedAttributeIsDefined public void HasAttributeDerivedFromShouldReturnFalseIfSpecifiedAttributeIsNotDefinedOnAMember() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -235,7 +218,7 @@ public void HasAttributeDerivedFromShouldReturnFalseIfSpecifiedAttributeIsNotDef public void HasAttributeDerivedFromShouldReturnFromCache() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); // Not using mocks here because for some reason a dictionary match of the mock is not returning true in the product code. MethodInfo memberInfo = typeof(ReflectHelperTests).GetMethod("HasAttributeDerivedFromShouldReturnFromCache"); @@ -259,7 +242,7 @@ public void HasAttributeDerivedFromShouldReturnFromCache() public void HasAttributeDerivedFromShouldReturnFalseQueryingProvidedAttributesExistenceIfGettingAllAttributesFail() { - var rh = new ReflectHelper(new NotCachedReflectHelper()); + var rh = new ReflectHelper(); var mockMemberInfo = new Mock(); var attributes = new Attribute[] { new TestableExtendedTestMethod() }; @@ -273,23 +256,6 @@ public void HasAttributeDerivedFromShouldReturnFalseQueryingProvidedAttributesEx Verify(!rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); } - - public void HasAttributeDerivedFromShouldReturnTrueQueryingProvidedAttributesExistenceIfGettingAllAttributesFail() - { - var rh = new ReflectHelper(new NotCachedReflectHelper()); - var mockMemberInfo = new Mock(); - var attributes = new Attribute[] { new TestableExtendedTestMethod() }; - - _testablePlatformServiceProvider.MockReflectionOperations. - Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, true)). - Returns((object[])null); - - _testablePlatformServiceProvider.MockReflectionOperations. - Setup(ro => ro.GetCustomAttributes(mockMemberInfo.Object, typeof(TestableExtendedTestMethod), true)). - Returns(attributes); - - Verify(rh.IsNonDerivedAttributeDefined(mockMemberInfo.Object, true)); - } } #region Dummy Implementations diff --git a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs index 30d9b8d174..4c803c0d0d 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs @@ -12,44 +12,56 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.UnitTests.TestableIm /// internal sealed class TestableReflectHelper : ReflectHelper { - /// - /// A dictionary to hold mock custom attributes. The int represents a hash code of - /// the Type of custom attribute and the level its applied at : - /// MemberTypes.All for assembly level - /// MemberTypes.TypeInfo for class level - /// MemberTypes.Method for method level. - /// - private readonly Dictionary _customAttributes; - public TestableReflectHelper() : base(new TestableReflectionAccessor()) { - _customAttributes = []; } public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes memberTypes) { - // tODO: Add the info to ours; - var ours = (TestableReflectionAccessor)this.NotCachedReflectHelper; - - - int hashCode = type.FullName.GetHashCode() + memberTypes.GetHashCode(); - _customAttributes[hashCode] = _customAttributes.TryGetValue(hashCode, out Attribute[] value) - ? value.Concat(values).ToArray() - : values; + var attributeProvider = (TestableReflectionAccessor)NotCachedAttributes; + attributeProvider.AddData(type, values, memberTypes); } } -internal class TestableReflectionAccessor : INotCachedReflectHelper +internal class TestableReflectionAccessor : INotCachedAttributeHelper { - public TestableReflectionAccessor() - { - } - // TODO: fix to fix tests. - public object[] GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); + /// + /// A collection to hold mock custom attributes. + /// MemberTypes.All for assembly level + /// MemberTypes.TypeInfo for class level + /// MemberTypes.Method for method level. + /// + private readonly List<(Type Type, Attribute Attribute, MemberTypes MemberType)> _data = new(); - public TAttribute[] GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); + public object[] GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) + { + var foundAttributes = new List(); + foreach ((Type Type, Attribute Attribute, MemberTypes MemberType) attributeData in _data) + { + if (attributeProvider is MethodInfo && (attributeData.MemberType == MemberTypes.Method)) + { + foundAttributes.Add(attributeData.Attribute); + } + else if (attributeProvider is TypeInfo && (attributeData.MemberType == MemberTypes.TypeInfo)) + { + foundAttributes.Add(attributeData.Attribute); + } + else if (attributeProvider is Assembly && attributeData.MemberType == MemberTypes.All) + { + foundAttributes.Add(attributeData.Attribute); + } + } + + return foundAttributes.ToArray(); + } - public bool IsDerivedAttributeDefinedNotCached(ICustomAttributeProvider attributeProvider, bool inherit) => throw new NotImplementedException(); + internal void AddData(Type type, Attribute[] values, MemberTypes memberTypes) + { + foreach (Attribute attribute in values) + { + _data.Add((type, attribute, memberTypes)); + } + } } From 85af0bf93bfd4daed9d024dd1ba70fba7ec5dc53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Mon, 27 May 2024 18:50:25 +0200 Subject: [PATCH 08/20] Fix tests --- .../Discovery/AssemblyEnumerator.cs | 2 +- .../Discovery/TypeEnumerator.cs | 10 +++-- .../MSTest.TestAdapter/Execution/TypeCache.cs | 38 +++++++++---------- .../Extensions/MethodInfoExtensions.cs | 3 +- .../Helpers/ReflectHelper.cs | 7 ++++ .../Execution/TestMethodRunnerTests.cs | 11 ++++-- .../Execution/TypeCacheTests.cs | 12 +++--- 7 files changed, 49 insertions(+), 34 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index f90926a204..9e0e7538a6 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -263,7 +263,7 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet return false; } - if (test.TestMethod.DataType != DynamicDataType.ITestDataSource) + if (test.TestMethod.DataType == DynamicDataType.None) { return false; } diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 2fc6aba3cb..0dbb607c40 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -158,17 +158,21 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT // The foreach below could move to ReflectHelper, but this is only place where we want to check if // an attribute implements something different than an Attribute derived class. So instead we just // grab all attributes, and do the check ourselves. - bool isDataDriven = false; + DynamicDataType dynamicDataType = DynamicDataType.None; foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes(method, inherit: true)) { if (AttributeComparer.IsDerived(attribute)) { - isDataDriven = true; + dynamicDataType = DynamicDataType.ITestDataSource; break; } + else if (AttributeComparer.IsDerived(attribute)) + { + dynamicDataType = DynamicDataType.DataSourceAttribute; + } } - testMethod.DataType = isDataDriven ? DynamicDataType.ITestDataSource : DynamicDataType.None; + testMethod.DataType = dynamicDataType; var testElement = new UnitTestElement(testMethod) { diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index 60213f6878..45dee5a69f 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -306,17 +306,19 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) foreach (MethodInfo methodInfo in classType.GetTypeInfo().DeclaredMethods) { - if (methodInfo.IsPublic && !methodInfo.IsStatic) - { - // Update test initialize/cleanup method - UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, false, instanceMethods); - } + // TODO: should this be wrapped or the ifs in code below should be removed? + // if (methodInfo.IsPublic && !methodInfo.IsStatic) + // { + // Update test initialize/cleanup method + UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, false, instanceMethods); - if (methodInfo.IsPublic && methodInfo.IsStatic) - { - // Update class initialize/cleanup method - UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); - } + // } + // if (methodInfo.IsPublic && methodInfo.IsStatic) + // { + // Update class initialize/cleanup method + UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); + + // } } Type? baseType = classType.GetTypeInfo().BaseType; @@ -486,11 +488,10 @@ private bool IsAssemblyOrClassInitializeMethod(MethodInfo where TInitializeAttribute : Attribute { // TODO: can we? it will then never throw for invalid, this is inconsistent in the codebase. - //if (!methodInfo.IsStatic) - //{ + // if (!methodInfo.IsStatic) + // { // return false; - //} - + // } if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; @@ -514,12 +515,11 @@ private bool IsAssemblyOrClassInitializeMethod(MethodInfo private bool IsAssemblyOrClassCleanupMethod(MethodInfo methodInfo) where TCleanupAttribute : Attribute { - // TODO: can we? it will then never throw for invalid, this is inconsitent in the codebase. - //if (!methodInfo.IsStatic) - //{ + // TODO: can we? it will then never throw for invalid, this is inconsistent in the codebase. + // if (!methodInfo.IsStatic) + // { // return false; - //} - + // } if (!_reflectionHelper.IsNonDerivedAttributeDefined(methodInfo, false)) { return false; diff --git a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs index acc5e14d6c..e125064c79 100644 --- a/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs +++ b/src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs @@ -98,8 +98,7 @@ internal static bool HasCorrectTimeout(this MethodInfo method, TimeoutAttribute? // TODO: redesign this, probably change this to GetTimeout? so we don't have to do this weird dance? timeoutAttribute ??= ReflectHelper.Instance.GetFirstNonDerivedAttributeOrDefault(method, inherit: false); - // Timeout cannot be less than 0. - return !(timeoutAttribute?.Timeout < 0); + return timeoutAttribute?.Timeout > 0; } /// diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index bcb9e9ccb9..5b2230924d 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.Reflection; using System.Security; +using System.Xml.Serialization; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; @@ -544,4 +545,10 @@ internal class NotCachedAttributeHelper : INotCachedAttributeHelper return attributesArray; // TODO: Investigate if we rely on NRE } } + + internal void ClearCache() + { + _inheritedAttributeCache.Clear(); + _nonInheritedAttributeCache.Clear(); + } } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs index 827563c91f..8c47711976 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TestMethodRunnerTests.cs @@ -7,6 +7,7 @@ using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; +using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices; using Microsoft.VisualStudio.TestPlatform.MSTestAdapter.UnitTests.TestableImplementations; @@ -89,6 +90,8 @@ public TestMethodRunnerTests() _testablePlatformServiceProvider = new TestablePlatformServiceProvider(); _testablePlatformServiceProvider.SetupMockReflectionOperations(); PlatformServiceProvider.Instance = _testablePlatformServiceProvider; + + ReflectHelper.Instance.ClearCache(); } protected override void Dispose(bool disposing) @@ -306,7 +309,7 @@ public void RunTestMethodShouldRunDataDrivenTestsWhenDataIsProvidedUsingDataSour TestDataSource testDataSource = new(); // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); _testablePlatformServiceProvider.MockTestDataSource.Setup(tds => tds.GetData(testMethodInfo, _testContextImplementation)).Returns(new object[] { 1, 2, 3 }); UnitTestResult[] results = testMethodRunner.RunTestMethod(); @@ -352,7 +355,7 @@ public void RunTestMethodShouldSetDataRowIndexForDataDrivenTestsWhenDataIsProvid var attributes = new Attribute[] { dataSourceAttribute }; // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); _testablePlatformServiceProvider.MockTestDataSource.Setup(tds => tds.GetData(testMethodInfo, _testContextImplementation)).Returns(new object[] { 1, 2, 3 }); UnitTestResult[] results = testMethodRunner.RunTestMethod(); @@ -378,7 +381,7 @@ public void RunTestMethodShouldRunOnlyDataSourceTestsWhenBothDataSourceAndDataRo var attributes = new Attribute[] { dataSourceAttribute, dataRowAttribute }; // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); _testablePlatformServiceProvider.MockTestDataSource.Setup(tds => tds.GetData(testMethodInfo, _testContextImplementation)).Returns(new object[] { 1, 2, 3 }); UnitTestResult[] results = testMethodRunner.RunTestMethod(); @@ -454,7 +457,7 @@ public void RunTestMethodShouldSetResultFilesIfPresentForDataDrivenTests() var attributes = new Attribute[] { dataRowAttribute1, dataRowAttribute2 }; // Setup mocks - _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny(), It.IsAny())).Returns(attributes); + _testablePlatformServiceProvider.MockReflectionOperations.Setup(rf => rf.GetCustomAttributes(_methodInfo, It.IsAny())).Returns(attributes); UnitTestResult[] results = testMethodRunner.RunTestMethod(); Verify(results[0].ResultFiles.ToList().Contains("C:\\temp.txt")); diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs index ab5dd0dd98..b50c711510 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Execution/TypeCacheTests.cs @@ -39,6 +39,8 @@ public TypeCacheTests() _testablePlatformServiceProvider = new TestablePlatformServiceProvider(); PlatformServiceProvider.Instance = _testablePlatformServiceProvider; + ReflectHelper.Instance.ClearCache(); + SetupMocks(); } @@ -315,7 +317,7 @@ public void GetTestMethodInfoShouldThrowIfAssemblyInitHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); @@ -347,7 +349,7 @@ public void GetTestMethodInfoShouldThrowIfAssemblyCleanupHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type.GetTypeInfo(), true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); @@ -651,7 +653,7 @@ public void GetTestMethodInfoShouldThrowIfClassInitHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyInit"), false)).Returns(true); @@ -683,7 +685,7 @@ public void GetTestMethodInfoShouldThrowIfClassCleanupHasIncorrectSignature() var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("AssemblyCleanup"), false)).Returns(true); @@ -755,7 +757,7 @@ public void GetTestMethodInfoShouldThrowIfTestInitOrCleanupHasIncorrectSignature var testMethod = new TestMethod("M", type.FullName, "A", isAsync: false); _mockReflectHelper.Setup( - rh => rh.IsNonDerivedAttributeDefined(type, true)).Returns(true); + rh => rh.IsDerivedAttributeDefined(type, true)).Returns(true); _mockReflectHelper.Setup( rh => rh.IsNonDerivedAttributeDefined(type.GetMethod("TestInit"), false)).Returns(true); From 5d61729cf6a5536d4dc619d0086ac89f6cc04e74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Mon, 27 May 2024 19:03:18 +0200 Subject: [PATCH 09/20] Remove or annotate todos --- .../Discovery/AssemblyEnumerator.cs | 2 +- .../MSTest.TestAdapter/Execution/TypeCache.cs | 7 ++-- .../Helpers/ReflectHelper.cs | 39 +++---------------- 3 files changed, 11 insertions(+), 37 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 9e0e7538a6..5de2eb91d8 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -310,7 +310,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth } catch (Exception ex) when (ex is ArgumentException && MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive) { - var discoveredTest = test.Clone(); + UnitTestElement discoveredTest = test.Clone(); discoveredTest.DisplayName = dataSource.GetDisplayName(methodInfo, null) ?? discoveredTest.DisplayName; tests.Add(discoveredTest); continue; diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index 45dee5a69f..42b3a91bed 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -306,7 +306,7 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) foreach (MethodInfo methodInfo in classType.GetTypeInfo().DeclaredMethods) { - // TODO: should this be wrapped or the ifs in code below should be removed? + // TODO: should this be wrapped or the ifs in code below should be removed? issue: https://github.com/microsoft/testfx/issues/2999 // if (methodInfo.IsPublic && !methodInfo.IsStatic) // { // Update test initialize/cleanup method @@ -328,6 +328,7 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) { foreach (MethodInfo methodInfo in baseType.GetTypeInfo().DeclaredMethods) { + // TODO: this is inconsistent, normally we inspect all methods for attributes and throw when they have incorrect shape, issue: https://github.com/microsoft/testfx/issues/2999 if (methodInfo.IsPublic && !methodInfo.IsStatic) { // Update test initialize/cleanup method from base type. @@ -487,7 +488,7 @@ private TestAssemblyInfo GetAssemblyInfo(Type type) private bool IsAssemblyOrClassInitializeMethod(MethodInfo methodInfo) where TInitializeAttribute : Attribute { - // TODO: can we? it will then never throw for invalid, this is inconsistent in the codebase. + // TODO: this would be inconsistent with the codebase, but potential perf gain, issue: https://github.com/microsoft/testfx/issues/2999 // if (!methodInfo.IsStatic) // { // return false; @@ -515,7 +516,7 @@ private bool IsAssemblyOrClassInitializeMethod(MethodInfo private bool IsAssemblyOrClassCleanupMethod(MethodInfo methodInfo) where TCleanupAttribute : Attribute { - // TODO: can we? it will then never throw for invalid, this is inconsistent in the codebase. + // TODO: this would be inconsistent with the codebase, but potential perf gain, issue: https://github.com/microsoft/testfx/issues/2999 // if (!methodInfo.IsStatic) // { // return false; diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index 5b2230924d..96c88b9398 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -5,7 +5,6 @@ using System.Globalization; using System.Reflection; using System.Security; -using System.Xml.Serialization; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; @@ -93,7 +92,7 @@ public virtual bool IsDerivedAttributeDefined(Type type, bool inheri /// Checks to see if a member or type is decorated with the given attribute, or an attribute that derives from it. e.g. [MyTestClass] from [TestClass] will match if you look for [TestClass]. The inherit parameter does not impact this checking. /// /// Attribute to search for. - /// Type to test. + /// Member to inspect for attributes. /// Inspect inheritance chain of the member or class. E.g. if parent class has this attribute defined. /// True if the attribute of the specified type is defined on this member or a class. public virtual /* for testing */ bool IsDerivedAttributeDefined(MemberInfo memberInfo, bool inherit) @@ -106,19 +105,6 @@ public virtual bool IsDerivedAttributeDefined(Type type, bool inheri // Get all attributes on the member. Attribute[] attributes = GetCustomAttributesCached(memberInfo, inherit); - if (attributes == null) - { - // TODO: - bool a = true; - if (a) - { - throw new NotSupportedException("THIS FALLBACK!"); - } - - PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning($"{nameof(ReflectHelper)}.{nameof(GetCustomAttributesCached)}: {Resource.FailedFetchAttributeCache}"); - - return IsNonDerivedAttributeDefined(memberInfo, inherit); - } // Try to find the attribute that is derived from baseAttrType. foreach (Attribute attribute in attributes) @@ -166,13 +152,7 @@ public virtual bool IsDerivedAttributeDefined(Type type, bool inheri throw new TypeInspectionException(errorMessage); } - // TODO: we can probably do better if we grab the enumerator? - if (expectedException == null) - { - return null; - } - - return expectedException; + return expectedException ?? null; } /// @@ -397,7 +377,6 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro } } - /// /// Get attribute defined on a method which is of given type of subtype of given type. /// @@ -434,8 +413,6 @@ internal virtual IEnumerable GetTestPropertiesAsTraits(MemberInfo testPro return ownerAttribute?.Owner; } - - /// /// Gets and caches the attributes for the given type, or method. /// @@ -489,12 +466,7 @@ Attribute[] GetOrAddAttributes(Dictionary PlatformServiceProvider.Instance.AdapterTraceLogger.LogWarning(Resource.FailedToGetCustomAttribute, attributeProvider.GetType().FullName!, description); - // Since we cannot check by attribute names, do it in reflection way. - // Note 1: this will not work for different version of assembly but it is better than nothing. - // Note 2: we cannot cache this because we don't know if there are other attributes defined. - - // TODO: handle this instead polluting the api with null check, that we don't handle correctly in most places. This path is already expensive and this way we can at least keep it unified. - return null; + return Array.Empty(); } DebugEx.Assert(attributes != null, "attributes should not be null."); @@ -525,7 +497,7 @@ Attribute[] GetOrAddAttributes(Dictionary internal class NotCachedAttributeHelper : INotCachedAttributeHelper { /// - /// Get custom attributes on a member without cache. + /// Get custom attributes on a member without cache. /// /// Member for which attributes needs to be retrieved. /// If inherited type of attribute. @@ -546,8 +518,9 @@ internal class NotCachedAttributeHelper : INotCachedAttributeHelper } } - internal void ClearCache() + internal /* for tests */ void ClearCache() { + // Tests manipulate the platform reflection provider, and we end up caching different attributes than the class / method actually has. _inheritedAttributeCache.Clear(); _nonInheritedAttributeCache.Clear(); } From ffd0212fb27ee1f19c1551ff58422c6ef9d0f6d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Mon, 27 May 2024 19:20:32 +0200 Subject: [PATCH 10/20] fix formatting --- .../Discovery/TestMethodValidator.cs | 1 - .../Execution/KnownNonTestMethods.cs | 33 ------------------- .../TestableReflectHelper.cs | 1 - 3 files changed, 35 deletions(-) delete mode 100644 src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs index 855d25cebe..729abe733d 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs @@ -4,7 +4,6 @@ using System.Globalization; using System.Reflection; -using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Extensions; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; using Microsoft.VisualStudio.TestTools.UnitTesting; diff --git a/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs b/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs deleted file mode 100644 index 25acb413f0..0000000000 --- a/src/Adapter/MSTest.TestAdapter/Execution/KnownNonTestMethods.cs +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Execution; - -internal static class KnownNonTestMethods -{ - private static readonly string[] MethodNames = [ - nameof(Equals), - nameof(GetHashCode), - nameof(GetType), - nameof(ReferenceEquals), - nameof(ToString) - ]; - - public static bool Contains(string methodName) - { - if (methodName == "Equals") - { - Console.WriteLine($"Known non test methods called with: {methodName}"); - Console.WriteLine(Environment.StackTrace); - } - foreach (string method in MethodNames) - { - if (string.Equals(method, methodName, StringComparison.Ordinal)) - { - return true; - } - } - - return false; - } -} diff --git a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs index 4c803c0d0d..a966421f79 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs @@ -26,7 +26,6 @@ public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes member internal class TestableReflectionAccessor : INotCachedAttributeHelper { - /// /// A collection to hold mock custom attributes. /// MemberTypes.All for assembly level From cadd8eb10d674058579436c1e460afea1f6f36da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 11:03:48 +0200 Subject: [PATCH 11/20] Fixes --- .../Discovery/AssemblyEnumerator.cs | 8 +++---- .../Discovery/TypeEnumerator.cs | 1 + .../Helpers/AttributeComparer.cs | 17 +++++++++++-- .../Services/FileOperations.cs | 24 +++++-------------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 7b4a85b831..8c1938cbd4 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -12,8 +12,6 @@ using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; using Microsoft.VisualStudio.TestTools.UnitTesting; -using FrameworkITestDataSource = Microsoft.VisualStudio.TestTools.UnitTesting.ITestDataSource; - namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery; /// @@ -277,7 +275,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth // We don't have a special method to filter attributes that are not derived from Attribute, so we take all // attributes and filter them. We don't have to care if there is one, because this method is only entered when // there is at least one (we determine this in TypeEnumerator.GetTestFromMethod. - IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false).OfType(); + IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false).OfType(); try { @@ -291,10 +289,10 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth } } - private static bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodInfo, IEnumerable testDataSources, + private static bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodInfo, IEnumerable testDataSources, List tests) { - foreach (FrameworkITestDataSource dataSource in testDataSources) + foreach (ITestDataSource dataSource in testDataSources) { IEnumerable? data; diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 6f1929fa81..9750965131 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -169,6 +169,7 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT else if (AttributeComparer.IsDerived(attribute)) { dynamicDataType = DynamicDataType.DataSourceAttribute; + break; } } diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs index 7d364bbabd..a6e757e27c 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs @@ -11,8 +11,21 @@ internal class AttributeComparer public static bool IsDerived(Attribute attribute) { Type attributeType = attribute.GetType(); + // IsSubclassOf returns false when the types are equal. - return attributeType == typeof(TAttribute) - || attributeType.IsSubclassOf(typeof(TAttribute)); + if (attributeType == typeof(TAttribute)) + { + return true; + } + + // IsAssignableFrom also does this internally, but later falls to check generic + // and we don't need that. + if (!typeof(TAttribute).IsInterface) + { + // This returns false when TAttribute is interface (like ITestDataSource). + return attributeType.IsSubclassOf(typeof(TAttribute)); + } + + return typeof(TAttribute).IsAssignableFrom(attributeType); } } diff --git a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs index c4ff1a9b96..46275bb72f 100644 --- a/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.cs +++ b/src/Adapter/MSTestAdapter.PlatformServices/Services/FileOperations.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 System.Collections.Concurrent; using System.Reflection; #if WIN_UI @@ -19,7 +20,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices; /// public class FileOperations : IFileOperations { - private readonly Dictionary _assemblyCache = new(); + private readonly ConcurrentDictionary _assemblyCache = new(); #if WIN_UI private readonly bool _isPackaged; @@ -47,15 +48,9 @@ public Assembly LoadAssembly(string assemblyName, bool isReflectionOnly) } #endif string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(assemblyName); - if (_assemblyCache.TryGetValue(fileNameWithoutExtension, out Assembly? assembly)) - { - return assembly; - } + Assembly assembly = _assemblyCache.GetOrAdd(fileNameWithoutExtension, fileNameWithoutExtension => Assembly.Load(new AssemblyName(fileNameWithoutExtension))); - var asm = Assembly.Load(new AssemblyName(fileNameWithoutExtension)); - _assemblyCache.Add(fileNameWithoutExtension, asm); - - return asm; + return assembly; #elif NETFRAMEWORK if (isReflectionOnly) { @@ -63,15 +58,8 @@ public Assembly LoadAssembly(string assemblyName, bool isReflectionOnly) } else { - if (_assemblyCache.TryGetValue(assemblyName, out Assembly? assembly)) - { - return assembly; - } - - var asm = Assembly.LoadFrom(assemblyName); - _assemblyCache.Add(assemblyName, asm); - - return asm; + Assembly assembly = _assemblyCache.GetOrAdd(assemblyName, Assembly.LoadFrom); + return assembly; } #endif } From f58e4a07d2423ce4710590c76e259177a9a361a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 11:26:55 +0200 Subject: [PATCH 12/20] cleanup --- .../MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs | 9 +++++---- .../Discovery/TestMethodValidatorTests.cs | 1 - 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 8c1938cbd4..ac3c80b30c 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -12,6 +12,8 @@ using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.ObjectModel; using Microsoft.VisualStudio.TestTools.UnitTesting; +using FrameworkITestDataSource = Microsoft.VisualStudio.TestTools.UnitTesting.ITestDataSource; + namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery; /// @@ -214,7 +216,6 @@ internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFile typeFullName = type.FullName; TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals, testIdGenerationStrategy); ICollection? unitTestCases = testTypeEnumerator.Enumerate(out ICollection warningsFromTypeEnumerator); - bool typeIgnored = ReflectHelper.IsNonDerivedAttributeDefined(type, false); warningMessages.AddRange(warningsFromTypeEnumerator); if (unitTestCases != null) @@ -275,7 +276,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth // We don't have a special method to filter attributes that are not derived from Attribute, so we take all // attributes and filter them. We don't have to care if there is one, because this method is only entered when // there is at least one (we determine this in TypeEnumerator.GetTestFromMethod. - IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false).OfType(); + IEnumerable? testDataSources = ReflectHelper.Instance.GetDerivedAttributes(methodInfo, inherit: false).OfType(); try { @@ -289,10 +290,10 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth } } - private static bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodInfo, IEnumerable testDataSources, + private static bool ProcessTestDataSourceTests(UnitTestElement test, MethodInfo methodInfo, IEnumerable testDataSources, List tests) { - foreach (ITestDataSource dataSource in testDataSources) + foreach (FrameworkITestDataSource dataSource in testDataSources) { IEnumerable? data; diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs index 6ab7a4ff1b..e7c3a59f05 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs @@ -57,7 +57,6 @@ public void IsValidTestMethodShouldReportWarningsForGenericTestMethodDefinitions { SetupTestMethod(); - // _mockMethodInfo.Setup(mi => mi.Module.Assembly.ReflectionOnly).Returns(false); _mockMethodInfo.Setup(mi => mi.IsGenericMethodDefinition).Returns(true); _mockMethodInfo.Setup(mi => mi.DeclaringType.FullName).Returns("DummyTestClass"); _mockMethodInfo.Setup(mi => mi.Name).Returns("DummyTestMethod"); From 7b2911ea97d779ca4aae61e1124a09c9f362e234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 13:20:58 +0200 Subject: [PATCH 13/20] Make test non-data driven when we allow no-data --- src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index ac3c80b30c..ed7fad6c6f 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -311,6 +311,8 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth catch (Exception ex) when (ex is ArgumentException && 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; From 22ab61a205265b4ecc9110f083b90d66d02bb838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 14:40:55 +0200 Subject: [PATCH 14/20] Make test non-data driven when we allow no-data --- .../MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index ed7fad6c6f..2d87bf6dd2 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -260,6 +260,11 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet return false; } + // REVIEW: We started setting this value to that actual DataType earlier so we can skip this expensive code + // but some of the code after is expecting this property to stay NONE (acceptance tests fail if you don't do this + // so we reset is here to None. + test.TestMethod.DataType = DynamicDataType.None; + // NOTE: From this place we don't have any path that would let the user write a message on the TestContext and we don't do // anything with what would be printed anyway so we can simply use a simple StringWriter. using var writer = new StringWriter(); From 9bb1fefcdadad517c9eda86a0fe67979f26070eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 18:32:40 +0200 Subject: [PATCH 15/20] only set it when strategy is set to during discovery --- .../Discovery/AssemblyEnumerator.cs | 15 +++-- .../Discovery/TypeEnumerator.cs | 56 +++++++++++-------- .../Helpers/ReflectHelper.cs | 2 +- .../Discovery/AssemblyEnumeratorTests.cs | 3 +- .../Discovery/TypeEnumeratorTests.cs | 2 + 5 files changed, 47 insertions(+), 31 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 2d87bf6dd2..4568acd157 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -188,14 +188,15 @@ internal static string GetLoadExceptionDetails(ReflectionTypeLoadException ex) /// The reflected assembly name. /// True to discover test classes which are declared internal in /// addition to test classes which are declared public. + /// to use when generating tests. /// to use when generating TestId. /// a TypeEnumerator instance. - internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFileName, bool discoverInternals, TestIdGenerationStrategy testIdGenerationStrategy) + internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFileName, bool discoverInternals, TestDataSourceDiscoveryOption discoveryOption, TestIdGenerationStrategy testIdGenerationStrategy) { var typeValidator = new TypeValidator(ReflectHelper, discoverInternals); var testMethodValidator = new TestMethodValidator(ReflectHelper, discoverInternals); - return new TypeEnumerator(type, assemblyFileName, ReflectHelper, typeValidator, testMethodValidator, testIdGenerationStrategy); + return new TypeEnumerator(type, assemblyFileName, ReflectHelper, typeValidator, testMethodValidator, discoveryOption, testIdGenerationStrategy); } private List DiscoverTestsInType(string assemblyFileName, string? runSettingsXml, Type type, @@ -214,7 +215,7 @@ internal virtual TypeEnumerator GetTypeEnumerator(Type type, string assemblyFile try { typeFullName = type.FullName; - TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals, testIdGenerationStrategy); + TypeEnumerator testTypeEnumerator = GetTypeEnumerator(type, assemblyFileName, discoverInternals, discoveryOption, testIdGenerationStrategy); ICollection? unitTestCases = testTypeEnumerator.Enumerate(out ICollection warningsFromTypeEnumerator); warningMessages.AddRange(warningsFromTypeEnumerator); @@ -260,9 +261,11 @@ private bool DynamicDataAttached(IDictionary sourceLevelParamet return false; } - // REVIEW: We started setting this value to that actual DataType earlier so we can skip this expensive code - // but some of the code after is expecting this property to stay NONE (acceptance tests fail if you don't do this - // so we reset is here to None. + // PERF: For perf we started setting DataType in TypeEnumerator, so when it is None we will not reach this line. + // But if we do run this code, we still reset it to None, because the code that determines if this is data drive test expects the value to be None + // and only sets it when needed. + // + // If you remove this line and acceptance tests still pass you are okay. test.TestMethod.DataType = DynamicDataType.None; // NOTE: From this place we don't have any path that would let the user write a message on the TestContext and we don't do diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs index 9750965131..e0fab8b22e 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeEnumerator.cs @@ -22,6 +22,7 @@ internal class TypeEnumerator private readonly TypeValidator _typeValidator; private readonly TestMethodValidator _testMethodValidator; private readonly TestIdGenerationStrategy _testIdGenerationStrategy; + private readonly TestDataSourceDiscoveryOption _discoveryOption; private readonly ReflectHelper _reflectHelper; /// @@ -33,7 +34,7 @@ internal class TypeEnumerator /// The validator for test classes. /// The validator for test methods. /// to use when generating TestId. - internal TypeEnumerator(Type type, string assemblyFilePath, ReflectHelper reflectHelper, TypeValidator typeValidator, TestMethodValidator testMethodValidator, TestIdGenerationStrategy testIdGenerationStrategy) + internal TypeEnumerator(Type type, string assemblyFilePath, ReflectHelper reflectHelper, TypeValidator typeValidator, TestMethodValidator testMethodValidator, TestDataSourceDiscoveryOption discoveryOption, TestIdGenerationStrategy testIdGenerationStrategy) { _type = type; _assemblyFilePath = assemblyFilePath; @@ -41,6 +42,7 @@ internal TypeEnumerator(Type type, string assemblyFilePath, ReflectHelper reflec _typeValidator = typeValidator; _testMethodValidator = testMethodValidator; _testIdGenerationStrategy = testIdGenerationStrategy; + _discoveryOption = discoveryOption; } /// @@ -149,32 +151,22 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT method.DeclaringType.Assembly); } - // PERF: AssemblyEnumerator would later try to figure if this is a data driven test method - // by creating a TestMethodInfo. This is really expensive for discovery, and should be done only - // when we are a data driven test. - // Instead we check if we have any data driven attribute here, and skip creating TestMethod info - // in AssemblyEnumerator. + // PERF: When discovery option is set to DuringDiscovery, we will expand data on tests to one test case + // per data item. This will happen in AssemblyEnumerator. But AssemblyEnumerator does not have direct access to + // the method info or method attributes, so it would create a TestMethodInfo to see if the test is data driven. + // Creating TestMethodInfo is expensive and should be done only for a test that we know is data driven. // - // The foreach below could move to ReflectHelper, but this is only place where we want to check if - // an attribute implements something different than an Attribute derived class. So instead we just - // grab all attributes, and do the check ourselves. - DynamicDataType dynamicDataType = DynamicDataType.None; - foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes(method, inherit: true)) + // So to optimize this we check if we have some data source attribute. Because here we have access to all attributes + // and we store that info in DataType. AssemblyEnumerator will pick this up and will get the real test data in the expensive way + // or it will skip over getting the data cheaply, when DataType = DynamicDataType.None. + // + // This needs to be done only when DuringDiscovery is set, because otherwise we would populate the DataType, but we would not populate + // and execution would not try to re-populate the data, because DataType is already set to data driven, so it would just throw error about empty data. + if (_discoveryOption == TestDataSourceDiscoveryOption.DuringDiscovery) { - if (AttributeComparer.IsDerived(attribute)) - { - dynamicDataType = DynamicDataType.ITestDataSource; - break; - } - else if (AttributeComparer.IsDerived(attribute)) - { - dynamicDataType = DynamicDataType.DataSourceAttribute; - break; - } + testMethod.DataType = GetDynamicDataType(method); } - testMethod.DataType = dynamicDataType; - var testElement = new UnitTestElement(testMethod) { // Get compiler generated type name for async test method (either void returning or task returning). @@ -229,4 +221,22 @@ internal UnitTestElement GetTestFromMethod(MethodInfo method, bool isDeclaredInT return testElement; } + + private DynamicDataType GetDynamicDataType(MethodInfo method) + { + foreach (Attribute attribute in _reflectHelper.GetDerivedAttributes(method, inherit: true)) + { + if (AttributeComparer.IsDerived(attribute)) + { + return DynamicDataType.ITestDataSource; + } + + if (AttributeComparer.IsDerived(attribute)) + { + return DynamicDataType.DataSourceAttribute; + } + } + + return DynamicDataType.None; + } } diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index b25fce9571..d71eba911e 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -244,7 +244,7 @@ internal virtual bool IsMethodDeclaredInSameAssemblyAsType(MethodInfo method, Ty /// The member to inspect. /// The reflected type that owns . /// Categories defined. - internal virtual string[] GetTestCategories(MemberInfo categoryAttributeProvider, Type owningType) + internal string[] GetTestCategories(MemberInfo categoryAttributeProvider, Type owningType) { IEnumerable? methodCategories = GetDerivedAttributes(categoryAttributeProvider, inherit: true); IEnumerable? typeCategories = GetDerivedAttributes(owningType, inherit: true); diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/AssemblyEnumeratorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/AssemblyEnumeratorTests.cs index 5e93dd113c..cfe6880287 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/AssemblyEnumeratorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/AssemblyEnumeratorTests.cs @@ -411,13 +411,14 @@ internal TestableAssemblyEnumerator() reflectHelper.Object, typeValidator.Object, testMethodValidator.Object, + TestDataSourceDiscoveryOption.DuringExecution, TestIdGenerationStrategy.FullyQualified); } internal Mock MockTypeEnumerator { get; set; } internal override TypeEnumerator GetTypeEnumerator(Type type, string assemblyFileName, bool discoverInternals, - TestIdGenerationStrategy testIdGenerationStrategy) + TestDataSourceDiscoveryOption discoveryOption, TestIdGenerationStrategy testIdGenerationStrategy) => MockTypeEnumerator.Object; } diff --git a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs index 0f39bf46fa..74fabf1e32 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/Discovery/TypeEnumeratorTests.cs @@ -564,6 +564,7 @@ private void SetupTestClassAndTestMethods(bool isValidTestClass, bool isValidTes } private TypeEnumerator GetTypeEnumeratorInstance(Type type, string assemblyName, + TestDataSourceDiscoveryOption discoveryOption = TestDataSourceDiscoveryOption.DuringExecution, TestIdGenerationStrategy idGenerationStrategy = TestIdGenerationStrategy.FullyQualified) => new( type, @@ -571,6 +572,7 @@ private void SetupTestClassAndTestMethods(bool isValidTestClass, bool isValidTes _mockReflectHelper.Object, _mockTypeValidator.Object, _mockTestMethodValidator.Object, + discoveryOption, idGenerationStrategy); #endregion From cbb2767d592c8f74dbe510c8a1ffce0405999949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 18:51:43 +0200 Subject: [PATCH 16/20] fixes --- .../MSTest.TestAdapter/Execution/TypeCache.cs | 12 --------- ...per.cs => INotCachedReflectionAccessor.cs} | 5 +++- .../Helpers/ReflectHelper.cs | 26 ++++++++----------- .../TestableReflectHelper.cs | 2 +- 4 files changed, 16 insertions(+), 29 deletions(-) rename src/Adapter/MSTest.TestAdapter/Helpers/{INotCachedAttributeHelper.cs => INotCachedReflectionAccessor.cs} (65%) diff --git a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs index 9d19d2d1f9..5798158cf2 100644 --- a/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs +++ b/src/Adapter/MSTest.TestAdapter/Execution/TypeCache.cs @@ -298,19 +298,9 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) foreach (MethodInfo methodInfo in classType.GetTypeInfo().DeclaredMethods) { - // TODO: should this be wrapped or the ifs in code below should be removed? issue: https://github.com/microsoft/testfx/issues/2999 - // if (methodInfo.IsPublic && !methodInfo.IsStatic) - // { - // Update test initialize/cleanup method UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, false, instanceMethods); - // } - // if (methodInfo.IsPublic && methodInfo.IsStatic) - // { - // Update class initialize/cleanup method UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, false, ref initAndCleanupMethods); - - // } } Type? baseType = classType.BaseType; @@ -319,14 +309,12 @@ private TestClassInfo CreateClassInfo(Type classType, TestMethod testMethod) { foreach (MethodInfo methodInfo in baseType.GetTypeInfo().DeclaredMethods) { - // TODO: this is inconsistent, normally we inspect all methods for attributes and throw when they have incorrect shape, issue: https://github.com/microsoft/testfx/issues/2999 if (methodInfo is { IsPublic: true, IsStatic: false }) { // Update test initialize/cleanup method from base type. UpdateInfoIfTestInitializeOrCleanupMethod(classInfo, methodInfo, true, instanceMethods); } - // TODO: this is inconsistent, normally we inspect all methods for attributes and throw when they have incorrect shape, issue: https://github.com/microsoft/testfx/issues/2999 if (methodInfo is { IsPublic: true, IsStatic: true }) { UpdateInfoIfClassInitializeOrCleanupMethod(classInfo, methodInfo, true, ref initAndCleanupMethods); diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedReflectionAccessor.cs similarity index 65% rename from src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs rename to src/Adapter/MSTest.TestAdapter/Helpers/INotCachedReflectionAccessor.cs index 0795da4019..12604078f3 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedAttributeHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/INotCachedReflectionAccessor.cs @@ -5,7 +5,10 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; -internal interface INotCachedAttributeHelper +/// +/// This interface is for unit tests tests so they can easily replace the implementation of accessing the attributes. +/// +internal interface INotCachedReflectionAccessor { object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit); } diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index d71eba911e..ab76969047 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -1,7 +1,6 @@ // 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.Diagnostics.CodeAnalysis; using System.Globalization; using System.Reflection; using System.Security; @@ -15,7 +14,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; internal class ReflectHelper : MarshalByRefObject { - private static readonly Lazy InstanceValue = new(() => new ReflectHelper(new NotCachedAttributeHelper())); + private static readonly Lazy InstanceValue = new(() => new ReflectHelper(new NotCachedReflectionAccessor())); // PERF: This was moved from Dictionary> to Dictionary // this allows us to store multiple attributes of the same type if we find them. It also has lower memory footprint, and is faster @@ -23,11 +22,11 @@ internal class ReflectHelper : MarshalByRefObject private readonly Dictionary _inheritedAttributeCache = []; private readonly Dictionary _nonInheritedAttributeCache = []; - internal /* for tests only */ ReflectHelper(INotCachedAttributeHelper notCachedAttributeHelper) => - NotCachedAttributes = notCachedAttributeHelper ?? new NotCachedAttributeHelper(); + internal /* for tests only */ ReflectHelper(INotCachedReflectionAccessor notCachedAttributeHelper) => + NotCachedAttributes = notCachedAttributeHelper ?? new NotCachedReflectionAccessor(); internal /* for tests only, because of Moq */ ReflectHelper() - : this(new NotCachedAttributeHelper()) + : this(new NotCachedReflectionAccessor()) { } @@ -35,7 +34,7 @@ internal class ReflectHelper : MarshalByRefObject public static ReflectHelper Instance => InstanceValue.Value; - public INotCachedAttributeHelper NotCachedAttributes { get; } + public INotCachedReflectionAccessor NotCachedAttributes { get; } /// /// Checks to see if a member or type is decorated with the given attribute. The type is checked exactly. If attribute is derived (inherits from) a class, e.g. [MyTestClass] from [TestClass] it won't match if you look for [TestClass]. The inherit parameter does not impact this checking. @@ -225,9 +224,12 @@ public virtual bool IsDerivedAttributeDefined(Type type, bool inheri /// The method to inspect. /// The return type to match. /// True if there is a match. - internal static bool MatchReturnType(MethodInfo method, Type returnType) => method == null + internal static bool MatchReturnType(MethodInfo method, Type returnType) => + method == null ? throw new ArgumentNullException(nameof(method)) - : returnType == null ? throw new ArgumentNullException(nameof(returnType)) : method.ReturnType.Equals(returnType); + : returnType == null + ? throw new ArgumentNullException(nameof(returnType)) + : method.ReturnType.Equals(returnType); /// /// Returns true when the method is declared in the assembly where the type is declared. @@ -492,7 +494,7 @@ Attribute[] GetOrAddAttributes(Dictionary /// /// Reflection helper that is accessing Reflection directly, and won't cache the results. /// - internal class NotCachedAttributeHelper : INotCachedAttributeHelper + internal class NotCachedReflectionAccessor : INotCachedReflectionAccessor { /// /// Get custom attributes on a member without cache. @@ -500,14 +502,8 @@ internal class NotCachedAttributeHelper : INotCachedAttributeHelper /// Member for which attributes needs to be retrieved. /// If inherited type of attribute. /// All attributes of give type on member. - [return: NotNullIfNotNull(nameof(attributeProvider))] public object[]? GetCustomAttributesNotCached(ICustomAttributeProvider attributeProvider, bool inherit) { - if (attributeProvider == null) - { - return null; - } - object[] attributesArray = attributeProvider is MemberInfo memberInfo ? PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes(memberInfo, inherit) : PlatformServiceProvider.Instance.ReflectionOperations.GetCustomAttributes((Assembly)attributeProvider, typeof(Attribute)); diff --git a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs index a966421f79..5679d5692d 100644 --- a/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs +++ b/test/UnitTests/MSTestAdapter.UnitTests/TestableImplementations/TestableReflectHelper.cs @@ -24,7 +24,7 @@ public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes member } } -internal class TestableReflectionAccessor : INotCachedAttributeHelper +internal class TestableReflectionAccessor : INotCachedReflectionAccessor { /// /// A collection to hold mock custom attributes. From 9808be757d4d09ca8e5d69e4e5ee5ba3a7dce759 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 18:53:08 +0200 Subject: [PATCH 17/20] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Amaury Levé --- src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs | 2 +- .../MSTest.TestAdapter/Discovery/TestMethodValidator.cs | 2 +- src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs | 2 +- src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs | 2 +- src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj | 3 --- 5 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 4568acd157..96e30a76d0 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -316,7 +316,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, FrameworkMessages.DynamicDataIEnumerableEmpty, "GetData", dataSource.GetType().Name)); } } - catch (Exception ex) when (ex is ArgumentException && MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive) + catch (ArgumentException aex) when (MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive) { UnitTestElement discoveredTest = test.Clone(); // Make the test not data driven, because it had no data. diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs index 729abe733d..861d108ab4 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs @@ -48,7 +48,7 @@ internal TestMethodValidator(ReflectHelper reflectHelper, bool discoverInternals /// Return true if a method is a valid test method. internal virtual bool IsValidTestMethod(MethodInfo testMethodInfo, Type type, ICollection warnings) { - // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every method info in the + // PERF: We are doing caching reflection here, meaning we will cache every method info in the // assembly, this is because when we discover and run we will repeatedly inspect all the methods in the assembly, and this // gives us a better performance. // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run, diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs index a36e9da76e..edea385520 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/TypeValidator.cs @@ -51,7 +51,7 @@ internal virtual bool IsValidTestClass(Type type, ICollection warnings) { TypeInfo typeInfo = type.GetTypeInfo(); - // PERF: Contrary to my intuition, we are doing caching reflection here, meaning we will cache every class info in the + // PERF: We are doing caching reflection here, meaning we will cache every class info in the // assembly, this is because when we discover and run we will repeatedly inspect all the types in the assembly, and this // gives us a better performance. // It would be possible to use non-caching reflection here if we knew that we are only doing discovery that won't be followed by run, diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs index a6e757e27c..5736b8295b 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/AttributeComparer.cs @@ -3,7 +3,7 @@ namespace Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Helpers; -internal class AttributeComparer +internal sealed class AttributeComparer { public static bool IsNonDerived(Attribute attribute) => attribute is TAttribute; diff --git a/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj b/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj index c33e8b6561..067bd24771 100644 --- a/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj +++ b/src/Adapter/MSTest.TestAdapter/MSTest.TestAdapter.csproj @@ -97,9 +97,6 @@ - - - From 2ddc8cd29713de601f06ca154da8ac985eb413d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Tue, 28 May 2024 18:56:54 +0200 Subject: [PATCH 18/20] Add virtual --- src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs index ab76969047..80664866aa 100644 --- a/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs +++ b/src/Adapter/MSTest.TestAdapter/Helpers/ReflectHelper.cs @@ -246,7 +246,7 @@ internal virtual bool IsMethodDeclaredInSameAssemblyAsType(MethodInfo method, Ty /// The member to inspect. /// The reflected type that owns . /// Categories defined. - internal string[] GetTestCategories(MemberInfo categoryAttributeProvider, Type owningType) + internal virtual /* for tests, we are mocking this */ string[] GetTestCategories(MemberInfo categoryAttributeProvider, Type owningType) { IEnumerable? methodCategories = GetDerivedAttributes(categoryAttributeProvider, inherit: true); IEnumerable? typeCategories = GetDerivedAttributes(owningType, inherit: true); From f23e83d8e2b74239c5a7f42b2904ca26666028de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 30 May 2024 07:53:57 +0200 Subject: [PATCH 19/20] Remove unused variable --- src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs index 96e30a76d0..4fca769971 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/AssemblyEnumerator.cs @@ -316,7 +316,7 @@ private static bool TryProcessTestDataSourceTests(UnitTestElement test, TestMeth throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, FrameworkMessages.DynamicDataIEnumerableEmpty, "GetData", dataSource.GetType().Name)); } } - catch (ArgumentException aex) when (MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive) + catch (ArgumentException) when (MSTestSettings.CurrentSettings.ConsiderEmptyDataSourceAsInconclusive) { UnitTestElement discoveredTest = test.Clone(); // Make the test not data driven, because it had no data. From 000f622b7d222efdb685e8864377f71b488e3709 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Jare=C5=A1?= Date: Thu, 30 May 2024 08:10:58 +0200 Subject: [PATCH 20/20] Revert outputs in discoverer --- .../Discovery/UnitTestDiscoverer.cs | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs index 8308f203f8..7b51db7095 100644 --- a/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs +++ b/src/Adapter/MSTest.TestAdapter/Discovery/UnitTestDiscoverer.cs @@ -1,7 +1,6 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. +// 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.Diagnostics; using System.Globalization; using Microsoft.VisualStudio.TestPlatform.MSTest.TestAdapter.Discovery; @@ -59,17 +58,7 @@ internal UnitTestDiscoverer() ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext) { - TimeSpan getTests; - var sw = Stopwatch.StartNew(); -#if NET6_0_OR_GREATER - long beforeDiscovery = GC.GetTotalAllocatedBytes(); -#endif ICollection? testElements = _assemblyEnumeratorWrapper.GetTests(source, discoveryContext?.RunSettings, out ICollection? warnings); - getTests = sw.Elapsed; -#if NET6_0_OR_GREATER - long afterDiscovery = GC.GetTotalAllocatedBytes(); -#endif - sw.Restart(); bool treatDiscoveryWarningsAsErrors = MSTestSettings.CurrentSettings.TreatDiscoveryWarningsAsErrors; @@ -96,14 +85,6 @@ internal UnitTestDiscoverer() source); SendTestCases(source, testElements, discoverySink, discoveryContext, logger); - TimeSpan sendOverhead = sw.Elapsed; -#if NET6_0_OR_GREATER - long afterSend = GC.GetTotalAllocatedBytes(); -#endif - Console.WriteLine($"discovered: {testElements.Count} tests in {getTests.TotalMilliseconds} ms, sent them in {sendOverhead.TotalMilliseconds} ms, total: {sendOverhead.TotalMilliseconds + getTests.TotalMilliseconds} <<<"); -#if NET6_0_OR_GREATER - Console.WriteLine($"discovered: discovery alloc: {(afterDiscovery - beforeDiscovery) / (1024 * 1024)} MB send alloc: {(afterSend - afterDiscovery) / (1024 * 1024)} MB"); -#endif } internal void SendTestCases(string source, IEnumerable testElements, ITestCaseDiscoverySink discoverySink, IDiscoveryContext? discoveryContext, IMessageLogger logger)