Skip to content
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

Expand NUnitEquality to use Equality of all Properties #4394

Closed
manfred-brands opened this issue May 23, 2023 · 7 comments · Fixed by #4436
Closed

Expand NUnitEquality to use Equality of all Properties #4394

manfred-brands opened this issue May 23, 2023 · 7 comments · Fixed by #4436
Labels
closed:done is:enhancement pri:normal requires:docs Issues that require documentation be added
Milestone

Comments

@manfred-brands
Copy link
Member

We have several class that we don't want to implement IEquatable<> for the sake of NUnit tests.

To compare these we have created a ReflectionAssert.PropertiesEqual class.
This class gets all the properties of a class and calls the method recursively on those.
If any properties implements IEquatable<> it will call the Equals method instead.
It would be a nice feature to add for NUnit4 so that we can compare more classes.

@stevenaw
Copy link
Member

@manfred-brands Just to confirm, this would only be for two objects of the same type? Supporting cross-type "shape" checks had some complicated edge cases in #4244

@manfred-brands
Copy link
Member Author

@stevenaw This would only be when both actual and expected have the same type.

@stevenaw
Copy link
Member

I could see this being helpful, and could've used such a feature within the last few weeks. How did you envision the API looking and acting?

@stevenaw
Copy link
Member

stevenaw commented May 27, 2023

Wait, perhaps we're thinking different things. Were you thinking this is a new constraint + API, or a change to the existing ones?

EDIT: Could you give a code sample of how you saw this working?

@manfred-brands
Copy link
Member Author

manfred-brands commented Jun 2, 2023

Either an built-in comparer that a user can pass in if the user knows the type at hand doesn't implement IEquatable<T>:

Assert.That(model, Is.EqualTo(_Model).Using(PropertyComparer.Instance));

Or do that automatically by NUnit for all classes that don't implement IEquatable<T>.
Currently the last call in NUnitEqualityComparer.AreEqual is return x.Equals(y);.
For classes where the method is not overloaded, the behaviour of that method is the same as reference equals (which is already covered earlier in the AreEqual method) and so if the IEquatable<T> case.
There could be code that overrides Equals(object) and not implements IEquatable<T>.
We could test here if the class does do this, if so call the Equals method, if not, call the PropertyComparer.

@manfred-brands
Copy link
Member Author

manfred-brands commented Jun 2, 2023

Here an implementation that seem to work for simple cases.
It will not work for cyclic references where a property has the same type as the type at hand.
It also will not deal with IEnumerables.
When built into nunit framework if could pass in Tolerance and ComparisonState.
It could also call the NUnitEqualityComparer to deal with the special case (IEnumerable) handled there.

using System;
using System.Collections;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

namespace NUnit.Framework.Constraints.Comparers
{
    internal sealed class PropertyComparer : IEqualityComparer
    {
        public static PropertyComparer Instance { get; } = new PropertyComparer();

        bool IEqualityComparer.Equals(object? x, object? y) => PropertyEquals(x, y);

        int IEqualityComparer.GetHashCode([DisallowNull] object obj) => throw new NotImplementedException();

        private static bool PropertyEquals(object? x, object? y)
        {
            if (ReferenceEquals(x, y))
                return true;

            if (x is null || y is null)
                return false;

            Type type = x.GetType();

            if (type != y.GetType())
                return false;

            // Shortcut if the type at hand implements IEquatable<T>
            MethodInfo? equalsMethod = type.GetMethod(
                "Equals",
                BindingFlags.Instance | BindingFlags.ExactBinding | BindingFlags.Public,
                null,
                new[] { type },
                null);
            if (equalsMethod is not null)
            {
                return (bool)equalsMethod.Invoke(x, new[] { y })!;
            }

            PropertyInfo[] properties = type.GetProperties(BindingFlags.Instance | BindingFlags.Public);
            foreach (var property in properties)
            {
                object? xPropertyValue = property.GetValue(x, null);
                object? yPropertyValue = property.GetValue(y, null);

                if (!PropertyEquals(xPropertyValue, yPropertyValue))
                {
                    return false;
                }
            }

            return true;
        }
    }
}

@stevenaw
Copy link
Member

@manfred-brands sorry it's taken me a bit to respond, I've been wanting to get to a computer to test a few things but that's proven difficult.

I like this idea! Particularly that it would minimize the amount of "test-only" code one may need to write, but also that it's opt-in.

I'd been wanting a computer to experiment on some potential usages with how this may also be exposed as a property of a constraint itself (like how we use Within, etc) but I'm not sure how that could look or if it would be helpful. Mostly, I'd been thinking about ways we could try to make such a new feature easily discoverable via Intellisense - though something like that doesn't necessarily need to be designed upfront.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed:done is:enhancement pri:normal requires:docs Issues that require documentation be added
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants