Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli · 2024-04-11T20:56:33Z

Summary

This PR introduces a bunch of rules associated to StepVerifier.Asserations and a trivial StepVerifier rule.

Example

I spotted the following interesting pattern:

Mono.empty()
    .as(StepVerifier::create)
    .expectError(SpecificException.class)
    .verifyThenAssertThat()
    .hasOperatorErrorWithMessage("msg");

Which made me find the interesting StepVerifier#Assertions API. It's quite low-level, and IMO not as straightforward to read and write.

The example itself is most often represented in our code-base using AssertJ's richer API:

Mono.empty()
    .as(StepVerifier::create)
    .verifyErrorSatisfies(t ->
        assertThat(t)
        .isInstanceOf(SpecificException.class)
        .hasMessage("msg"));

Suggested commit message:

Introduce assorted Reactor `StepVerifier` Refaster rules (#1132)

github-actions · 2024-04-11T20:58:27Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

werli · 2024-04-11T21:00:32Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorSatisfies(Consumer)} with AssertJ over more
+   * contrived alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorSatisfiesAssertJ {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Class<? extends Throwable> 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<? extends Throwable> clazz, String message) {
+      step.verifyErrorSatisfies(
+          t -> Assertions.assertThat(t).isInstanceOf(clazz).hasMessage(message));
+    }
+  }


Asserting both class and message blocks one to use the recommended StepVerifier.Step#verifyError(Class) or StepVerifier.Step#verifyErrorMessage(String). Choosing an alternative isn't easy since there are many possibilities. Out of all the options, AssertJ's assertions have the richest API, so I'd recommend this.

The StepVerifierVerify rule introduced in this PR aims to rewrite the use-cases in which only one of class or message are asserted by dropping the StepVerifier.Assertions type which allows other rules to rewrite further.

werli · 2024-04-11T21:08:26Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose
+   * alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorMatchesAssertions {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate);
+    }
+
+    @AfterTemplate
+    void after(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.verifyErrorMatches(predicate);
+    }
+  }


This case is not exhaustive due to the overload of StepVerifier.LastStep#expectedError, but IMO covers sufficiently.

We can collapse this one into the preceding method.

werli · 2024-04-11T21:09:28Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /** Don't unnecessarily invoke {@link StepVerifier#verifyLater()} multiple times. */
+  static final class StepVerifierVerifyLater<T> {
+    @BeforeTemplate
+    StepVerifier before(StepVerifier stepVerifier) {
+      return stepVerifier.verifyLater().verifyLater();
+    }
+
+    @AfterTemplate
+    StepVerifier after(StepVerifier stepVerifier) {
+      return stepVerifier.verifyLater();
+    }
+  }


The API spec mentions this is a noop. I didn't fully understand why this API may be useful for in the first place.

github-actions · 2024-04-11T21:15:23Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-12T07:11:43Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added a commit. Tnx @werli!

Stephan202 · 2024-04-12T16:03:30Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+  /**
+   * Prefer {@link StepVerifier.LastStep#verifyErrorMatches(Predicate)} over more verbose
+   * alternatives.
+   */
+  static final class StepVerifierLastStepVerifyErrorMatchesAssertions {
+    @BeforeTemplate
+    void before(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.expectError().verifyThenAssertThat().hasOperatorErrorMatching(predicate);
+    }
+
+    @AfterTemplate
+    void after(StepVerifier.LastStep step, Predicate<Throwable> predicate) {
+      step.verifyErrorMatches(predicate);
+    }
+  }


We can collapse this one into the preceding method.

Stephan202 · 2024-04-12T16:18:11Z

...e-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/ReactorRulesTestInput.java

+    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);


We can use three different Throwable types like a bit further up.

Stephan202 · 2024-04-12T16:22:47Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/ReactorRules.java

+   */
+  static final class StepVerifierVerify {
+    @BeforeTemplate
+    void before(StepVerifier stepVerifier) {


While in this case arguably incorrect, we do generally match any expression, even if that can lead to compilation errors. (At least this will force the user to clean up the code.)

I'll apply those changes to this PR, though I could also see us change our mind on that in the future.

My intent was indeed to avoid breaking code changes by only matching the "dangling" case in which the created StepVerifier.Assertions isn't used or cases we should clearly rewrite.

StepVerifier#verifyThenAssertThat has some valid yet niche usages, see internal usages. Sure, we could suppress the warning here, but IMO the warning would be a false positive. WDYT?

github-actions · 2024-04-12T17:33:55Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice improvements!

…rule

… rule

Nice

github-actions · 2024-05-02T06:09:52Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarcloud · 2024-05-02T06:09:58Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

werli added the new feature label Apr 11, 2024

werli commented Apr 11, 2024

View reviewed changes

werli force-pushed the werli/reactor-verify-error-satisfies branch from 7395f93 to 0eea3ac Compare April 11, 2024 21:13

Stephan202 added this to the 0.17.0 milestone Apr 12, 2024

Stephan202 approved these changes Apr 12, 2024

View reviewed changes

rickie approved these changes May 2, 2024

View reviewed changes

werli and others added 6 commits May 2, 2024 08:07

Introduce StepVerifierVerifyLater Refaster rule

14497b0

Introduce StepVerifierVerify{,Duration} Refaster rule

38116b7

Introduce StepVerifierLastStepVerifyErrorSatisfiesAssertJ Refaster …

d740235

…rule

Introduce StepVerifierLastStepVerifyErrorMatchesAssertions Refaster…

7bc9663

… rule

Run ./apply-error-prone-suggestions.sh

581bf77

Nice

Suggestions

fcbda80

rickie force-pushed the werli/reactor-verify-error-satisfies branch from 3742218 to fcbda80 Compare May 2, 2024 06:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli commented Apr 11, 2024 •

edited

github-actions bot commented Apr 11, 2024

werli Apr 11, 2024

werli Apr 11, 2024

Stephan202 Apr 12, 2024

werli Apr 11, 2024

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

Stephan202 left a comment

Stephan202 Apr 12, 2024

Stephan202 Apr 12, 2024

Stephan202 Apr 12, 2024

werli Apr 15, 2024

github-actions bot commented Apr 12, 2024

rickie left a comment •

edited

github-actions bot commented May 2, 2024

sonarcloud bot commented May 2, 2024

Introduce assorted Reactor StepVerifier Refaster rules #1132

Are you sure you want to change the base?

Introduce assorted Reactor StepVerifier Refaster rules #1132

Conversation

werli commented Apr 11, 2024 • edited

Summary

Example

Suggested commit message:

github-actions bot commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 11, 2024

github-actions bot commented Apr 12, 2024

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 12, 2024

rickie left a comment • edited

Choose a reason for hiding this comment

github-actions bot commented May 2, 2024

sonarcloud bot commented May 2, 2024

Quality Gate passed

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

Introduce assorted Reactor `StepVerifier` Refaster rules #1132

werli commented Apr 11, 2024 •

edited

rickie left a comment •

edited