Unify reflection helpers #7344
Unify reflection helpers #7344
Conversation
src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs
Outdated
Show resolved
Hide resolved
# Conflicts: # src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TestPropertyAttributeTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Execution/TypeCacheTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Helpers/ReflectHelperTests.cs
…tion # Conflicts: # src/Adapter/MSTestAdapter.PlatformServices/Services/ReflectionOperations.cs # src/Adapter/MSTestAdapter.PlatformServices/Services/TestDeployment.cs # src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs # src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentUtilityBase.cs # src/Adapter/MSTestAdapter.PlatformServices/Utilities/ReflectionOperationsNetFrameworkAttributeHelpers.cs # test/IntegrationTests/PlatformServices.Desktop.IntegrationTests/ReflectionUtilityTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/DesktopTestDeploymentTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Services/TestDeploymentTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Utilities/DeploymentItemUtilityTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Utilities/DeploymentUtilityTests.cs # test/UnitTests/MSTestAdapter.PlatformServices.UnitTests/Utilities/ReflectionUtilityTests.cs
Youssef1313
left a comment
Youssef1313
left a comment
There was a problem hiding this comment.
I guess we are good now that everything uses the caching ReflectHelper on current main?
(Except that we need to investigate if it's possible to simplify the implementation of reflection operations under .NET Framework)
| // PERF: This was moved from Dictionary<MemberInfo, Dictionary<string, object>> to Concurrent<ICustomAttributeProvider, Attribute[]> | ||
| // storing an array 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 ConcurrentDictionary<ICustomAttributeProvider, Attribute[]> _attributeCache = []; |
There was a problem hiding this comment.
reflection operations itself isn't expected to be doing the caching, I think.
It was introduced only as a way to abstract how we do reflection (e.g, real reflection or something based on source generation). Then, reflection operations is supposed to be called by ReflectHelper which does the caching.
There was a problem hiding this comment.
Does it change anything? We had 2 helpers for reflection and now we have a single one, that seems to be matching.
There was a problem hiding this comment.
We had 2 helpers for reflection
Yup but mostly one that wraps the other and only one of them should be used by the rest of the product, not two separate helpers that are used across the product (as it was with ReflectHelper and ReflectionUtility before the past refactorings).
My concerns here are:
- We might be complicating things when we want to introduce different reflection-like mechanisms, as we no longer have a centralized place that can do the caching.
- The PR does more than unification, as it removes also the logic behind reflection-only assemblies, which I don't know exactly if it will have any impacts in any edge case scenarios. While it's a logic I would like to remove and simplify, I want to be 100% sure about it to avoid regressions.
2 participants
Fixes #3043