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

Idiomatic assertions for IEqualityComparer<T> #1194

Merged
merged 5 commits into from Sep 10, 2020

Conversation

Kralizek
Copy link
Contributor

At my company we have ended up creating some implementations of IEqualityComparer<T> so I've worked on an idiomatic assertion to quickly check that they respect some basic requirements:

  • Equals(a, a) == true (reflexive property)
  • Equals(a, b) == Equals(b, a) (symmetric property)
  • Equals(a, b) == Equals(b, c) == Equals(a, c) (transitive property)
  • Equals(a, null) == false
  • Equals(null, null) == true
  • GetHashCode(a) == GetHashCode(a)

I added to this PR all the code produced right now. If there is an interest, I can continue working on this PR.

Here is a sample on how to use it:

Given the following class implementing IEqualityComparer<string>

public class TestEqualityComparer : IEqualityComparer<string>
{
	public bool Equals(string x, string y)
	{
		if (x is null ^ y is null) return false;
		
		return string.Equals(x, y);
	}

	public int GetHashCode(string obj)
	{
		return obj.GetHashCode();
	}
}

Developers can use this assertion as simply as

[Test]
public void IEqualityComparer_interface_is_implemented_correctly()
{
	var fixture = new Fixture();
	
	var assertion = fixture.Create<EqualityComparerAssertion>();
	
	assertion.Verify(typeof(TestEqualityComparer));
}

I haven't gone added the unit tests I have in my own solution and decorated methods and property with documentation comments. Also, I have used few features from C# 8 but I'm not sure if the current build setup supports them.

@Kralizek Kralizek marked this pull request as draft August 27, 2020 09:46
@aivascu
Copy link
Member

aivascu commented Aug 31, 2020

@Kralizek thank you for the contribution. I think this will make a nice addition to AutoFixture.

There is a package that implements these same assertions and some more.
I don't know if @baks would like to contribute those features to AutoFixture but at least it could serve as inspiration for this PR.

I think fixing the issues from the appveyor build log and adding the tests would be the first step to moving forward with the PR.

@Kralizek
Copy link
Contributor Author

@aivascu
Glad you like it! The package you linked doesn't have anything (AFAIK) for implementing IEqualityComparer, hence my PR.

Yep, appveyor went crazy on feedback. I'll work on it when I have some free time.

@Kralizek Kralizek marked this pull request as ready for review September 5, 2020 20:19
@Kralizek
Copy link
Contributor Author

Kralizek commented Sep 5, 2020

@aivascu I fixed the build errors and added relevant tests so this PR is ready for a review. I looked ad @baks's code as suggested but I couldn't take much as it uses a lot of helper classes and I didn't know if we wanted to import them into AutoFixture.Idioms.

Copy link
Member

@aivascu aivascu left a comment

Choose a reason for hiding this comment

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

@Kralizek The PR looks good, I've made only a few code style changes. Let me know if you're OK with them.

@Kralizek
Copy link
Contributor Author

No worries! Nice changes. I didn't think about putting that ugly pattern match into its own method.

@aivascu aivascu merged commit 3092ff1 into AutoFixture:master Sep 10, 2020
@zvirja
Copy link
Member

zvirja commented Sep 10, 2020

@Kralizek Nice work, like how thorough we are testing it! Having more assertions of the quality you created could make library insanely useful! Thanks for your contribution! 🤝

@Kralizek
Copy link
Contributor Author

Glad you like it!

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.

None yet

3 participants