Allow Comparing only a subset of properties.

[manfred-brands]

Fixes #4687
Fixes #4935
Fixes #4244

[stevenaw]

Thanks for your patience on this PR too @manfred-brands . I hope to catch up on reviewing this too in the next few days

[manfred-brands]

@stevenaw I have converted this back to draft to allow me ironing out the updated syntax/functionality.

[manfred-brands]

This PR adds the following syntax:

public XXXConstraint UsingPropertiesComparer(Func<PropertiesComparerConfigurationUntyped, PropertiesComparerConfigurationUntyped> configure)
public XXXConstraint<T> UsingPropertiesComparer(Func<PropertiesComparerConfiguration<T>, PropertiesComparerConfiguration<T>> configure)

Where PropertiesComparerConfiguration<T> has the following options:

• AllowDifferentTypes() This enables comparing properties from two different types, but with matching names.
• AllowExtraProperties() This allows the different types to have extra properties and only the common ones are compared.
• Using(params expression) This specifies which properties to compare, e.g.: Using(x => x.Name).
• Excluding(params expression) This specifies which properties to ignore, e.g.: Excluding(x => x.Age).
• Map<TFrom, TTo>(expressionFrom, expressionTo) This maps a property from one type to another property in another type, e.g.: Map(x => x.AreaCode, y => y.ZipCode)
• Map(expression, value) This substitutes value when comparing property, e..g.: Map(x => x.Country, "Australia")

I'm open to changes in syntax or if more functionality is needed.

[stevenaw]

Thanks @manfred-brands

A few thoughts on naming and other things:

AllowDifferentTypes()

No strong feelings either way on this. I think this naming is clear as a function used to configure the comparer. Another option I'm not sure if we've considered here is to use generics to allow the user to declaratively say the types.

// Compares two objects of the same type
UsingPropertiesComparer()

// Compares two objects of different types
UsingPropertiesComparer<T1, T2>()

This might also pair more nicely if we ever get type-safety for different generic typing at the assert level (ex: Assert.That(new A(), Is.EqualTo(new B())). Thoughts?

AllowExtraProperties()

Would this mean moving away from the user providing a list of prop names they'd want to compare? I like the idea of it "just working" in that we either compare all props or a predefined list without the framework having to decide what the user may or may not intend to compare as "common" properties.

Using(params expression) This specifies which properties to compare, e.g.: Using(x => x.Name).

I think we already have a Using() function on the constraint itself which controls the comparer to use. Would changing this to Including() help disambiguate? It would also pair more naturally with the Excluding() method as they serve complimentary, opposing options.

Map<TFrom, TTo>(expressionFrom, expressionTo)
Map(expression, value) This substitutes value when comparing property

I'm having a hard time understanding how this would work from the API itself, as well as the need for it. Would one prop get mapped to another only to then use the default comparer for that type? I'm wondering if these functions opens a bit of a slippery slope into effectively making our own AutoMapper. I'm wondering if a first draft here might be simpler if we approach it as "two objects, with the option to do all or a predefined subset of properties" while still allowing NUnit equality rules to take effect.

[manfred-brands]

Thanks @manfred-brands

A few thoughts on naming and other things:

AllowDifferentTypes()

No strong feelings either way on this. I think this naming is clear as a function used to configure the comparer. Another option I'm not sure if we've considered here is to use generics to allow the user to declaratively say the types.

// Compares two objects of the same type
UsingPropertiesComparer()

// Compares two objects of different types
UsingPropertiesComparer<T1, T2>()

This might also pair more nicely if we ever get type-safety for different generic typing at the assert level (ex: Assert.That(new A(), Is.EqualTo(new B())). Thoughts?

A long as NUnit has no coupling between the TActual and TConstraint this would require always specifying both types.
Note that there is a generic version of the PropertiesComparrConfiguration which is picked when the EqualConstraint<T> and equivalents are known. Because of that it allows specifying properties as expressions instead of strings.

AllowExtraProperties()

Would this mean moving away from the user providing a list of prop names they'd want to compare? I like the idea of it "just working" in that we either compare all props or a predefined list without the framework having to decide what the user may or may not intend to compare as "common" properties.

At the moment this means CompareOnlyCommonProperties. We can rename it to that.
If class A has properties X, Y, Z
If class B has properties ID, X, Y, Z. Here ID could be an auto-column in a database.
Then only X, Y and Z will be compared and the value of ID is not considered.

The alternative is to explicitly exclude the ID property with c.Excluding("ID").

Using(params expression) This specifies which properties to compare, e.g.: Using(x => x.Name).

I think we already have a Using() function on the constraint itself which controls the comparer to use. Would changing this to Including() help disambiguate? It would also pair more naturally with the Excluding() method as they serve complimentary, opposing options.

See my comments in #4687 why including is not correct as it implies extra.
Using does fit in with what we already have. Either using a specific comparer or using specific properties.

Map<TFrom, TTo>(expressionFrom, expressionTo)
Map(expression, value) This substitutes value when comparing property

I'm having a hard time understanding how this would work from the API itself, as well as the need for it. Would one prop get mapped to another only to then use the default comparer for that type? I'm wondering if these functions opens a bit of a slippery slope into effectively making our own AutoMapper. I'm wondering if a first draft here might be simpler if we approach it as "two objects, with the option to do all or a predefined subset of properties" while still allowing NUnit equality rules to take effect.

For each property, the NUnit rules are in effect.
There is nothing Auto as the user has to specify the mapping.
The request of mapping properties is listed in #4935 and is loosely based on functionality in Fluent Assertions

The mapping of value is my own. It seems to fit with my example in the nunit test where a USAddress is only equivalent with a generic Address if the Address.Country == "U.S.A."
If gives a better comparison than ignoring the Country property.

[stevenaw]

Ah, you are right, I had missed the mapping request in the original issue.

It still rubs me the wrong way a bit not to have Including() provide the opposite functionality to Excluding() but having read your explanation earlier I can understand that Including is perhaps a bit overloaded here. I don't have an alternative to suggest other than Using() that you've chosen so I'm also fine with that. Naming is hard.

If class A has properties X, Y, Z
If class B has properties ID, X, Y, Z. Here ID could be an auto-column in a database.
Then only X, Y and Z will be compared and the value of ID is not considered.

I think cases like this are the core of my questions. I haven't used FluentAssertions and so I'm unfamiliar with how they approached this. Having thought about it a bit more from a few different ways, I think I'd find CompareOnlyCommonProperties() or perhaps even CommonPropertiesOnly() as more intuitive to someone like myself without a FluentAssertions background

[OsirisTerje]

All tests works. Code looks good.

The new added syntax should also be documented (in the docs).

Using using makes it equal to the other similar usages we have, so I don't mind that.

CompareOnlyCommonProperties or CommonPropertiesOnly makes more sense to me than AllowExtraProperties.

What are the default for the properties?

[manfred-brands]

The new added syntax should also be documented (in the docs).

I will create a PR shortly.

CompareOnlyCommonProperties or CommonPropertiesOnly makes more sense to me than AllowExtraProperties.

Renamed into CompareOnlyCommonProperties

What are the default for the properties?

Defaults are as before:

• Only compare identical types, which implies they have common properties.
• No excluded properties

[stevenaw]

Thanks for having taken a look at this @OsirisTerje
It's been hard for me to find a large enough block of time to have dug into it at the code level as you have.

I'll unassign myself as reviewer for now but will still try to review this later when time allows to catch myself up

[manfred-brands]

@OsirisTerje Why is the WIP check hanging?

[OsirisTerje]

@manfred-brands No idea. I'll force merge it.