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

assertThat(actual).usingRecursiveComparison().ignoringAllOverriddenEquals().isEqualTo(expected) does not verify fields in a parent class #1815

Closed
VasilyV opened this issue Mar 28, 2020 · 7 comments

Comments

@VasilyV
Copy link

VasilyV commented Mar 28, 2020

Summary

I've come across an interesting behavior of recursive comparison in assertj library. If you compare objects of classes that are sub-classes, fields of super classes seem to be skipped during the comparison. Is that a known issue? Or am I doing something wrong? Here is a brief example:

Example

import org.junit.Test;
import static org.assertj.core.api.Assertions.assertThat;

public class ExampleTest {

  @Test
  public void test() {
    MyException expected =
            new MyException("Expected text message");
    MyException actual = new MyException("Actual text message"); //values for the field Throwable.detailMessage are different
    assertThat(actual).usingRecursiveComparison().ignoringAllOverriddenEquals().isEqualTo(expected);
  }
}

class MyException extends RuntimeException {

  MyException(String message) {
    super(message);
  }
}

This test will pass when actually it should not as actual.getMessage() and expected.getMessage() will return different values.

@joel-costigliola
Copy link
Member

Thanks for reporting this, I'm going to have a look at it shortly.

@VasilyV
Copy link
Author

VasilyV commented Mar 29, 2020

Thanks for the prompt response.
Seems I've found the reason. Apparently, the lib skips comparison of the fields that are inherited from superclasses that reside in java.lang. As MyException is using a detailMessage field inherited from java.lang.Throwable, it is skipped. Here is the code from org.assertj.core.internal.Objects that seems to be responsible for such a behavior:

/**
   * Returns the declared fields of given class and its superclasses stopping at superclass in <code>java.lang</code>
   * package whose fields are not included.
   *
   * @param clazz the class we want the declared fields.
   * @return the declared fields of given class and its superclasses.
   */
public static Set<Field> getDeclaredFieldsIncludingInherited(Class<?> clazz) {
    checkNotNull(clazz, "expecting Class parameter not to be null");
    Set<Field> declaredFields = getDeclaredFieldsIgnoringSyntheticAndStatic(clazz);
    // get fields declared in superclass
    Class<?> superclazz = clazz.getSuperclass();
    while (superclazz != null && !superclazz.getName().startsWith("java.lang")) {
      declaredFields.addAll(getDeclaredFieldsIgnoringSyntheticAndStatic(superclazz));
      superclazz = superclazz.getSuperclass();
    }
    return declaredFields;
  }

May I ask why there is such a limitation?

@joel-costigliola
Copy link
Member

Awesome investigation @VasilyV, appreciated.

The reason was to avoid base types to be compared field by field, at some point you have to compare stuff normally.

In your case if we did not do that we would for example compare Throwable.backtrace fields which I think is not relevant (at least for most users).

@VasilyV
Copy link
Author

VasilyV commented Mar 30, 2020

@joel-costigliola Thank you for the nice explanation!

@joel-costigliola
Copy link
Member

You can always use a specific comparator for a given type or field if you need to, see https://assertj.github.io/doc/#assertj-core-recursive-comparison-comparators

@david0
Copy link

david0 commented Apr 9, 2020

See also #1224 (seems to be a duplicate)

@joel-costigliola
Copy link
Member

Closing since an explanation has been given.

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

No branches or pull requests

3 participants