From 1af46453f6f47f5638976b81d35dcf53a76a295a Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Thu, 11 Apr 2024 21:47:15 +0200 Subject: [PATCH 1/9] Introduce `StepVerifierVerifyLater` Refaster rule --- .../errorprone/refasterrules/ReactorRules.java | 13 +++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 4 ++++ .../refasterrules/ReactorRulesTestOutput.java | 4 ++++ 3 files changed, 21 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index d1202f83f4..cc233488a3 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1768,6 +1768,19 @@ StepVerifier.FirstStep after(Flux flux) { } } + /** Don't unnecessarily invoke {@link StepVerifier#verifyLater()} multiple times. */ + static final class StepVerifierVerifyLater { + @BeforeTemplate + StepVerifier before(StepVerifier stepVerifier) { + return stepVerifier.verifyLater().verifyLater(); + } + + @AfterTemplate + StepVerifier after(StepVerifier stepVerifier) { + return stepVerifier.verifyLater(); + } + } + /** Don't unnecessarily have {@link StepVerifier.Step} expect no elements. */ static final class StepVerifierStepIdentity { @BeforeTemplate diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index bd6b6ffbbd..82f30c26bf 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -598,6 +598,10 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return StepVerifier.create(Flux.just(1)); } + StepVerifier testStepVerifierVerifyLater() { + return Mono.empty().as(StepVerifier::create).expectError().verifyLater().verifyLater(); + } + ImmutableSet> testStepVerifierStepIdentity() { return ImmutableSet.of( Mono.just(1).as(StepVerifier::create).expectNext(), diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 048e089568..d8ed29ce5d 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -586,6 +586,10 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return Flux.just(1).as(StepVerifier::create); } + StepVerifier testStepVerifierVerifyLater() { + return Mono.empty().as(StepVerifier::create).expectError().verifyLater(); + } + ImmutableSet> testStepVerifierStepIdentity() { return ImmutableSet.of( Mono.just(1).as(StepVerifier::create), From c93356e027893883506b9b9a6f3eca3aacb1a0e5 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Thu, 11 Apr 2024 22:22:00 +0200 Subject: [PATCH 2/9] Introduce `StepVerifierVerify{,Duration}` Refaster rule --- .../refasterrules/ReactorRules.java | 32 +++++++++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 8 +++++ .../refasterrules/ReactorRulesTestOutput.java | 8 +++++ 3 files changed, 48 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index cc233488a3..9e13bfa4e4 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1768,6 +1768,38 @@ StepVerifier.FirstStep after(Flux flux) { } } + /** + * Prefer {@link StepVerifier.LastStep#verify()} over a dangling {@link + * StepVerifier#verifyThenAssertThat()}. + */ + static final class StepVerifierVerify { + @BeforeTemplate + void before(StepVerifier stepVerifier) { + stepVerifier.verifyThenAssertThat(); + } + + @AfterTemplate + void after(StepVerifier stepVerifier) { + stepVerifier.verify(); + } + } + + /** + * Prefer {@link StepVerifier.LastStep#verify(Duration)} over a dangling {@link + * StepVerifier#verifyThenAssertThat(Duration)}. + */ + static final class StepVerifierVerifyDuration { + @BeforeTemplate + void before(StepVerifier stepVerifier, Duration duration) { + stepVerifier.verifyThenAssertThat(duration); + } + + @AfterTemplate + void after(StepVerifier stepVerifier, Duration duration) { + stepVerifier.verify(duration); + } + } + /** Don't unnecessarily invoke {@link StepVerifier#verifyLater()} multiple times. */ static final class StepVerifierVerifyLater { @BeforeTemplate diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 82f30c26bf..c38cbf7d14 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -598,6 +598,14 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return StepVerifier.create(Flux.just(1)); } + void testStepVerifierVerify() { + Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(); + } + + void testStepVerifierVerifyDuration() { + Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(Duration.ZERO); + } + StepVerifier testStepVerifierVerifyLater() { return Mono.empty().as(StepVerifier::create).expectError().verifyLater().verifyLater(); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index d8ed29ce5d..795f33c6f6 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -586,6 +586,14 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return Flux.just(1).as(StepVerifier::create); } + void testStepVerifierVerify() { + Mono.empty().as(StepVerifier::create).expectError().verify(); + } + + void testStepVerifierVerifyDuration() { + Mono.empty().as(StepVerifier::create).expectError().verify(Duration.ZERO); + } + StepVerifier testStepVerifierVerifyLater() { return Mono.empty().as(StepVerifier::create).expectError().verifyLater(); } From 3fbd08b92ad7030d838f42484b5c671ba1110d97 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Thu, 11 Apr 2024 20:38:49 +0200 Subject: [PATCH 3/9] Introduce `StepVerifierLastStepVerifyErrorSatisfiesAssertJ` Refaster rule --- .../refasterrules/ReactorRules.java | 25 +++++++++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 19 ++++++++++++++ .../refasterrules/ReactorRulesTestOutput.java | 15 +++++++++++ 3 files changed, 59 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 9e13bfa4e4..d0ffa482b1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -43,6 +43,7 @@ import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Stream; +import org.assertj.core.api.Assertions; import org.jspecify.annotations.Nullable; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; @@ -1935,6 +1936,30 @@ Duration after(StepVerifier.LastStep step, Consumer consumer) { } } + /** + * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} with AssertJ over more + * contrived alternatives. + */ + static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ { + @BeforeTemplate + void before(StepVerifier.LastStep step, Class clazz, String message) { + Refaster.anyOf( + step.expectError() + .verifyThenAssertThat() + .hasOperatorErrorOfType(clazz) + .hasOperatorErrorWithMessage(message), + step.expectError(clazz).verifyThenAssertThat().hasOperatorErrorWithMessage(message), + step.expectErrorMessage(message).verifyThenAssertThat().hasOperatorErrorOfType(clazz)); + } + + @AfterTemplate + @UseImportPolicy(STATIC_IMPORT_ALWAYS) + void after(StepVerifier.LastStep step, Class clazz, String message) { + step.verifyErrorSatisfies( + t -> Assertions.assertThat(t).isInstanceOf(clazz).hasMessage(message)); + } + } + /** * Prefer {@link StepVerifier.LastStep#verifyErrorMessage(String)} over more verbose alternatives. */ diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index c38cbf7d14..0f3768a842 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -661,6 +661,25 @@ Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).expectErrorSatisfies(t -> {}).verify(); } + void testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { + Mono.empty() + .as(StepVerifier::create) + .expectError() + .verifyThenAssertThat() + .hasOperatorErrorOfType(IllegalStateException.class) + .hasOperatorErrorWithMessage("foo"); + Mono.empty() + .as(StepVerifier::create) + .expectError(IllegalStateException.class) + .verifyThenAssertThat() + .hasOperatorErrorWithMessage("bar"); + Mono.empty() + .as(StepVerifier::create) + .expectErrorMessage("baz") + .verifyThenAssertThat() + .hasOperatorErrorOfType(IllegalStateException.class); + } + Duration testStepVerifierLastStepVerifyErrorMessage() { return Mono.empty().as(StepVerifier::create).expectErrorMessage("foo").verify(); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 795f33c6f6..58f7b4d1ff 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -641,6 +641,21 @@ Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).verifyErrorSatisfies(t -> {}); } + void testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("foo")); + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("bar")); + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("baz")); + } + Duration testStepVerifierLastStepVerifyErrorMessage() { return Mono.empty().as(StepVerifier::create).verifyErrorMessage("foo"); } From bb6f51aa05c4d633ca110cad076fc484e448ec03 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Thu, 11 Apr 2024 22:36:58 +0200 Subject: [PATCH 4/9] Introduce `StepVerifierLastStepVerifyErrorMatchesAssertions` Refaster rule --- .../errorprone/refasterrules/ReactorRules.java | 16 ++++++++++++++++ .../refasterrules/ReactorRulesTestInput.java | 8 ++++++++ .../refasterrules/ReactorRulesTestOutput.java | 6 ++++++ 3 files changed, 30 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index d0ffa482b1..b2f904f8ab 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1920,6 +1920,22 @@ Duration after(StepVerifier.LastStep step, Predicate predicate) { } } + /** + * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose + * alternatives. + */ + static final class StepVerifierLastStepVerifyErrorMatchesAssertions { + @BeforeTemplate + void before(StepVerifier.LastStep step, Predicate predicate) { + step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate); + } + + @AfterTemplate + void after(StepVerifier.LastStep step, Predicate predicate) { + step.verifyErrorMatches(predicate); + } + } + /** * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} over more verbose * alternatives. diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 0f3768a842..26fad064b6 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -657,6 +657,14 @@ Duration testStepVerifierLastStepVerifyErrorMatches() { .verify(); } + void testStepVerifierLastStepVerifyErrorMatchesAssertions() { + Mono.empty() + .as(StepVerifier::create) + .expectError() + .verifyThenAssertThat() + .hasOperatorErrorMatching(IllegalArgumentException.class::equals); + } + Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).expectErrorSatisfies(t -> {}).verify(); } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 58f7b4d1ff..5bbec0478b 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -637,6 +637,12 @@ Duration testStepVerifierLastStepVerifyErrorMatches() { .verifyErrorMatches(IllegalArgumentException.class::equals); } + void testStepVerifierLastStepVerifyErrorMatchesAssertions() { + Mono.empty() + .as(StepVerifier::create) + .verifyErrorMatches(IllegalArgumentException.class::equals); + } + Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).verifyErrorSatisfies(t -> {}); } From 62d4f7eb0f2090ab055cfc225620b226b3dcc484 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Fri, 12 Apr 2024 09:09:17 +0200 Subject: [PATCH 5/9] Run `./apply-error-prone-suggestions.sh` Nice --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index b2f904f8ab..fc26148b28 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -43,7 +43,6 @@ import java.util.function.Supplier; import java.util.stream.Collector; import java.util.stream.Stream; -import org.assertj.core.api.Assertions; import org.jspecify.annotations.Nullable; import org.reactivestreams.Publisher; import reactor.core.publisher.Flux; @@ -1971,8 +1970,7 @@ void before(StepVerifier.LastStep step, Class clazz, String @AfterTemplate @UseImportPolicy(STATIC_IMPORT_ALWAYS) void after(StepVerifier.LastStep step, Class clazz, String message) { - step.verifyErrorSatisfies( - t -> Assertions.assertThat(t).isInstanceOf(clazz).hasMessage(message)); + step.verifyErrorSatisfies(t -> assertThat(t).isInstanceOf(clazz).hasMessage(message)); } } From 55528aac4575d540281eabac0a23c696072019d2 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 12 Apr 2024 18:29:08 +0200 Subject: [PATCH 6/9] Suggestions --- .../refasterrules/ReactorRules.java | 47 ++++++------- .../refasterrules/ReactorRulesTestInput.java | 67 +++++++++---------- .../refasterrules/ReactorRulesTestOutput.java | 53 +++++++-------- 3 files changed, 78 insertions(+), 89 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index fc26148b28..a6d7528efd 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1774,13 +1774,13 @@ StepVerifier.FirstStep after(Flux flux) { */ static final class StepVerifierVerify { @BeforeTemplate - void before(StepVerifier stepVerifier) { - stepVerifier.verifyThenAssertThat(); + StepVerifier.Assertions before(StepVerifier stepVerifier) { + return stepVerifier.verifyThenAssertThat(); } @AfterTemplate - void after(StepVerifier stepVerifier) { - stepVerifier.verify(); + Duration after(StepVerifier stepVerifier) { + return stepVerifier.verify(); } } @@ -1790,13 +1790,13 @@ void after(StepVerifier stepVerifier) { */ static final class StepVerifierVerifyDuration { @BeforeTemplate - void before(StepVerifier stepVerifier, Duration duration) { - stepVerifier.verifyThenAssertThat(duration); + StepVerifier.Assertions before(StepVerifier stepVerifier, Duration duration) { + return stepVerifier.verifyThenAssertThat(duration); } @AfterTemplate - void after(StepVerifier stepVerifier, Duration duration) { - stepVerifier.verify(duration); + Duration after(StepVerifier stepVerifier, Duration duration) { + return stepVerifier.verify(duration); } } @@ -1913,25 +1913,15 @@ Duration before(StepVerifier.LastStep step, Predicate predicate) { return step.expectErrorMatches(predicate).verify(); } - @AfterTemplate - Duration after(StepVerifier.LastStep step, Predicate predicate) { - return step.verifyErrorMatches(predicate); - } - } - - /** - * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose - * alternatives. - */ - static final class StepVerifierLastStepVerifyErrorMatchesAssertions { @BeforeTemplate - void before(StepVerifier.LastStep step, Predicate predicate) { - step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate); + @SuppressWarnings("StepVerifierVerify" /* This is a more specific template. */) + StepVerifier.Assertions before2(StepVerifier.LastStep step, Predicate predicate) { + return step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate); } @AfterTemplate - void after(StepVerifier.LastStep step, Predicate predicate) { - step.verifyErrorMatches(predicate); + Duration after(StepVerifier.LastStep step, Predicate predicate) { + return step.verifyErrorMatches(predicate); } } @@ -1955,10 +1945,11 @@ Duration after(StepVerifier.LastStep step, Consumer consumer) { * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} with AssertJ over more * contrived alternatives. */ - static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ { + static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ { @BeforeTemplate - void before(StepVerifier.LastStep step, Class clazz, String message) { - Refaster.anyOf( + @SuppressWarnings("StepVerifierVerify" /* This is a more specific template. */) + StepVerifier.Assertions before(StepVerifier.LastStep step, Class clazz, String message) { + return Refaster.anyOf( step.expectError() .verifyThenAssertThat() .hasOperatorErrorOfType(clazz) @@ -1969,8 +1960,8 @@ void before(StepVerifier.LastStep step, Class clazz, String @AfterTemplate @UseImportPolicy(STATIC_IMPORT_ALWAYS) - void after(StepVerifier.LastStep step, Class clazz, String message) { - step.verifyErrorSatisfies(t -> assertThat(t).isInstanceOf(clazz).hasMessage(message)); + Duration after(StepVerifier.LastStep step, Class clazz, String message) { + return step.verifyErrorSatisfies(t -> assertThat(t).isInstanceOf(clazz).hasMessage(message)); } } diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java index 26fad064b6..80375ea4b4 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java @@ -598,12 +598,12 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return StepVerifier.create(Flux.just(1)); } - void testStepVerifierVerify() { - Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(); + Object testStepVerifierVerify() { + return Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(); } - void testStepVerifierVerifyDuration() { - Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(Duration.ZERO); + Object testStepVerifierVerifyDuration() { + return Mono.empty().as(StepVerifier::create).expectError().verifyThenAssertThat(Duration.ZERO); } StepVerifier testStepVerifierVerifyLater() { @@ -650,42 +650,41 @@ ImmutableSet testStepVerifierLastStepVerifyErrorClass() { .verifyErrorSatisfies(t -> assertThat(t).isInstanceOf(AssertionError.class))); } - Duration testStepVerifierLastStepVerifyErrorMatches() { - return Mono.empty() - .as(StepVerifier::create) - .expectErrorMatches(IllegalArgumentException.class::equals) - .verify(); - } - - void testStepVerifierLastStepVerifyErrorMatchesAssertions() { - Mono.empty() - .as(StepVerifier::create) - .expectError() - .verifyThenAssertThat() - .hasOperatorErrorMatching(IllegalArgumentException.class::equals); + ImmutableSet testStepVerifierLastStepVerifyErrorMatches() { + return ImmutableSet.of( + Mono.empty() + .as(StepVerifier::create) + .expectErrorMatches(IllegalArgumentException.class::equals) + .verify(), + Mono.empty() + .as(StepVerifier::create) + .expectError() + .verifyThenAssertThat() + .hasOperatorErrorMatching(IllegalStateException.class::equals)); } Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).expectErrorSatisfies(t -> {}).verify(); } - void testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { - Mono.empty() - .as(StepVerifier::create) - .expectError() - .verifyThenAssertThat() - .hasOperatorErrorOfType(IllegalStateException.class) - .hasOperatorErrorWithMessage("foo"); - Mono.empty() - .as(StepVerifier::create) - .expectError(IllegalStateException.class) - .verifyThenAssertThat() - .hasOperatorErrorWithMessage("bar"); - Mono.empty() - .as(StepVerifier::create) - .expectErrorMessage("baz") - .verifyThenAssertThat() - .hasOperatorErrorOfType(IllegalStateException.class); + ImmutableSet testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { + return ImmutableSet.of( + Mono.empty() + .as(StepVerifier::create) + .expectError() + .verifyThenAssertThat() + .hasOperatorErrorOfType(IllegalArgumentException.class) + .hasOperatorErrorWithMessage("foo"), + Mono.empty() + .as(StepVerifier::create) + .expectError(IllegalStateException.class) + .verifyThenAssertThat() + .hasOperatorErrorWithMessage("bar"), + Mono.empty() + .as(StepVerifier::create) + .expectErrorMessage("baz") + .verifyThenAssertThat() + .hasOperatorErrorOfType(AssertionError.class)); } Duration testStepVerifierLastStepVerifyErrorMessage() { diff --git a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java index 5bbec0478b..37f8f4f8eb 100644 --- a/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java +++ b/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestOutput.java @@ -586,12 +586,12 @@ StepVerifier.FirstStep testStepVerifierFromFlux() { return Flux.just(1).as(StepVerifier::create); } - void testStepVerifierVerify() { - Mono.empty().as(StepVerifier::create).expectError().verify(); + Object testStepVerifierVerify() { + return Mono.empty().as(StepVerifier::create).expectError().verify(); } - void testStepVerifierVerifyDuration() { - Mono.empty().as(StepVerifier::create).expectError().verify(Duration.ZERO); + Object testStepVerifierVerifyDuration() { + return Mono.empty().as(StepVerifier::create).expectError().verify(Duration.ZERO); } StepVerifier testStepVerifierVerifyLater() { @@ -631,35 +631,34 @@ ImmutableSet testStepVerifierLastStepVerifyErrorClass() { Mono.empty().as(StepVerifier::create).verifyError(AssertionError.class)); } - Duration testStepVerifierLastStepVerifyErrorMatches() { - return Mono.empty() - .as(StepVerifier::create) - .verifyErrorMatches(IllegalArgumentException.class::equals); - } - - void testStepVerifierLastStepVerifyErrorMatchesAssertions() { - Mono.empty() - .as(StepVerifier::create) - .verifyErrorMatches(IllegalArgumentException.class::equals); + ImmutableSet testStepVerifierLastStepVerifyErrorMatches() { + return ImmutableSet.of( + Mono.empty() + .as(StepVerifier::create) + .verifyErrorMatches(IllegalArgumentException.class::equals), + Mono.empty() + .as(StepVerifier::create) + .verifyErrorMatches(IllegalStateException.class::equals)); } Duration testStepVerifierLastStepVerifyErrorSatisfies() { return Mono.empty().as(StepVerifier::create).verifyErrorSatisfies(t -> {}); } - void testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { - Mono.empty() - .as(StepVerifier::create) - .verifyErrorSatisfies( - t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("foo")); - Mono.empty() - .as(StepVerifier::create) - .verifyErrorSatisfies( - t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("bar")); - Mono.empty() - .as(StepVerifier::create) - .verifyErrorSatisfies( - t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("baz")); + ImmutableSet testStepVerifierLastStepVerifyErrorSatisfiesAssertJ() { + return ImmutableSet.of( + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(IllegalArgumentException.class).hasMessage("foo")), + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(IllegalStateException.class).hasMessage("bar")), + Mono.empty() + .as(StepVerifier::create) + .verifyErrorSatisfies( + t -> assertThat(t).isInstanceOf(AssertionError.class).hasMessage("baz"))); } Duration testStepVerifierLastStepVerifyErrorMessage() { From f82c671bffcbfaca792d41a2dc2bcf5829a9bf82 Mon Sep 17 00:00:00 2001 From: Phil Werli Date: Fri, 1 Nov 2024 13:31:20 +0100 Subject: [PATCH 7/9] Indicate that a subset of these rules have breaking behavior --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index a6d7528efd..f71729bb24 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1772,6 +1772,9 @@ StepVerifier.FirstStep after(Flux flux) { * Prefer {@link StepVerifier.LastStep#verify()} over a dangling {@link * StepVerifier#verifyThenAssertThat()}. */ + // XXX: This rule may break existing code. We want to explicitly nudge towards using {@link + // StepVerifier.Step#assertNext(Consumer)} or {@link StepVerifier.Step#expectNext(Object)} + // together with {@link Step#verifyComplete()}. static final class StepVerifierVerify { @BeforeTemplate StepVerifier.Assertions before(StepVerifier stepVerifier) { From f2830a5656f2cb333bf69f8178000a75780d9f40 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 3 Nov 2024 11:28:23 +0100 Subject: [PATCH 8/9] Write down current thinking --- .../errorprone/refasterrules/ReactorRules.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index f71729bb24..9f0daab1ba 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1772,9 +1772,15 @@ StepVerifier.FirstStep after(Flux flux) { * Prefer {@link StepVerifier.LastStep#verify()} over a dangling {@link * StepVerifier#verifyThenAssertThat()}. */ - // XXX: This rule may break existing code. We want to explicitly nudge towards using {@link - // StepVerifier.Step#assertNext(Consumer)} or {@link StepVerifier.Step#expectNext(Object)} - // together with {@link Step#verifyComplete()}. + // XXX: Application of this rule (and several others in this class) will cause invalid code if the + // result of the rewritten expression is dereferenced. Consider introducing a bug checker that + // identifies rules that change the return type of an expression and annotates them accordingly. + // The associated annotation can then be used to instruct an annotation processor to generate + // corresponding `void` rules that match only statements. This would allow the `Refaster` check to + // conditionally skip "not fully safe" rules. This allows conditionally flagging more dubious + // code, at the risk of compilation failures. With this rule, for example, we want to explicitly + // nudge users towards `StepVerifier.Step#assertNext(Consumer)` or + // `StepVerifier.Step#expectNext(Object)`, together with `Step#verifyComplete()`. static final class StepVerifierVerify { @BeforeTemplate StepVerifier.Assertions before(StepVerifier stepVerifier) { From cee4b2c812b19d82dcb7a4e591a5108c378c3360 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 11 Nov 2024 13:31:12 +0100 Subject: [PATCH 9/9] Doh! --- .../tech/picnic/errorprone/refasterrules/ReactorRules.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java index 9f0daab1ba..404f78305c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java @@ -1769,7 +1769,7 @@ StepVerifier.FirstStep after(Flux flux) { } /** - * Prefer {@link StepVerifier.LastStep#verify()} over a dangling {@link + * Prefer {@link StepVerifier#verify()} over a dangling {@link * StepVerifier#verifyThenAssertThat()}. */ // XXX: Application of this rule (and several others in this class) will cause invalid code if the @@ -1794,7 +1794,7 @@ Duration after(StepVerifier stepVerifier) { } /** - * Prefer {@link StepVerifier.LastStep#verify(Duration)} over a dangling {@link + * Prefer {@link StepVerifier#verify(Duration)} over a dangling {@link * StepVerifier#verifyThenAssertThat(Duration)}. */ static final class StepVerifierVerifyDuration {