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

IsEquivalentTo and BeEquivalentTo do not handle member hiding #956

Closed
mdonoughe opened this issue Nov 3, 2018 · 4 comments
Closed

IsEquivalentTo and BeEquivalentTo do not handle member hiding #956

mdonoughe opened this issue Nov 3, 2018 · 4 comments
Assignees
Labels
bug

Comments

@mdonoughe
Copy link
Contributor

@mdonoughe mdonoughe commented Nov 3, 2018

Description

I noticed the implementation for IsEquivarentTo in TypeExtensions considers two members to be equivalent if they have the same name and the declaring type of either property is the same or inherits the type of the other property. This is incorrect due to member hiding.

Complete minimal example reproducing the issue

E.g.

void Main()
{
	var aProperty = SelectedMemberInfo.Create(typeof(A).GetProperty("Property"));
	var bProperty = SelectedMemberInfo.Create(typeof(B).GetProperties().First(p => p.DeclaringType == typeof(B)));
	aProperty.IsEquivalentTo(bProperty).Should().BeFalse();
	
	B b1 = new B(), b2 = new B();
	b1.Should().BeEquivalentTo(b2, config => config.Including(b => b.Property));
}

class A
{
	public string Property { get; set; } = Guid.NewGuid().ToString();
}

class B: A
{
	public new string[] Property { get; set; }
}

Expected behavior:

Main should execute successfully, because the two properties are different and the two instances of B are equivalent when comparing only the null arrays.

Actual behavior:

Both assertions throw because of the logic error in IsEquivalentTo. The second assertion throws because the logic for BeEquivalentTo gets the property from the expression and then uses IsEquivalentTo to compare it against other properties later.

Versions

  • Which version of Fluent Assertions are you using? 5.4.2
  • Which .NET runtime and version are you targeting? .NET Framework 4.7.2

Additional Information

Using Excluding(b => ((A)b).Property) instead of Including(b => b.Property) does not work either because the code for extracting the member from the expression does not support casts.

@dennisdoomen dennisdoomen added the bug label Nov 3, 2018
@dennisdoomen

This comment has been minimized.

Copy link
Member

@dennisdoomen dennisdoomen commented Nov 3, 2018

Nice catch.

@dennisdoomen

This comment has been minimized.

Copy link
Member

@dennisdoomen dennisdoomen commented Nov 5, 2018

Pfew, this proofs to be much more complex to fix than I thought. Just saying...

dennisdoomen added a commit to dennisdoomen/fluentassertions that referenced this issue Nov 6, 2018
dennisdoomen added a commit to dennisdoomen/fluentassertions that referenced this issue Nov 6, 2018
dennisdoomen added a commit to dennisdoomen/fluentassertions that referenced this issue Nov 7, 2018
dennisdoomen added a commit to dennisdoomen/fluentassertions that referenced this issue Nov 7, 2018
dennisdoomen added a commit that referenced this issue Nov 7, 2018
…ring type (#960)

Fixes #956
@BrunoJuchli

This comment has been minimized.

Copy link
Contributor

@BrunoJuchli BrunoJuchli commented Nov 15, 2018

I suspect this fix caused a breaking change for us with the move from 5.4.2 to 5.5:

We globally configure the AssertionOptions like this:

AssertionOptions.AssertEquivalencyUsing(options => options
                .WithStrictOrdering()
                .RespectingRuntimeTypes()
                .IncludingAllRuntimeProperties();

We have one test which stopped working, again in connection with BeEquivalentTo. Here, the objects compared include Moq Mocks.
The test:

IReadOnlyList<FileEntity> result = this.testee.Retrieve(operations);

result.Should().BeEquivalentTo(
	new List<FileEntity>
	{
		operationWithoutSubNcPrograms.Manufacturing.MainNcProgram,
		operationWithSubNcPrograms.Manufacturing.MainNcProgram,
                    ....
	},
	o => o.WithoutStrictOrdering());

Now fails with:

FluentAssertions.Execution.AssertionFailedException
Expected item[1].Id to be {49de3eb7-9a4b-4945-ba49-c056f68e7118}, but found {68f779e0-e582-4049-be63-cb0b1688b479}.
Expected item[1].Mock.MutableInvocations to be a collection with 24 item(s), but {FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version}"
"contains 20 item(s) less than"
"{FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version, FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version, FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version, FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version, FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version, FileEntity.Name, FileEntity.FilePath, AbstractEntity1.Id, AbstractEntity1.Version}.

Notice the comparison of the Mock.MutableInvocations property. IMHO comparison of this property could never have resulted in equal. With 5.4.2 the test passes (no code changes). I deduct 5.4.2 hasn't compared this property.

Fix

This one is easy enough to fix by adjusting the AssertionOptions to exclude unwanted properties:

AssertionOptions.AssertEquivalencyUsing(options => options
	.RespectingRuntimeTypes()
	.IncludingAllRuntimeProperties()
	.Excluding(x => typeof(Mock).IsAssignableFrom(x.SelectedMemberInfo.MemberType));

Release Notes

It might be worth mentioning this in the release notes?

@dennisdoomen

This comment has been minimized.

Copy link
Member

@dennisdoomen dennisdoomen commented Nov 15, 2018

Thanks for that note @BrunoJuchli. Essentially < 5.5 contained a false positive that was fixed. I'll update the release notes.

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.