Compare only fields of given type #2794
Conversation
Short descriptionIntroduce new field in Recursive Comparison Configuration - set of types to be compared, and include it to existing places (description, equals, etc.) Keep track of parent fields in separate field. Since when matching We should evaluate matching of the existing This change does not break any of existing tests so it's safe I belive. |
|
Thanks, I will review your PR in the next few days |
There was a problem hiding this comment.
Thanks @beyond-danube nice PR, first round of code review done.
| private boolean shouldBeComparedBasedOnFieldValue(DualValue dualValue) { | ||
| if (parentDualValuePathsComparedFieldsOfTypes.stream().anyMatch(path -> dualValue.fieldLocation.getPathToUseInRules().startsWith(path))) return matchFiledComparedByTypeAndStorePath(dualValue); | ||
| if (dualValue.actual != null && comparedFieldsOfTypes.contains(dualValue.actual.getClass())) return matchFiledComparedByTypeAndStorePath(dualValue); | ||
| if (dualValue.expected != null && comparedFieldsOfTypes.contains(dualValue.expected.getClass())) return matchFiledComparedByTypeAndStorePath(dualValue); |
There was a problem hiding this comment.
I think we should only evaluate dualValue.expected when dualValue.actual is null as done in matchesAnIgnoredFieldType, the rationale is that actual is the object which we filter fields for (not expected).
Would be good to add a note in the javadoc of RecursiveComparisonAssert.comparingOnlyFieldsOfTypes
There was a problem hiding this comment.
Added javadoc on how is compared type evaluated.
We don't evaluate dualValue.expected always now, we'll get to the dualValue.expected's if statement only if dualValue.actual is null
However we can remove matchFiledComparedByTypeAndStorePath(DualValue dualValue) method, which only adds FieldLocation to parent compared fields set and always returns true and modify shouldBeComparedBasedOnFieldValue(DualValue dualValue) to something like:
private boolean shouldBeComparedBasedOnFieldValue(DualValue dualValue) {
if (fieldLocationOfComparedFieldsOfTypes.stream().anyMatch(dualValue.fieldLocation::hasParent)) {
fieldLocationOfComparedFieldsOfTypes.add(dualValue.fieldLocation);
return true;
}
if (dualValue.actual != null && comparedFieldsOfTypes.contains(dualValue.actual.getClass())) {
fieldLocationOfComparedFieldsOfTypes.add(dualValue.fieldLocation);
return true;
}
if (dualValue.expected != null && comparedFieldsOfTypes.contains(dualValue.expected.getClass())) {
fieldLocationOfComparedFieldsOfTypes.add(dualValue.fieldLocation);
return true;
}
return false;
}So that might be more readable.
To be honest I don't have any strong argument for any approach here so we can just pick one - more obvious or less repetitiveness.
| @@ -792,6 +835,19 @@ private boolean matchesAnIgnoredFieldType(DualValue dualValue) { | |||
| return false; | |||
| } | |||
|
|
|||
| private boolean shouldBeComparedBasedOnFieldValue(DualValue dualValue) { | |||
| if (parentDualValuePathsComparedFieldsOfTypes.stream().anyMatch(path -> dualValue.fieldLocation.getPathToUseInRules().startsWith(path))) return matchFiledComparedByTypeAndStorePath(dualValue); | |||
There was a problem hiding this comment.
FieldLocation has methods hasParent and hasChild that I believe should be used here, need to change parentDualValuePathsComparedFieldsOfTypes to store FieldLocation
anyMatch(parentFieldLocation -> dualValue.fieldLocation.hasParent(parentFieldLocation))
if we do that, we could rename parentDualValuePathsComparedFieldsOfTypes to fieldLocationOfComparedFieldsOfTypes (feels more expressive to me but if you have better suggestions I'm all ears)
There was a problem hiding this comment.
Totally agree here, seems that I have just missed this capability in my initial implementation. It's more obvious way to store filed locations indeed so I have done that.
| * {@code comparingOnlyFieldsOfTypes} can be combined with ignoring fields or compare only fields by name methods to restrict further the fields actually compared, | ||
| * the resulting compared fields = {specified compared fields of types} {@code -} {specified ignored fields}.<br> | ||
| * For example if the specified compared fields of types = {String.class, Integer.class, Double.class}, when there are fields String foo, Integer buzz and Double bar | ||
| * and the ignored fields = {"bar"} then only {"foo", "baz"} fields will be compared. |
There was a problem hiding this comment.
nice to state the behavior of combined methods :)
|
|
||
| // THEN | ||
| verifyShouldBeEqualByComparingFieldByFieldRecursivelyCall(actual, expected, | ||
| differences); |
|
@joel-costigliola thanks for the review! I will go through your points in the nearest few days |
|
Hi @joel-costigliola I did go though your points and added changes on all of them, also spotted a typo and fixed that as well. Please have a look at the changes and this part. |
…dsOfTypes new methods in RecursiveComparisonAssert and RecursiveComparisonConfiguration to compare only fields of given type
…figuration Builder test
…tiline description test
…tions instead of field path
…h comparingOnlyFieldsOfTypes
…ert.comparingOnlyFieldsOfTypes on how actual's and expected's fields are matched for comparisons
…ert.comparingOnlyFieldsOfTypes
joel-costigliola
force-pushed
the
compare-only-fields-of-given-type
branch
from
a767fbe
to
6862791
Compare
|
Integrated thanks @beyond-danube! |
|
Great, thanks @joel-costigliola! |
Check List: