Retain assertTrue/assertFalse(a.equals(null)) as is - #1075
Merged
Conversation
Converting these to assertEquals/assertNotEquals with a null argument silently drops the null-contract test, as Assertions.objectsAreEqual short circuits on a null argument instead of calling a.equals(null). Fixes #1071
AssertionsArgumentOrder moved a null literal into the expected position, which short circuits the equality check instead of calling equals. The argument flip was previously needed to avoid an ambiguous assertThat(null) call in the AssertJ migration; the JUnit assert(Not)Equals to AssertJ recipes now assert on the non-null argument instead. Fixes #1071
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three recipes in the JUnit 4 → 5 pipeline conspired to turn
assertFalse(a.equals(null))intoassertNotEquals(null, a), which never callsa.equals(null)—Assertions.objectsAreEqualshort circuits when its first argument isnull. Tests written to cover the null contract of a customequals()then pass unconditionally.AssertTrueEqualsToAssertEqualsandAssertFalseEqualsToAssertNotEqualsno longer convert when the argument to.equals()is anullliteral (also when parenthesized or cast).AssertionsArgumentOrderno longer reordersassertEquals/assertNotEqualsarguments when either compared argument is anullliteral. Reference assertions such asassertSameare still reordered, as==is symmetric.That argument flip was added in #492 to keep the AssertJ migration compiling:
assertNotEquals(myVariable, null)would otherwise migrate toassertThat(null).isNotEqualTo(myVariable), an ambiguous method call.JUnitAssertEqualsToAssertThatandJUnitAssertNotEqualsToAssertThatnow handle that directly by asserting on the non-null argument —assertThat(myVariable).isNotEqualTo(null)— which both compiles and still callsmyVariable.equals(null)through AssertJ'sObjects.areEqual.Two existing expectations in
CleanupAssertionsTestchanged as a result, both now asserting the more conservative outcome:assertFalse(!"".equals(null))stops atassertTrue("".equals(null))instead of collapsing toassertNull("").assertNotEquals(myVariable, null)is left as is instead of being flipped.