Repository navigation
Skip Hamcrest matchers that take nested matchers in HamcrestMatcherToAssertJ - #1236
Merged
Merged
Conversation
…erToAssertJ` The three-argument `assertThat(reason, actual, not(matcher))` path only removed the `org.hamcrest.Matchers` static imports, so code that imported `not` and the matcher from `CoreMatchers` was left with unused Hamcrest imports after the migration. The two- and three-argument paths are now one code path that only differs in the `.as(reason)` call, so both remove the same imports.
…oAssertJ`
The `*(org.hamcrest.Matcher)` exclusion only matched single-matcher overloads, not the `Matcher...` varargs, `List<Matcher>` or two-matcher `hasEntry` overloads, so `assertThat(list, contains(equalTo("a"), startsWith("b")))` became `assertThat(list).containsExactly(equalTo("a"), startsWith("b"))`, which compares the elements against Hamcrest matcher objects and fails (or does not compile). `HamcrestMatcherToAssertJ` and `HamcrestNotMatcherToAssertJ` now leave the call alone when any parameter or argument of the matcher is a `Matcher`, an array of matchers or an `Iterable` of matchers.
…atcherToAssertJ` Hamcrest infers `T` from the array, so `assertThat(list, containsInAnyOrder(states.toArray()))` compiles for a `List<String>`, but AssertJ's `containsExactlyInAnyOrder(String...)` does not accept an `Object[]`. The test for this case locked in the non-compiling output and hid it with `methodInvocations(false)`. The recipe now leaves the call alone when an array passed to a varargs matcher has a component type that is not assignable to the element type of the actual value, and the test covers a `String[]` that does convert.
23 of 76 tasks
…xpectation `VariableReferences` in rewrite-static-analysis now matches identifiers semantically, so the `Save.text` field no longer counts as a use of the `text` parameter.
…skip-nested-matchers-v1
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
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.
HamcrestMatcherToAssertJandHamcrestNotMatcherToAssertJexcluded nested matchers with a*(org.hamcrest.Matcher)pattern, which misses theMatcher...varargs,List<Matcher>and two-matcherhasEntryoverloads. SoassertThat(list, contains(equalTo("a"), startsWith("b")))becameassertThat(list).containsExactly(equalTo("a"), startsWith("b")), which compares the elements against matcher objects and fails (or doesn't compile). The same happened forcontainsInAnyOrder,hasItems,arrayContaining,hasEntryandnot(hasItems(..)). Both recipes now leave the call alone when any parameter or argument of the matcher is aMatcher, an array of them or anIterableof them.The existing
containsInAnyOrderWithArraytest locked inassertThat(list).containsExactlyInAnyOrder(states.toArray())for aList<String>, which doesn't compile, and hid that withmethodInvocations(false). The template stub is typed with raw types, so type validation can't catch this.HamcrestMatcherToAssertJnow also skips a varargs matcher whose array argument has a component type that isn't assignable to the actual's element type. The test now converts aString[]and keepsObject[]as a no-change case, without the relaxation. A separate commit merges the two- and three-argument paths ofHamcrestNotMatcherToAssertJ, so the reason path now also removesCoreMatchers.notand the matcher import.Verified with
./gradlew test --tests 'org.openrewrite.java.testing.hamcrest.*' --tests 'org.openrewrite.java.testing.assertj.*': 695 tests, 1 failure. The failure isMigrateHamcrestToAssertJTest$Dependencies.assertjGradleDependencyAddedWithTestScope, a Tooling API test that can't resolve its Gradle classpath locally (Maven Central 429), so it's an environment problem and not caused by this change. The 7 new regression tests fail without the fix.Fixes item E3 of #1211.