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 a friendly warning to usages of .isEqualTo and .isNotEqualTo on a… #2921

Conversation

ascopes
Copy link
Contributor

@ascopes ascopes commented Jan 15, 2023

…ssertions.

Currently, if I were to perform the following code...

var assertion = assertThat(something);
assertThat(assertion).isEqualTo(assertion);

... I would get an error about the use of .equals being unsupported, and that I should consider using .isEqualTo instead. This is somewhat confusing to the reader, since they ARE using .isEqualTo. This is due to the fact that .isEqualTo internally will invoke calls to .equals on the 'actual' member which will trigger the aforementioned warning.

I have added two new tests to .isEqualTo and .isNotEqualTo that check if the 'actual' member is an assertion itself, and then raise an error if the condition is met. The advisory note is to use .isSameAs and .isNotSameAs instead.

This is useful when using assertj to test custom assertion types for extension libraries, where we may have wanted to use ".satisfies" instead of ".isEqualTo".

An example of where I hit this error:

      // Given
      var assertions = new JavaFileObjectKindAssert(kind);

      // Then
      assertThat(assertions.extension())
          .isInstanceOf(AbstractStringAssert.class)
          .isEqualTo(kind.extension);
java.lang.UnsupportedOperationException: 'equals' is not supported...maybe you intended to call 'isEqualTo'

...of course, what I really meant to do was this:

      // Given
      var assertions = new JavaFileObjectKindAssert(kind);

      // Then
      assertThat(assertions.extension())
          .isInstanceOf(AbstractStringAssert.class)
          .satisfies(extensionAssertion -> extensionAssertion.isEqualTo(kind.extension));

…ssertions.

Currently, if I were to perform the following code...

    var assertion = assertThat(something);
    assertThat(assertion).isEqualTo(assertion);

... I would get an error about the use of .equals being unsupported, and
that I should consider using .isEqualTo instead. This is somewhat confusing
to the reader, since they *ARE* using .isEqualTo. This is due to the fact
that .isEqualTo internally will invoke calls to .equals on the 'actual' member
which will trigger the aforementioned warning.

I have added two new tests to .isEqualTo and .isNotEqualTo that check if the
'actual' member is an assertion itself, and then raise an error if the
condition is met. The advisory note is to use .isSameAs and .isNotSameAs
instead.

This is useful when using assertj to test custom assertion types for extension
libraries, where we may have wanted to use ".satisfies" instead of ".isEqualTo".
@scordio scordio added this to the 3.25.0 milestone Aug 1, 2023
@joel-costigliola
Copy link
Member

Integrated thanks @ascopes and @scordio !

scordio pushed a commit that referenced this pull request Aug 1, 2023
Before this change, the following code:

    AbstractAssert<?, ?> assertion = assertThat(something);
    assertThat(assertion).isEqualTo(assertion);

would throw an exception with a message about the use of `equals` being
unsupported, suggesting to use `isEqualTo` instead. The message is
somewhat confusing to the reader, since `isEqualTo` is indeed used, and
is due to the fact that `isEqualTo` internally relies on the `equals` of
`actual`, which is an `AbstractAssert` instance and throws the
aforementioned exception.

`isEqualTo` and `isNotEqualTo` now check if `actual` is an assertion
instance and raise an `UnsupportedOperationException` if so, with
a message suggesting to use `isSameAs` and `isNotSameAs` instead.

This is, for example, useful for testing custom assertion types for
extension libraries, where the use of `satisfies` instead of `isEqualTo`
may be desirable.

Closes #2921
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

Successfully merging this pull request may close these issues.

None yet

3 participants