Skip to content

Fix PropertiesComparer when properties are not comparable - #4934

Merged
stevenaw merged 3 commits into
nunit:mainfrom
Dreamescaper:fix_property_comparer_for_different_types
Feb 9, 2025
Merged

Fix PropertiesComparer when properties are not comparable#4934
stevenaw merged 3 commits into
nunit:mainfrom
Dreamescaper:fix_property_comparer_for_different_types

Conversation

@Dreamescaper

Copy link
Copy Markdown
Member

Fixes #4933 .

Turns out the problem is not Dictionary specific. It reproduces even for something like this:

class TypeWithObjectProperty
{
     public object? Value { get; set; }
}

var actual = new TypeWithObjectProperty { Value = new ObjectA("WTF") };
var expected = new TypeWithObjectProperty { Value = new ObjectB(123.345) };

Assert.That(actual, Is.EqualTo(expected).UsingPropertiesComparer()); // PASS!!!

The problem is that PropertiesComparer didn't check for NotSupported result - it basically treated it like a successul comparision.

@Dreamescaper

Copy link
Copy Markdown
Member Author

@manfred-brands
Could you please take a look?

EqualMethodResult result = equalityComparer.AreEqual(xPropertyValue, yPropertyValue, ref tolerance, comparisonState);
if (result == EqualMethodResult.ComparedNotEqual)

if (result == EqualMethodResult.ComparedNotEqual || result == EqualMethodResult.TypesNotSupported)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should it not return the TypesNotSupported?
The idea is that if one comparer doesn't support items, it bubble up and the next comparer is called.

As the PropertiesComparer is the last, the result is likely the same.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Originally I returned it, but the error message is not very helpful. It prints root object type name, something like Can't find comparer for types 'TypeWithObjectProperty' and 'TypeWithObjectProperty'. In case of assertion failure it prints exact property, which is much more useful.

Plus if I return TypesNotSupported, it fails with NotSupportedException, AssertionException is probably more suitable here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It depends if we want to distinguish between comparing as different vs not being able to compare.

What does normal comparison do when comparing instances of different types?

@Dreamescaper Dreamescaper Feb 3, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In regular case, it's NotSupportedException.
Which makes sense - as I have explicitly passed two incomparrable values to an assert method, i.e. I have done something wrong.

In this case, however - I'm passing two TypeWithObjectProperty instances, that's a perfectly valid thing to do. I can't know if some deep down property values have different types - that's exactly what I'm testing. Therefore - it simply should be an assertion failure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One final thing. What does a regular compare on a dictionary with different element types return?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AssertionException:
Assert.That(actual, Is.EqualTo(expected))
Expected: <NUnit.Framework.Tests.TypeWithObjectProperty>
But was: <NUnit.Framework.Tests.TypeWithObjectProperty>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh, Dictionary, not object. Still,

Assert.That(actual, Is.EqualTo(expected))
  Expected and actual are both <System.Collections.Generic.Dictionary`2[System.String,System.Object]> with 1 elements

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks. Nothing further.

@stevenaw

stevenaw commented Feb 9, 2025

Copy link
Copy Markdown
Member

Thanks for another contribution @Dreamescaper
Great test coverage.

@stevenaw
stevenaw merged commit 507e9a2 into nunit:main Feb 9, 2025
@Dreamescaper
Dreamescaper deleted the fix_property_comparer_for_different_types branch February 9, 2025 11:39
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.

Dictionary assertion falsely passes with PropertiesComparer when subtypes are involved

3 participants