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

Add usingRecursiveComparison for Iterables #1238

Closed
sandeep-chekuri opened this issue May 9, 2018 · 14 comments
Closed

Add usingRecursiveComparison for Iterables #1238

sandeep-chekuri opened this issue May 9, 2018 · 14 comments
Labels
type: improvement A general improvement

Comments

@sandeep-chekuri
Copy link

Summary

Currently there is a feature to exclude fields using isEqualToIgnoringGivenFields while asserting a bean
http://joel-costigliola.github.io/assertj/assertj-core-features-highlight.html

We are exactly looking for similar feature while asserting list (or Iterable Objects)

Example

List frodos = Arrays.asList(
new TolkienCharacter("Frodo1", 33, HOBBIT),
new TolkienCharacter("Frodo2", 34, MAN));
List sams= Arrays.asList(
new TolkienCharacter("Sam1", 38, HOBBIT);
new TolkienCharacter("Sam2", 39, MAN);
// frodo and sam are equals when ignoring name and age as the only remaining field is race
assertThat(frodos).isEqualToIgnoringGivenFields(sams, "name", "age"); // OK both are HOBBIT

// ... but they are not equals if only age is ignored as their names differ.
assertThat(frodos).isEqualToIgnoringGivenFields(sams, "age"); // FAIL

@joel-costigliola joel-costigliola added the type: improvement A general improvement label Jul 27, 2018
@haruntuncay
Copy link

haruntuncay commented May 15, 2019

Hello @joel-costigliola
Hope you are well.
I would like to contribute to this library. Please inform me whether I can work on this issue or any other issues that I can tackle. Thanks in advance.

@joel-costigliola
Copy link
Member

joel-costigliola commented May 17, 2019

The thing is that assertion is not recursive which I think is what users would expect from it so I don't want to add this improvement to isEqualToIgnoringGivenFields.

Fortunately the new recursive API #1002 has been mostly implemented but not yet the direct support for iterables which is what is requested here. If it did you would be able to write:

List frodos = asList(new TolkienCharacter("Frodo1", 33, HOBBIT), new TolkienCharacter("Frodo2", 34, MAN));
List sams= asList(new TolkienCharacter("Sam1", 38, HOBBIT), new TolkienCharacter("Sam2", 39, MAN);

// frodo and sam are equals when ignoring name and age as the only remaining field is race
assertThat(frodos).usingRecursiveComparison().
                  .ignoringFields("name", "age")
                  .isEqualTo(sams);

@haruntuncay if you feel like it you are welcome to give it a try.

@haruntuncay
Copy link

haruntuncay commented May 27, 2019

Hello @joel-costigliola,

Hope you are doing well. I am going to give this issue a try this week and I wanted to ask your opinion for some pointers on the implementation first.

I was thinking of creating a new class, namely RecursiveIterableComparisonAssert, that extends RecursiveComparisonAssert and delegates the compasion of items contained in the iterables/arrays to RecursiveComparisonDifferenceCalculator just as the RecursiveComparisonAssert does. Apart from isEqualTo method, based on #1002, I believe it should also expose the ObjectEnumerableAssert api or similarly named methods.

Is my understanding of the requested API correct ? And could you please share your opinions and ideas on the design ? Thank you in advance.

@joel-costigliola
Copy link
Member

joel-costigliola commented May 28, 2019

@haruntuncay I think we should try a simpler approach by adding usingRecursiveComparison to AbstractIterableAssert without needing a RecursiveIterableComparisonAssert class.

Let me know if you need further clarifications.

@haruntuncay
Copy link

haruntuncay commented May 29, 2019

I think we should try a simpler approach by adding usingRecursiveComparison to
AbstractIterableAssert without needing a RecursiveIterableComparisonAssert class.

@joel-costigliola, Thank you for the help and please excuse my inadequate design decisions😄 as I am a student and not a seasoned developer. But I really would like to help the project with this issue as it can help me improve a lot.

Therefore, could you please clarify how we can achieve a chain structure like this

assertThat(frodos).usingRecursiveComparison()
                  .withStrictTypeChecking()
                  .ignoringFields("name", "age")
                  .isEqualTo(sams);

since AbstractIterableAssert doesn't expose those methods ? If usingRecursiveComparison returns an instance of RecursiveComparisonAssert, then how can I associateObjectEnumerableAssert methods with the new RecursiveComparisonDifferenceCalculator api ?

Thank you in advance and please excuse me if I am causing a bit of a pain in the head 😄.

@joel-costigliola
Copy link
Member

Try first to add usingRecursiveComparison() to AbstractIterableAssert, then add a test to see if it works as you would expect.

I'm not sure what you mean by associating ObjectEnumerableAssert methods with the RecursiveComparisonDifferenceCalculator, can you give me a concrete example of why it would be needed ?

@haruntuncay
Copy link

haruntuncay commented May 29, 2019

I'm not sure what you mean by associating ObjectEnumerableAssert methods with the RecursiveComparisonDifferenceCalculator, can you give me a concrete example of why it would be needed ?

Sure. For example in here, (without usingRecursiveComparison)

assertThat(fellowshipOfTheRing).contains(frodo, sam, merry, pippin, gandalf, 
                                         legolas, boromir, aragorn, gimli);

the contains method in AbstractIterableAssert currently delegates the call to Iterables.assertContains, which by default uses a StandardCompasionStrategy that essentially boils down to an equals method call.

I thought, by adding usingRecursiveComparison

assertThat(fellowshipOfTheRing).usingRecursiveComparison()
                               .contains(frodo, sam, merry, pippin, gandalf, 
                                         legolas, boromir, aragorn, gimli);

the goal was to use the new RecursiveComparisonDifferenceCalculator for a field by field comparison instead of the old way. Since the contains method is declared in ObjectEnumerableAssert, I thought that they needed to be adapted to use this new field by field comparison api.

Could you please explain where I am mistaken ?

@joel-costigliola
Copy link
Member

joel-costigliola commented May 29, 2019

Alright, I get it now, I was thinking of simply adding the existing usingRecursiveComparison to AbstractIterableAssert but that would only expose isEqualTo, an improvement for sure but as you pointed out users would expect to have iterables assertions after usingRecursiveComparison().

I think an implementation worth exploring is to define (as you suggested) a RecursiveIterableComparisonAssert that will expose methods to configure the recursive comparison behavior (like RecursiveComparisonAssert).

When an assertion is called (eg. contains) it would:

  • create a RecursiveComparisonDifferenceCalculator with the specified RecursiveComparisonConfiguration
  • instantiate a comparator that would use the built RecursiveComparisonDifferenceCalculator
  • call usingElementComparator on an internal IterableAssert and then the assertion.

The comparator is like the existing RecursiveFieldByFieldComparator but implemented with an internal RecursiveComparisonDifferenceCalculator.

I would suggest to write a PoC of RecursiveIterableComparisonAssert that only exposes contains to see how it goes (I don't have yet a clear design for this, it is an exploration phase at the moment).

@haruntuncay
Copy link

Hello @joel-costigliola,

Hope you are well. As you suggested, I have implemented a PoC and it seems to work well. I have created a new branch in my fork of AssertJ and committed here.

Could you please check it out when you have the time ? I would love to hear your feedback on it.

Thanks in advance and hoping to hear from you soon. (btw had to edit the fork because I pushed some unnecessary files as well.)

@joel-costigliola
Copy link
Member

@haruntuncay could you create a PR even if it is not ready ? it is easier to give feedback on a PR

@haruntuncay
Copy link

@joel-costigliola, just wanted to notify that I have created the PR. Thanks in advance for the review.

@joel-costigliola
Copy link
Member

I'm a bit busy on working for releasing 3.13.0, I'll have a look at your PR likely next week. Ping me if you don't hear from me within 2 weeks ;-)

@joel-costigliola joel-costigliola changed the title Add isEqualToIgnoringGivenFields to IterableAssert class Add usingRecursiveComparison for Iterables Dec 15, 2019
@joel-costigliola joel-costigliola added this to the 3.15 milestone Dec 22, 2019
@joel-costigliola
Copy link
Member

I added usingRecursiveComparison in [iterable assertions](basic support for ) but this only provides isEqualTo assertion.

To expose more assertions we need to take the road of RecursiveIterableComparisonAssert, to report differences for existing assertions we have to rewrite iterable assertions inRecursiveIterableComparisonAssert, the other option is to use a comparator implemented with RecursiveComparisonDifferenceCalculator, with this option no need to rewrite assertions but it is hard/impossible to surface the differences in the assertion error messages.

We'll give it shot in 3.16.0

@joel-costigliola joel-costigliola modified the milestones: 3.15, 3.16.0 Jan 14, 2020
@joel-costigliola joel-costigliola removed this from the 3.16.0 milestone Mar 21, 2020
@joel-costigliola
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement A general improvement
Projects
None yet
3 participants