-
Notifications
You must be signed in to change notification settings - Fork 731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IStructuralEquatable support #3646
Conversation
…omparer to delegate to enumerablecomparer
@@ -48,6 +49,10 @@ internal EquatablesComparer(NUnitEqualityComparer equalityComparer) | |||
Type xType = x.GetType(); | |||
Type yType = y.GetType(); | |||
|
|||
if (x is IEnumerable && DoesUseStructuralEquality(xType) | |||
&& y is IEnumerable && DoesUseStructuralEquality(yType)) | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skip using EquatablesComparer if each type fulfills these criteria:
- Implements IEnumerable
- Implements either IStructuralComparable or IStructuralEquatable
- Is a value type
I'm unsure if this is overkill. Would it be enough to just check for IEnumerable
here, or would that be overly broad and break something? Alternately, would it be better to instead check if the types are valuetypes that implement IStructuralEquatable
(though I suspect this might break the "defer to enumerables" idea)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea that we would give IStructural interfaces priority over IEnumerable. Not sure that being a value type should make a difference; is there are reason you can think of?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points thanks @jnm2 . I think I'd been looking for a "quick and easy" fix here, but I see it's a little more nuanced. Especially with having IStructural take priority over IEnumerable (similar to how EquatablesComparer takes priority).
Re: value types, I had originally added that in error. I noticed MSDN primarily showed value types as implementing this, but of course both structs and classes can implement interfaces.
I've stepped back and given this a different approach now, with a new StructuralComparer
type which, when invoked, will use NUnitEqualityComparer for all comparisons (including Tolerance).
src/NUnitFramework/framework/Constraints/Comparers/EquatablesComparer.cs
Outdated
Show resolved
Hide resolved
@@ -32,6 +32,7 @@ | |||
|
|||
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="15.7.0" /> | |||
<PackageReference Include="NUnit3TestAdapter" Version="3.13.0" /> | |||
<PackageReference Include="System.Collections.Immutable" Version="1.5.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to hit issues on the tests when building with Cake with version >= 1.6 on netcoreapp21 (similar to what was encountered here: Azure/azure-functions-vs-build-sdk#353). VS tests run fine with the latest version of this (1.7.1).
if (!type.IsValueType) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would a reference type not use structural equality? For example, C# 9 records are emitted as ordinary classes, but they implement structural equality as indeed any class can if it wishes.
@@ -11,6 +11,10 @@ | |||
<PackageReference Include="jnm2.ReferenceAssemblies.net35" Version="1.0.1" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(TargetFramework)' != 'net35' and '$(TargetFramework)' != 'net40'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IStructuralEquatable
is not available to NET4 on NuGet via System.Runtime
. Instead, it is just a part of the framework
Fixes #3447
It looks like the reason for the failures here was that, as an implementer of IEquatable<>, ImmutableArray was not deferring comparisons down to the enumerable-level. IEquatable<> in turn attempted reference equality on the boxed objects of ImmutableArray (a struct).
I'd welcome some feedback on the implementation here, as I'm not entirely sure the best solution.