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

New recursive comparison api #1002

Open
joel-costigliola opened this Issue May 29, 2017 · 15 comments

Comments

8 participants
@joel-costigliola
Owner

joel-costigliola commented May 29, 2017

Recursive comparison api

Here's an example to give users an idea of what it looks like:

assertThat(person).usingRecursiveComparison()
                  .ignoringNullFields()
                  .ignoringFields("name", "age")                                                 
                  .forcingRecursiveComparisonFor(Child.class, Sister.class)
                  .isEqualTo(expectedPerson);

This will be in Beta as it might change according to people's feedback.

Requirements

Ignoring fields in comparison

Users can specify to ignore all in the object under test null fields.

Users can specify to ignore specific fields in the object under test:

  • giving their full path, ex: person.sister.name
  • their name wherever they are in the graph, ex: ..name means name, person.name and person.sister.name are ignored.

Allow to use overridden equals method or not

By default the recursive comparison will use equals if it had been overridden but users should be able to specify to ignore overridden equals methods and compare objects field by field.

This can be done:

  • for all types with forcingRecursiveComparisonForAllTypes
  • a specific given list of types with forcingRecursiveComparisonFor(Child.class, Sister.class)
  • a specific given list of fields with forcingRecursiveComparisonForFields("child", "sister") Nice to have

Example:

assertThat(person).usingRecursiveComparison()
                  .forcingRecursiveComparisonFor(Child.class, Sister.class)
                  .forcingRecursiveComparisonForFields("name", "city")
                  .isEqualTo(expectedPerson);

Specify a comparison strategy per type or fields

Users should be able to specify a comparator for a given type or fields:

  • usingComparatorForType(String.class, caseInsensitiveComparator) for all types
  • usingComparatorForFields(caseInsensitiveComparator, "name", "city") a specific given list of fields (vararg)

To keep the API simple once a comparator for a given type is registered it should be used at any level, collection element or collection element field.

Field comparators take precedence over type ones as they are more fine grained.

Strict/lenient type checking

This will specify if two instances with the same fields but from different classes are considered as equals.

By default the check is strict but it can be made lenient to avoid failing the comparison. (this can be discussed)

Handle cycles

The recursive comparison should handle cycles, for example when a -> b -> c -> a.

Map support

Maps entries should be considered as fields.

Iterable support

It should be possible to compare elements recursively.
Nice to have: in order or not (by default elements are compared in order).

Example:

// other options:
// - usingRecursiveComparison()
// - comparingElementsFieldByFieldRecursively()
assertThat(fellowshipOfTheRing).usingRecursiveComparisonForElements()
             .contains(frodo, sam, merry, pippin, gandalf, legolas, boromir, aragorn, gimli);

Error reporting

A failure must report the recursive comparison specification:

  • comparators used per type
  • comparators used per fields
  • ignored fields
  • when overridden equals are used and not used

The failure should show the number of differences and describe them all.

A difference describes fields by their full path from the root object, the actual value, the expected one and the comparison used if the user specified one.
Values are represented with AssertJ standard representation or the one set by the user.

Actual and expected values type should be displayed in type checking was set to be strict.

Difference report example:

person.sister.name differs: 
- actual value  : "Sansa"
- expected value: "Arya"
- comparison was performed with caseInsensitiveComparator 

The path to field must support maps and adress #1303.

status

  • basic error reporting
  • error reporting : show comparators used
  • error reporting : list ignored fields
  • error reporting : show that actual's null field were ignored in the comparison
  • error reporting : show which overridden equals were used
  • error reporting : the path to field must support maps

Globally setting the recursive comparison behavior

To avoid repeating the same recursive configuration before each assertion, AssertJ should provide:

  • a global way to configure the default recursive comparison behavior
  • a way capture easily on the recursive comparison configuration in order to reuse it

Changing the default recursive comparison behavior before all tests

AssertJ will extend the mechanism used to configure Representation.

Capture the recursive comparison setting to reuse it

If users don't want to change the default behavior globally but on a smaller scope, it will possible to capture a recursive comparison specification and reuse it.

Example:

// default behavior
RecursiveComparisonSpecification recursiveComparisonSpecification =  new RecursiveComparisonSpecification();
recursiveComparisonSpecification.ignoreNullFields();
recursiveComparisonSpecification.forceRecursiveComparisonFor(Child.class, Sister.class);

assertThat(person).usingRecursiveComparison(recursiveComparisonSpecification)
                  .isEqualTo(expectedPerson);

assertThat(person2).usingRecursiveComparison(recursiveComparisonSpecification)
                   .isEqualTo(expectedPerson2);

The example above is equivalent to this where we have to repeat call to ignoringNullFields and forcingRecursiveComparisonFor for each assertions:

assertThat(person).usingRecursiveComparison()
                  .ignoringNullFields()
                  .forcingRecursiveComparisonFor(Child.class, Sister.class)
                  .isEqualTo(expectedPerson);

assertThat(person2).usingRecursiveComparison(recursiveComparisonSpecification)
                   .ignoringNullFields()
                   .forcingRecursiveComparisonFor(Child.class, Sister.class)
                   .isEqualTo(expectedPerson2);

Tests checklist

The recursive comparison specification must be transferred after calling methods that change the object under test: extracting, asString ...


Initial issue description

The current way comparing objects recursively is starting to pollute the api, the methods introduced for the recursive comparison don't apply for the other assertion cluttering the assertj api, ex usingComparatorForType.

I would like to introduce a separate API to use recursive comparison allowing to fine tune the comparison behavior.

API rough draft (please criticize it !)

import static RecursiveComparisonSpecification;

// ignoringNullFields() comes from RecursiveComparisonSpecification
assertThat(person).usingRecursiveComparison(ignoringNullFields()
                                           .ignoringFields("name", "age")                                                 
                                           .forcingRecursiveComparisonFor(Child.class, Sister.class))
                  .isEqualTo(expectedPersont)
@PascalSchumacher

This comment has been minimized.

Collaborator

PascalSchumacher commented May 29, 2017

Another option would be to let usingRecursiveComparison return a special Assert class (e.g. RecursiveComparisonAssert), so you could write

assertThat(person).usingRecursiveComparison()
     .ignoringNullFields()
     .ignoringFields("name", "age")                                                 
     .forcingRecursiveComparisonFor(Child.class, Sister.class)
     .isEqualTo(expectedPersont);
@ylegat

This comment has been minimized.

ylegat commented May 29, 2017

I do like the design! But there is something that, I believe, is misleading (actually it is already the case with current version of AssertJ): usingRecursiveComparison expression does not say anything about the fact that the recursive comparator will not be used on attributes having a type overidding equals. Of course, it becomes clear if we introduce a forcingRecursiveComparisonFor method, but all users won't see it at first glance.

So, it would be nice to have a way to clarify that for users, either by changing usingRecursiveComparison to something else, either by forcing users to call a specific method in order to get a RecursiveComparisonSpecification instance. Going a step forward: maybe that specific method could be used to specify if the recursive comparison should be forced on any type ?

Here is my proposal:

import static RecursiveComparisonSpecificationFactory;

// forcingRecursiveComparisonForAll() comes from RecursiveComparisonSpecificationFactory
assertThat(person).usingRecursiveComparison(forcingRecursiveComparisonForAll()
                                           .ignoringFields("name", "age"))
                  .isEqualTo(expectedPerson)

// forcingRecursiveComparisonFor() comes from RecursiveComparisonSpecificationFactory
assertThat(person).usingRecursiveComparison(forcingRecursiveComparisonFor(Child.class, Sister.class)
                                           .ignoringFields("name", "age"))
                  .isEqualTo(expectedPerson)

You can only get a RecursiveComparisonSpecification instance by calling one of two methods on RecursiveComparisonSpecificationFactory:

  • forcingRecursiveComparisonFor
  • forcingRecursiveComparisonForAll

This way it will be clear for users how the comparison will actually be performed. What do you think ?

@PascalSchumacher

This comment has been minimized.

Collaborator

PascalSchumacher commented May 29, 2017

@joel-costigliola usingComparatorForType and usingComparatorForFields are also used by the non-recursive field/property by field/property comparison methods (isEqualToComparingFieldByField, isEqualToIgnoringNullFields, isEqualToComparingOnlyGivenFields and isEqualToIgnoringGivenFields). In order to remove them from ObjectAssert we would have to add a similar API for non-recursive comparison.

@pierrefevrier

This comment has been minimized.

pierrefevrier commented May 29, 2017

@joel-costigliola I think you are right. Recursive comparison is one of the best feature of AssertJ and having a dedicated api make sense.
It would be nice to be able to explain the way recursive comparison should work for some attributes (for a given depth).
Maybe give a list of rules for each of those specials attributes could be a way to do so ?

@joel-costigliola

This comment has been minimized.

Owner

joel-costigliola commented May 30, 2017

thank you all for your feedback, the api and its behaviour are still opened to discussion and changes, let's continue brainstorming it.

I wonder if the field by field comparison should be the default even for classes having overriding equals, I'd like to have your opinion on this one guys. In that case we could add a useOverriddenEquals to change that maybe with whitelist/blacklist (if that's not much).

I like @PascalSchumacher suggestion, it will make the API easier to discover, we will go that way except if we encounter technical limitations.

@pierrefevrier can you elaborate ? do you refer to add information in the error message explaining how the comparison was performed and (obviously) what went wrong ? (yes we will do this).

@PascalSchumacher

This comment has been minimized.

Collaborator

PascalSchumacher commented May 30, 2017

I think the custom equals of classes should be used by default, because otherwise it is a pain to compare two objects e.g. containing a collection object.

@jomora

This comment has been minimized.

jomora commented Jul 3, 2017

Hey,
what do you think about the possibility to decide whether field-by-field comparison or comparison using equals() for certain types or attributes should be used?

For types one could specify the classes similar to @PascalSchumacher's example:

assertThat(person).usingRecursiveComparison()
     .ignoringNullFields()
     .ignoringFields("name", "age")                                                 
     .forcingRecursiveFieldByFieldComparisonFor(Child.class, Sister.class)
     .forcingRecursiveEqualityComparisonFor(Mother.class, Father.class)
     .isEqualTo(expectedPersont);

For attributes the information in which type the attribute is located might be useful.
What about something like of() or memberOf():

assertThat(person).usingRecursiveComparison()
     .ignoringNullFields()
     .ignoringFields("name", "age")                                                 
//  of() and in() are aliases for memberOf()
     .forcingRecursiveFieldByFieldComparisonForField("sister").of(Person.class)
     .forcingRecursiveEqualityComparisonForField("child").memberOf(Father.class)
     .forcingRecursiveEqualityComparisonForField("mother").in(Child.class)
     .isEqualTo(expectedPersont);

This would result in the equality-comparison being used for the child attribute of class Father, but not for class Mother.
I think sth. in this direction could work out, but I surely haven't thought through it completely.

What's your opinion? Do you see pitfalls?

@GaspardPO

This comment has been minimized.

GaspardPO commented Aug 10, 2017

As explained in #1054 :

when using isEqualToComparingFieldByFieldRecursively or isEqualToComparingFieldByField, the comparison is not done on class type, only on field value (and name).

So two POJO with the same fields, but from different classes are considered as equals.
It could be useful to test the object instance (recursively, or not)

Example

@Test
 public void test() {
     A a = new A(10);
     B b = new B(10);
     assertThat(a).isEqualToComparingFieldByFieldRecursively(b); 
// I would like this line to fail because A and B are different classes
// but it is not failing
 }

 private static class A {
     int x;

     public A(int x) {
         this.x = x;
     }
 }

 private static class B {
     int x;

     public B(int x) {
         this.x = x;
     }

 }

Same too when the object contains some other object :

  private static class Something {
        InnerObject inner;  // can be A or B

        public Something(InnerObject inner) {
            this.inner = inner;
        }
    }


Something someObjectContainingA = new Something(new A(10));
Something someObjectContainingB = new Something(new B(10));
assertThat(someObjectContainingA).isEqualToComparingFieldByFieldRecursively(someObjectContainingB); 

the 2 object have the same class, but they are composed of different object, and it could be interesting to check this field type.

@tloist

This comment has been minimized.

tloist commented Nov 9, 2017

Thanks for the great library.

I think I can also add an edge case: Shadowing variables.

public class ShadowedValuesAreComparedFlat {

  public class Super {
    private int x;

    public Super(int x) {
      this.x = x;
    }
  }

  public class Sub extends Super {
    private int x;

    public Sub(int superValue, int subValue) {
      super(superValue);
      this.x = subValue;
    }
  }

  @Test
  public void shadowed_values_are_not_considered() {
    Super instanceSuper = new Super(10);
    Sub instanceSub = new Sub(20, 10);
    // Passes the test: Should it fail instead?
    assertThat(instanceSub).isEqualToComparingFieldByFieldRecursively(instanceSuper);
  }
}

here Sub contains two values of x on two different levels.
I think the current implementation takes the first one it finds on each and compares them. So it ends up comparing super.x with sub.x, which are both 10.

I am not quite sure what the expectation would be. It depends if one wants to compare super with subclasses at all and if yes, what should be the policy to do it.

Status Quo in assertj-core 3.8.0 for the above is: the test passes.
As this ticket is about redesigning the recursive comparison, I think this would be the right place to make you aware and possible discuss, what should be the output.

@joel-costigliola

This comment has been minimized.

Owner

joel-costigliola commented Nov 12, 2017

That's really an edge case ! Thanks for reporting it.

The philosophy of the recursive is to provide a data comparison of actual and expected but should it look at the data values only or also at the data graph structure (i.e. where do the data values come from) ?
In the "the data values only" case, the assertion should pass because if you ask the subclass instance for its x value it will always return its own and not super.x so from this point of view super.xis like it does not exist.
If we want to consider the data structure then the assertion should fail because Sub has two fields whereas Super only one. We could also go further and only compare instances of the same class only.

I'm not overly opinionated on this but I think that we should go data values only and make sure we get the values from the subclasses field first (which we do), I also think that edge case comes from a bad coding practice (why anyone would shadow a parent field ?) and thus I don't think we should cater for it (I'm happy to change my mind if you can show me a relevant use case for shadowing fields).

@tloist

This comment has been minimized.

tloist commented Nov 12, 2017

I agree that this is both an edge case and likely bad practice.
My intention was to make you aware of it, to make an aware decision for the new API. Depending on the definition of equals in a class doing this, it could or could not be consistent with what isEqualToComparingFieldByFieldRecursively is doing.

We could also go further and only compare instances of the same class only.

If you go down this path, this becomes a non-issue. However in a previous issue I saw you opting for not walking that path, though I am not sure if this holds true for the new API as well?

In the case where subclassing is allowed, one needs a policy for dealing with this.

  • Differentiate between equal named variables steeming from different classes
  • Regard only the outer appearance of the objects
  • Make it configurable with the downside of complicating the API

I also have no strong opinion about it. Normally I would go for "do not consider different classes as equal" to begin with. That that's a strong opinionated approach which is maybe not suitable for a library which wants to support different developer.

My gut feeling is that I would not support this and shy away from the effort of making a path dependent aware equality check and would just support the "no subclassing at all" and "subclassing without shadowing" cases.

However I think it might be worth documenting that this case is not supported.

@joel-costigliola

This comment has been minimized.

Owner

joel-costigliola commented Nov 14, 2017

I'm not keen of verifying instances are of the same class since it is something that users can do easily if they want to before performing the recursive comparison.

As you suggested, the behavior should be clearly documented.

@joel-costigliola

This comment has been minimized.

Owner

joel-costigliola commented Nov 26, 2017

Requirement:
To keep the API simple once a comparator for a given type is registered it should be used at any level, collection element or collection element field. At the moment we can register comparators that are only applied at the collection element field level and others that are applied at both element and element's field levels - too complicated !

@joel-costigliola

This comment has been minimized.

Owner

joel-costigliola commented Nov 26, 2017

Requirement:
Come with good description to clearly state which comparators were used.

@paul-hammant

This comment has been minimized.

paul-hammant commented Apr 6, 2018

isDeepEqualTo() is what I was looking for in AssertJ when I arrived at #1002.

I think I'll stick with DeepEquals from https://github.com/jdereg/java-util

Or the technique I've been using for fifteen years via XStream:

assertEqual(xstream.toXML(expected), xstream.fromXML(actual));

@joel-costigliola joel-costigliola added this to the 3.10 milestone Apr 6, 2018

@joel-costigliola joel-costigliola modified the milestones: 3.10, 3.11 May 12, 2018

@joel-costigliola joel-costigliola modified the milestones: 3.11, 3.12 Jul 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment