Issue 1649 #1651
Issue 1649 #1651
Conversation
Use PropertyInfo for accessing properties
|
|
||
| namespace NUnit.Extensibility | ||
| { | ||
| using Wrappers; |
There was a problem hiding this comment.
I don't like using statements in 2 places in the same file.
As it isn't used anywhere else in the project, I would prefer to move this outside the namespace declaration.
| private object _wrappedInstance; | ||
| private Type _wrappedType; |
There was a problem hiding this comment.
The can be declared readonly
| private struct MethodSignature | ||
| { | ||
| public string Name; | ||
| public object[] ArgTypes; |
There was a problem hiding this comment.
Should this not be Type[]?
I know this type only exists for the sake of the Dictionary.
However it doesn't work.
Even though this type is declared a struct, the structural equality doesn't work on class members such as the array. It fails as soon as the arrays are not the same instance which is every time as the array is re-created for every call.
I added a test to verify the cache is working.
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Security.Cryptography.X509Certificates; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
A lot of unnecessary using statements.
|
|
||
| protected void Invoke(string methodName, params object[] args) | ||
| { | ||
| WrapMethod(methodName, Type.GetTypeArray(args)).Invoke(_wrappedInstance, args); |
There was a problem hiding this comment.
Although elegant, there are two problem with Type.GetTypeArray.
It doesn't support null arguments as you can't get the type of null.
I don't see any of your wrappers allowing null. So it might not be relevant.
The other problem is that you get a new array every call which means that the MethodSignature key is different and there is no caching. To a lesser extent it is a small hit on the GC, but with this calling reflection that would be irrelevant.
I can fix the caching by overriding GetHashCode and Equals, but as the signature is fixed at compile time, it would be better to pass the types in.
I added overloads allowing passing in the ArgTypes.
I also added overloads for the common method that have one string argument.
| protected T Invoke<T>(string methodName, params object[] args) | ||
| { | ||
| Console.WriteLine($"Wrapper called for {typeof(T)} {methodName}"); | ||
| if (args == null) args = Array.Empty<object>(); |
There was a problem hiding this comment.
This would only happen if the derived class explicitly passes in null. Not if they pass in no parameters.
| @@ -88,18 +87,42 @@ private PropertyInfo WrapProperty(string propertyName) | |||
| return prop; | |||
| } | |||
|
|
|||
| private struct MethodSignature | |||
| internal IEnumerable<MethodSignature> GetMethodSignatures() => _methods.Keys; | |||
There was a problem hiding this comment.
Adding a method to permit testing is a good idea!
This may fix #1649, depending on the discussion on the issue itself.