Address second round of PR review comments: guard unsupported types in GetCustomAttributesCached, add null-guard test, fix integration test attribute ordering, add doc comments

Author: Evangelink

src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs

@@ -224,6 +224,13 @@ public IEnumerable<TAttributeType> GetAttributes<TAttributeType>(ICustomAttribut
     /// <returns>attributes defined.</returns>
     public Attribute[] GetCustomAttributesCached(ICustomAttributeProvider attributeProvider)
     {
+        if (attributeProvider is not (MemberInfo or Assembly))
+        {
+            throw new ArgumentException(
+                $"Unsupported attribute provider type: {attributeProvider?.GetType()}. Only MemberInfo and Assembly are supported.",
+                nameof(attributeProvider));
+        }
+
         // If the information is cached, then use it otherwise populate the cache using
         // the reflection APIs.
         return _attributeCache.GetOrAdd(attributeProvider, GetAttributes);

test/IntegrationTests/PlatformServices.Desktop.IntegrationTests/ReflectionUtilityTests.cs

@@ -45,7 +45,7 @@ public void GetCustomAttributesShouldReturnAllAttributes()
         attributes.Should().NotBeNull();
         attributes.Should().HaveCount(2);
 
-        string[] expectedAttributes = ["TestCategory : base", "Owner : base"];
+        string[] expectedAttributes = ["Owner : base", "TestCategory : base"];
         GetAttributeValuePairs(attributes!).Should().Equal(expectedAttributes);
     }
 
@@ -59,7 +59,7 @@ public void GetCustomAttributesShouldReturnAllAttributesWithBaseInheritance()
         attributes.Should().HaveCount(3);
 
         // Notice that the Owner on the base method does not show up since it can only be defined once.
-        string[] expectedAttributes = ["TestCategory : derived", "TestCategory : base", "Owner : derived"];
+        string[] expectedAttributes = ["Owner : derived", "TestCategory : derived", "TestCategory : base"];
         GetAttributeValuePairs(attributes!).Should().Equal(expectedAttributes);
     }
 
@@ -125,7 +125,7 @@ public void GetCustomAttributesShouldReturnAllAttributesIncludingUserDefinedAttr
         attributes.Should().NotBeNull();
         attributes.Should().HaveCount(3);
 
-        string[] expectedAttributes = ["Duration : superfast", "TestCategory : base", "Owner : base"];
+        string[] expectedAttributes = ["Duration : superfast", "Owner : base", "TestCategory : base"];
         GetAttributeValuePairs(attributes!).Should().Equal(expectedAttributes);
     }
 

test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/ReflectionOperationsTests.cs

@@ -22,6 +22,13 @@ public class ReflectionOperationsTests : TestContainer
     private readonly Mock<MethodInfo> _method;
     private readonly TestablePlatformServiceProvider _testablePlatformServiceProvider;
 
+    // NOTE: This class mutates the shared static PlatformServiceProvider.Instance (set in
+    // the constructor, restored in Dispose). Tests that call GetCustomAttributesCached or
+    // methods that go through NotCachedReflectionAccessor depend on this static being set
+    // to _testablePlatformServiceProvider. This pattern is inherited from the former
+    // ReflectHelperTests. If parallel execution with other test classes that also mutate
+    // PlatformServiceProvider.Instance causes intermittent failures, consider adding
+    // [DoNotParallelize] to this class.
     public ReflectionOperationsTests()
     {
         _reflectionOperations = new ReflectionOperations();
@@ -235,6 +242,13 @@ public void IsAttributeDefinedShouldReturnTrueIfSpecifiedAttributeIsDefinedOnAMe
         rh.IsAttributeDefined<TestMethodAttribute>(mockMemberInfo.Object).Should().BeTrue();
     }
 
+    public void IsAttributeDefinedShouldThrowWhenMemberInfoIsNull()
+    {
+        var rh = new ReflectionOperations();
+        Action action = () => rh.IsAttributeDefined<TestMethodAttribute>(null!);
+        action.Should().Throw<ArgumentNullException>().WithParameterName("memberInfo");
+    }
+
     public void IsAttributeDefinedShouldReturnFalseIfSpecifiedAttributeIsNotDefinedOnAMember()
     {
         var rh = new ReflectionOperations();
@@ -439,6 +453,12 @@ internal class AttributeMockingHelper
         private readonly List<(Type Type, Attribute Attribute, MemberTypes MemberType)> _data = [];
         private readonly Mock<IReflectionOperations> _mockReflectionOperations;
 
+        /// <summary>
+        /// Adds (not replaces) attribute entries of <paramref name="memberTypes"/>
+        /// to the internal list used by the mock handler. Successive calls for the
+        /// same <paramref name="memberTypes"/> are cumulative — previous entries are
+        /// kept alongside new ones.
+        /// </summary>
         public void SetCustomAttribute(Type type, Attribute[] values, MemberTypes memberTypes)
         {
             foreach (Attribute attribute in values)

test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/TestableImplementations/MockableReflectionOperations.cs

@@ -75,7 +75,7 @@ public Attribute[] GetCustomAttributesCached(ICustomAttributeProvider attributeP
         {
             MemberInfo memberInfo => mock.Object.GetCustomAttributes(memberInfo)?.OfType<Attribute>().ToArray() ?? [],
             Assembly assembly => mock.Object.GetCustomAttributes(assembly, typeof(Attribute))?.OfType<Attribute>().ToArray() ?? [],
-            _ => attributeProvider.GetCustomAttributes(true).OfType<Attribute>().ToArray(),
+            _ => throw new NotSupportedException($"Unsupported attribute provider type: {attributeProvider.GetType()}. Only MemberInfo and Assembly are supported."),
         };
 
     public bool IsMethodDeclaredInSameAssemblyAsType(MethodInfo method, Type type)