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

Equ all the way down #25

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Equ all the way down #25

wants to merge 8 commits into from

Conversation

ibebbs
Copy link

@ibebbs ibebbs commented Feb 25, 2021

Hi,

I encountered unexpected behaviour in Equ wherein the following (uncommented) unit test failed:

namespace Equ.Test
{
    public class NestedCollectionsTest
    {
        public class Level2
        {
            public string BasicProperty { get; set; }
        }

        public class Level1
        {
            public ICollection<Level2> Items { get; set; }
        }

        public class Container
        {
            public ICollection<Level1> Items { get; set; }
        }

        public static IEnumerable<object[]> ShouldDetermineCorrectEqualityTests => new List<object[]>
        {
            //new object[] { new Container(), new Container(), true },
            //new object[] { 
            //    new Container { Items = new List<Level1>() },
            //    new Container { },
            //    false
            //},
            //new object[] {
            //    new Container { Items = new List<Level1>() },
            //    new Container { Items = new List<Level1>() },
            //    true
            //},
            //new object[] {
            //    new Container { Items = new List<Level1>() },
            //    new Container { Items = Array.Empty<Level1>() },
            //    true
            //},
            //new object[] {
            //    new Container { Items = new [] { new Level1() } },
            //    new Container { Items = Array.Empty<Level1>() },
            //    false
            //},
            new object[] {
                new Container { Items = new [] { new Level1() } },
                new Container { Items = new [] { new Level1() } },
                true
            },
        };

        [Theory]
        [MemberData(nameof(ShouldDetermineCorrectEqualityTests))]
        public void ShouldDetermineCorrectEquality(Container x, Container y, bool expected)
        {
            Assert.Equal(expected, MemberwiseEqualityComparer<Container>.ByProperties.Equals(x, y));
        }
    }
}

It turns out that, while properties were being used for equality comparision at the root level, collections properties were falling back to using Enumerable.SequenceEqual to determine equality. This caused the above test to fail as the Level1 objects were different instances despite the fact that MemberwiseEqualityConverter<Level1>.ByProperties.Equals would have returned true if used to compare them.

This PR introduces new ByFieldsRecursive and ByPropertiesRecursive properties to MemberwiseEqualityComparer<T> which (via an additional MemberwiseEqualityMode parameter on EqualityFunctionGenerator) cause ElementwiseSequenceEqualityComparer<T> to use the appropriate MemberwiseEqualityComparer<T> instance to compare internal collections such that the following unit tests pass:

    public class NestedCollectionsTest
    {
        public class Level2
        {
            public string BasicProperty { get; set; }
        }

        public class Level1
        {
            public ICollection<Level2> Items { get; set; }
        }

        public class Container
        {
            public ICollection<Level1> Items { get; set; }
        }

        public static IEnumerable<object[]> ShouldDetermineCorrectEqualityTests => new List<object[]>
        {
            new object[] { new Container(), new Container(), MemberwiseEqualityComparer<Container>.ByProperties, true },
            new object[] {
                new Container { Items = new List<Level1>() },
                new Container { },
                MemberwiseEqualityComparer<Container>.ByProperties,
                false
            },
            new object[] {
                new Container { Items = new List<Level1>() },
                new Container { Items = new List<Level1>() },
                MemberwiseEqualityComparer<Container>.ByProperties,
                true
            },
            new object[] {
                new Container { Items = new List<Level1>() },
                new Container { Items = Array.Empty<Level1>() },
                MemberwiseEqualityComparer<Container>.ByProperties,
                true
            },
            new object[] {
                new Container { Items = new [] { new Level1() } },
                new Container { Items = Array.Empty<Level1>() },
                MemberwiseEqualityComparer<Container>.ByProperties,
                false
            },
            new object[] {
                new Container { Items = new [] { new Level1() } },
                new Container { Items = new [] { new Level1() } },
                MemberwiseEqualityComparer<Container>.ByProperties,
                false
            },
            new object[] {
                new Container { Items = new [] { new Level1() } },
                new Container { Items = new [] { new Level1() } },
                MemberwiseEqualityComparer<Container>.ByPropertiesRecursive,
                true
            },
            new object[] {
                new Container { Items = new [] { new Level1 { Items = new[] { new Level2() } } } },
                new Container { Items = new [] { new Level1 { Items = new[] { new Level2() } } } },
                MemberwiseEqualityComparer<Container>.ByProperties,
                false
            },
            new object[] {
                new Container { Items = new [] { new Level1 { Items = new[] { new Level2() } } } },
                new Container { Items = new [] { new Level1 { Items = new[] { new Level2() } } } },
                MemberwiseEqualityComparer<Container>.ByPropertiesRecursive,
                true
            },
        };

        [Theory]
        [MemberData(nameof(ShouldDetermineCorrectEqualityTests))]
        public void ShouldDetermineCorrectEquality(Container x, Container y, MemberwiseEqualityComparer<Container> equalityComparer, bool expected)
        {
            Assert.Equal(expected, equalityComparer.Equals(x, y));
        }
    }

I hope you will find this PR useful and worth merging into the Equ package. If, for some reason, you don't feel it's appropriate to add this functionality to the package, could you let me know so that I can build my own package encompasing it.

Thanks, Ian

…s` and `MemberwiseEqualityComparer<T>.ByProperties` by add a `MemberwiseEqualityMode` parameter to `EqualityFunctionGenerator` and adding new `MemberwiseEqualityComparer<T>.ByFieldsRecursive` and `MemberwiseEqualityComparer<T>.ByPropertiesRecursive` static properties.

private object CreateMemberwiseEqualityComparer(MemberwiseEqualityMode mode)
{
var propertyName = mode switch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I've seen this syntax before. I haven't paid much attention to the new features for a few years though. What is it doing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruler501
Copy link
Contributor

This seems like a reasonable and useful change to me.

…yComparer<T>.ByPropertyRecursive` now use MemberwiseEqualityComparer for nested objects instead of Object.Equals
@ibebbs
Copy link
Author

ibebbs commented Feb 26, 2021

Amazingly I hadn't realised Equ doesn't provided deep equality for nested objects either.

Since @ruler501's last review I've added an additional commit which allows MemberwiseEqualityComparer<T>.ByFieldRecursive & MemberwiseEqualityComparer<T>.ByPropertyRecursive to use MemberwiseEqualityComparer for nested objects instead of Object.Equals.

@ruler501
Copy link
Contributor

Needs documentation of the new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants