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

[MERGE FOR 5.2] Feature/add equals verifier concept #2795

Merged
merged 23 commits into from Feb 7, 2022

Conversation

pelletier197
Copy link
Contributor

@pelletier197 pelletier197 commented Jan 22, 2022

Proof of concept using Comparators concept. I have not pushed the concept very far at first, since I wanted to get feedback on the idea first.

I will add more unit tests to this if you like the idea.

What Comparators can bring to Kotest is more flexible matching on list elements and reduced redundancy. The interface is quite similar to the Matcher interface, but in the difference that it takes two parameters instead of one. Therefore, allowing for more complex pre-defined implementations.

The goal could be in long term to help reduce the number of required functions in the library. For instance:

fun <T : Any> T.shouldBeEqualToIgnoringFields(other: T, property: KProperty<*>, vararg others: KProperty<*>) 
// and
fun <T : Any> T.shouldBeEqualToIgnoringFields(other: T, ignorePrivateFields: Boolean, property: KProperty<*>, vararg others: KProperty<*>)
// and
fun <T : Any> T.shouldBeEqualToUsingFields(other: T, vararg properties: KProperty<*>) 
// and many other functions like these

could become a single function that takes in parameter configurable implementations of Comparator

fun <T : Any> T.shouldBe(other: T, comparator: Comparator<T>) 

which could be used like this for instance

a.shouldBe(b, comparator=Comparators.reflectionIgnoringFields(B::test))

a.shouldBe(b, comparator=Comparators.reflectionIgnoringFields(B::test).includingPrivateFields())

a.shouldBe(b, comparator=Comparators.reflectionUsingFields(B::test))

The same would apply for collections (which currently do not support matchers ignoring fields)

listOf(a).shouldContain(b, comparator=Comparators.reflectionIgnoringFields(B::test))

So in term, it would simplify the library a lot by removing many redundant functions, and would easily allow people to implement their own comparators.

Let me know what you think!

@sksamuel
Copy link
Member

Love the idea.

I would change Comparator to be Equals and have the function return a boolean. Then it's easier to resuse later for other matchers, such as contains in order, etc.

Other than that, looks awesome.

@pelletier197
Copy link
Contributor Author

pelletier197 commented Jan 22, 2022

I would change Comparator to be Equals

Agree with that

and have the function return a boolean.

I agree that reusing the matcher was maybe not the best approach. I wanted to reuse what was there, but it doesn't fit much.

Tho, not a fan of returning simply a boolean, because it offers no detail as of why the equality check failed. It could be nice to have details for field by field comparison for instance.

I'll try creating another object. Something like "EqualityResult".

Thanks for the feedback!

@sksamuel
Copy link
Member

Ok EqualityResult seems good.

@pelletier197 pelletier197 changed the title Feature/add comparators concept Draft: Feature/add comparators concept Jan 23, 2022
@pelletier197 pelletier197 changed the title Draft: Feature/add comparators concept Feature/add equals verifier concept Jan 28, 2022
@pelletier197
Copy link
Contributor Author

Hi @sksamuel I modified the Comparator to EqualityVerifier. A bit long, but I feel like Equals alone was a weird class name.

I tried to keep my changes short for this first one, but my long term goal would be to use these verifiers for the matchers of shouldBe, shouldContain. That would move the static eq function, into various verifiers. This would allow to easily re-use the exact same equality methods for shouldBe and shouldContain etc.

Let me know what you think!

@sksamuel
Copy link
Member

I like the idea. A lot of reuse.

On the naming, I think EqualityVerifier is a bit verbose. equals and verify is kinda the same thing. What about just Equality? Typeclasses like this tend to be single words - Comparator, Applicative etc.

@pelletier197
Copy link
Contributor Author

pelletier197 commented Jan 29, 2022

I agree that the name is verbose.

How do you see this? Could keep the interface named this way:

interface EqualityVerifier<T: Any?> {
   fun name(): String

   fun verify(actual: T, expected: T) : EqualityResult
}

but call the factory object like this?

object Equality {
   fun <T> default() = objectEquality<T>()
}

This would make it less verbose to use. I don't think users would call these verifier constructors directly. They would use the factory class. So they would not really have to worry about the interface, except if they implement their own verifier.

Otherwise, If we call the interface Equality, what would be the name of the factory object? Equalities? Doesn't that look a little weird? Or would you implement the factory in a different way?

@sksamuel
Copy link
Member

sksamuel commented Jan 29, 2022 via email

@pelletier197
Copy link
Contributor Author

I went with the companion object as you proposed. It's great 💯

@sksamuel
Copy link
Member

This looks awesome.

@sksamuel sksamuel changed the title Feature/add equals verifier concept [MERGE FOR 5.2] Feature/add equals verifier concept Jan 30, 2022
@sksamuel sksamuel linked an issue Jan 30, 2022 that may be closed by this pull request
@sksamuel
Copy link
Member

I'll merge this once 5.1.1 is out and master becomes the 5.2 build.

@sksamuel sksamuel merged commit 68fec75 into kotest:master Feb 7, 2022
@sksamuel sksamuel added this to the 5.2 milestone Feb 7, 2022
@sksamuel sksamuel mentioned this pull request Mar 12, 2022
22 tasks
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.

Support matching elements in arrays ignoring fields
2 participants