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
Recursive comparison api improvements #1002
Comments
Another option would be to let assertThat(person).usingRecursiveComparison()
.ignoringNullFields()
.ignoringFields("name", "age")
.forcingRecursiveComparisonFor(Child.class, Sister.class)
.isEqualTo(expectedPersont); |
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): So, it would be nice to have a way to clarify that for users, either by changing 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
This way it will be clear for users how the comparison will actually be performed. What do you think ? |
@joel-costigliola |
@joel-costigliola I think you are right. Recursive comparison is one of the best feature of AssertJ and having a dedicated api make sense. |
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 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). |
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. |
Hey, 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. 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. What's your opinion? Do you see pitfalls? |
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. Example
Same too when the object contains some other object :
the 2 object have the same class, but they are composed of different object, and it could be interesting to check this field type. |
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 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 |
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) ? 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). |
I agree that this is both an edge case and likely bad practice.
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.
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. |
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. |
Requirement: |
Requirement: |
I think I'll stick with Or the technique I've been using for fifteen years via XStream:
|
It looks like you are right, I'll look into it tonight and push a fix for it (at least the error message was helpful!). |
@merlin-hst I have (force) pushed the branch with a fix for #1410 if you want to give it a try. |
Now I'm getting the follwing error: java.lang.AssertionError:
Expecting:
<AEntity [id=6]>
to be equal to:
<ADocument@729452f2>
when recursively comparing field by field, but found the following 2 differences:
field/property 'details' differ:
- actual value : [ADetailEntity@6bb01b34, ADetailEntity@374bbe80]
- expected value : [ADetailDocument@381c78ea, ADetailDocument@75cb2090]
field/property 'eventContent' differ:
- actual value : AContentEntity@156aba9a
- expected value : AContentDocument@45dac901
The recursive comparison was performed with this configuration:
- the following fields were ignored in the comparison: id, isMarkedForTermination
- overridden equals methods were used in the comparison
- these types were compared with the following comparators:
- java.lang.Double -> DoubleComparator[precision=1.0E-15]
- java.lang.Float -> FloatComparator[precision=1.0E-6]
- actual and expected objects and their fields were compared field by field recursively even if they were not of the same type, this allows for example to compare a Person to a PersonDto (call strictTypeChecking(true) to change that behavior). Based on that message it looks like that for associated objects the types are compared. But I didn't take a look into the code. |
@merlin-hst By default overridden equals are used to compare instances, this can be disabled by fields or type at the moment with I probably should add an |
1 similar comment
@merlin-hst By default overridden equals are used to compare instances, this can be disabled by fields or type at the moment with I probably should add an |
I've seen that. But I don't have overriden equals. Nevertheless having |
Interesting, would you be able to provide a test to reproduce this ? |
I'll try it. If it helps my objects look like: class AEntity extends AbstractEntity{
private Set<ADetailEntity> details;
private AContentEntity content;
//....
}
class ADocument extends BaseDocument {
private Set<ADetailDocument> details;
private AContentDocument content;
//...
} None of them have equals ovverriden. Even not the base classes. |
I'm keen to see the more details on the classes you deal with, I have this test passing which compares different types and no equals method: @Test
public void should_pass_by_default_when_objects_data_are_equals_whatever_their_types_are() {
// GIVEN
Person actual = new Person("John");
actual.home.address.number = 1;
actual.dateOfBirth = new Date(123);
actual.neighbour = new Person("Jack");
actual.neighbour.home.address.number = 123;
actual.neighbour.neighbour = new Person("James");
actual.neighbour.neighbour.home.address.number = 124;
PersonDto expected = new PersonDto("John");
expected.home.address.number = 1;
expected.dateOfBirth = new Date(123);
expected.neighbour = new PersonDto("Jack");
expected.neighbour.home.address.number = 123;
expected.neighbour.neighbour = new PersonDto("James");
expected.neighbour.neighbour.home.address.number = 124;
// THEN
assertThat(actual).usingRecursiveComparison()
.isEqualTo(expected);
} |
Taking a quick look into the test class and the data objects it seems to me that |
^^ I actually have updated the test to have Dto versions of all types ( |
I've made a simple demo to reproduce the mentioned error. If you're interested in I can provide it as maven project. But in the meantime it's getting more cleaner to me: I've just found another issue (?):
By using a pattern like Hoe that helps |
@merlin-hst a small project would be great or if it is simpler just create a PR on top of this branch. I'll look into the stack overflow error, thanks again for you feedback. |
@merlin-hst I have pushed a fix for the recursive error, the test you gave me fails with the error below.
Isn't that achievable with regexes ?
I did not understand this one, can you clarify with an example ?
|
I'll provide you a working version. But as I already mentioned in my opinion the error message is a little bit to abstract. It indicates that the fields 'content' and 'details' of AEntity and ADocument differs. But because both fields are complex objects I'd like to know why they differ. When I inspect the ComparisonDifference objects in the debugger I can see that there is a field called 'additionalInformation' with some more information about the difference, for example what exactly is missing (AContentDocument does not declare all AContentEntity fields, it lacks these:[id]]). So for me it looks like that the information of these field should be available too in the AssertError message. Second using 'ignoringFieldsMatchingRegexes' could be sometimes too much. When I'd like to ignore a field called 'id' regardless of the path I can use '.*id' as pattern. But that would match all fields ending with 'id' which might not what I wanted. So from my point of view an option to ignoring specific fields regarding of the path could be very convenient. |
Hi,
It would be great to implement something like this:
Also, I noticed that assert error message for complex objects is not precise. If you have only one difference in child of childs of the object you get the first level child in the assert error message. It's too hard to find where is the error
|
Hello, first of all amazing framework ! I'd like to know if it is possible, when asking for a strict type checking, to discard it for lambda expressions ? Also is there a way to provide a custom comparator for some fields matching a regex ? The regex builder for discarding some fields is very useful as I am now dealing with a code that has some cyclical pointing in it, but I would need the same kind of logic for applying a custom comparator to those fields. Is there already a way to do so please ? Currently I have a work around with just discarding them in a first place and then looking for them explicitly but this is hardly extensible. Thank you very much ! |
hey thanks for the kind words!
it's not supported at the moment, can you give us a concrete use case? (with an example if possible)
Not supported, but we can add that, same as my other comment, create a new issue for it and provide a concrete example (will be helpful for testing) |
closing this as the recursive comparison has reached a decent state |
Recursive comparison api
Here's an example to give users an idea of what it looks like:
This will be in Beta as it might change according to people's feedback.
Requirements
Recursive comparison Fluent API configuration
Implements an easy to use/discover API as in:
Ignoring fields in comparison
Users can specify the comparison to ignore all null fields in the object under test.
Users can specify to ignore specific fields in the object under test:
person.sister.name
.*name
meansname
,person.name
andperson.sister.name
are ignored.Note if you ignore specific field, all sub fields are ignored too, for example if
person
is ignored thenperson.name
,person.address
,person.address.number
are ignored too.Nice to have: report all the fields ignored in the comparison
status
Allow to use overridden
equals
method or notBy default the recursive comparison will use
equals
if it had been overridden but users should be able to specify to ignore overriddenequals
methods and compare objects field by field.This can be done:
forcingRecursiveComparisonFor(Child.class, Sister.class)
forcingRecursiveComparisonForFields("child", "sister")
Nice to haveExample:
status
equals
method for types matching given regexesequals
method for given typesequals
method for given fieldsequals
method for all fieldsSpecify 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 typesusingComparatorForFields(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.
Once a comparator is registered for a type or a field, it replaces the recursive comparison when these types/fields are being compared.
status
Strict/lenient type checking
This will specify if two instances with the same fields but from different classes are considered equals, it allows for example to compare a
Person
with aPersonDto
. If the check is strict the expected object class must be compatible (i.e. extends) actuals, so ifEmployee
inheritsPerson
one can compare a person with an employee (but not an employee with a person. The check is performed on the root objects and their fields.By default the check is lenient but it can be made strict to fail the comparison.
status
TreeSet
vsHashSet
).Handle cycles
The recursive comparison should handle cycles, for example when a -> b -> c -> a.
status
Map support
Maps entries should be considered as fields.
See if it is possible and relevant to report all compared maps internal differences (size, missing elements, elements differences, order when order is consistent ...) instead just reporting that the maps differ.
status
On hold at the moment as it is not crystal what can be achieved reasonably.
Iterable/array support
At the moment
assertThat(Iterable/array)
does not exposeusingRecursiveComparison()
which makes it cumbersome to test them field/field.The comparison by default fails if we compare an ordered collection to an unordered one but this should be configurable
Nice to have: ignores order in comparison (by default elements are compared in order for ordered collection).
Nice to have: allow to compare ordered collections with unordered one. (this should be disabled by default)
Nice to have: allow to compare array with ordered collections like list. (this should be disabled by default)
Example:
status
Error reporting
A failure must report the recursive comparison specification:
equals
are used and not usedThe 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 when strict type checking is enable.
The differences reported must be ordered by path alphabetically.
Difference report example:
The path to field must support maps and adress #1303.
status
Globally configuring the recursive comparison behavior
To avoid repeating the same recursive configuration before each assertion, AssertJ should provide:
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:
The example above is equivalent to this where we have to repeat call to
ignoringNullFields
andforcingRecursiveComparisonFor
for each assertions:status
Tests checklist
The recursive comparison specification must be transferred after calling methods that change the object under test:
extracting
,asString
...status
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 !)
The text was updated successfully, but these errors were encountered: