Apply AssertJ best practices #1031
Conversation
There was a problem hiding this comment.
Pointed out some of the changes and why; the neat thing here is that you can also have these pointed out to you by a bot on new PRs: https://www.moderne.ai/blog/stop-breaking-ci-annotate-prs-with-openrewrite-recipe-fixes-as-quality-gate
| @@ -18,8 +18,7 @@ void copyHappyPath() { | |||
| Object original = instantiate(SimpleRecord.class); | |||
| Object copy = copyOf(original); | |||
|
|
|||
| assertThat(copy).isNotSameAs(original); | |||
| assertThat(copy).isEqualTo(original); | |||
| assertThat(copy).isNotSameAs(original).isEqualTo(original); | |||
There was a problem hiding this comment.
You can chain assertions on the same element; I think that looks best with each on a new line, but spotless disagrees.
There was a problem hiding this comment.
I agree it would look better that way. It's hard to find a good formatter configuration that works well in all situations...
There was a problem hiding this comment.
Trailing // to the rescue 😄
| assertThat(copy).isNotSameAs(original).isEqualTo(original); | |
| assertThat(copy) // | |
| .isNotSameAs(original) | |
| .isEqualTo(original); |
There was a problem hiding this comment.
Haha, that's not a bad workaround, I'll keep it in mind 😄
| @@ -241,7 +241,7 @@ void succeed_whenCallingForPackageRecursivelyOnAPackageContainingFailingClasses_ | |||
| void succeed_whenReportingOnSeveralCorrectClasses() { | |||
| List<EqualsVerifierReport> reports = EqualsVerifier.forClasses(A.class, B.class, C.class).report(); | |||
|
|
|||
| assertThat(reports.size()).isEqualTo(3); | |||
| assertThat(reports).hasSize(3); | |||
There was a problem hiding this comment.
Now when this fails you'll get a message saying "a collection was expected with three elements, but it was a collection with x elements, containing elem1, elem2, ....". Much more helpful than expected 3 but was 5.
| @@ -53,7 +53,7 @@ void allValuesReturnToOriginalState_whenEqualsVerifierIsFinishedWithException() | |||
| } | |||
| catch (AssertionError e) { | |||
| // Make sure EV fails on a check that actually mutates fields. | |||
| assertThat(e.getMessage().contains("Mutability")).isTrue(); | |||
| assertThat(e.getMessage()).contains("Mutability"); | |||
There was a problem hiding this comment.
The assertion failure here would go from expected true was false to expected a string containing Mutability, but was ....
| @@ -59,7 +59,7 @@ void onlySubClassFields() { | |||
| @Test | |||
| void noFields() { | |||
| FieldIterable iterable = FieldIterable.of(NoFields.class); | |||
| assertThat(iterable.iterator().hasNext()).isFalse(); | |||
| assertThat(iterable.iterator()).isExhausted(); | |||
There was a problem hiding this comment.
Again; we're being more expressive about what we expect, such that you get better failure messages.
|
Thank you! I was already doing some of this stuff manually, I didn't even think to look for a recipe like. Only proves my point that I haven't even scratched the surface of either OpenRewrite or AssertJ 😄 |
|
I'll take a look at that PR bot later, that looks very promising! |
What problem does this pull request solve?
Apply AssertJ best practices, such that you get more expressive failure messages.
What alternatives did you consider?
Not making these changes.
Please provide any additional information below
Ran via
As per https://docs.openrewrite.org/recipes/java/testing/assertj/assertj-best-practices