Introduce Refaster templates for `AbstractComparableAssert`s #225

CoolTomatos · 2022-09-06T10:03:44Z

❗ This PR is on top of #237 ❗

With assertJ, sometimes comparisons are evaluated as booleans.
The introduced templates replace them with methods from the AbstractComparableAssert API.

For example

assertThat(1 <= 2).isTrue();

will be then replaced with

assertThat(1).isLessThanOrEqualTo(2);

Further more,

assertThat(1 > 0).isTrue();

will first be replaced with

assertThat(1).isGreaterThan(0);

and then

assertThat(1).isPositive();

CoolTomatos · 2022-09-06T15:46:57Z

...test/resources/tech/picnic/errorprone/refastertemplates/AssertJDoubleTemplatesTestInput.java

+    return ImmutableSet.of(
+        assertThat(1.0 == 2.0).isTrue(),
+        assertThat(1.0 != 2.0).isFalse(),
+        assertThat((byte) 1 == (byte) 2).isTrue(),
+        assertThat(1F == 2F).isTrue(),
+        assertThat(1 == 2).isTrue(),
+        assertThat(1L == 2L).isTrue(),
+        assertThat((short) 1 == (short) 2).isTrue());


At the beginning, I added separate templates for different primitive types like short, long, int, float and byte.
However, the double templates render all the other ones obsolete.
So I removed them and added the primitive types here in the test case.

Stephan202

Rebased and added a commit. Also added some changes that should be moved to a separate PR. Not yet done reviewing, but need to head to the train 😄

Stephan202 · 2022-09-08T15:28:42Z

...ntrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJComparableTemplates.java

+final class AssertJComparableTemplates {
+  private AssertJComparableTemplates() {}
+
+  static final class AbstractComparableAssertActualIsEqualByComparingToExpected<


Generally we'd name this type AssertThatIsEqualByComparingTo; will push a proposal :)

Was struggling quite a bit about how to name stuff here. Consulted @rickie with it as well.
But the proposal looks good to me.

Yes you are right, this part isn't easy.

I recall discussing a template where we would've had clashes if we didn't use the parameter names in the Refaster template name. Where possible we indeed like to simplify and omit the AbstractComparable, actual and expected.

In the previous case that is mentioned here we would've probably needed this setup because of the clashes 😉.

Just saying: an extra argument for picking up the task of auto-suggesting/fixing Refaster template names. I expect this'll be a topic for most/all upcoming Refaster pull requests ;)

Stephan202 · 2022-09-08T16:16:30Z

...test/resources/tech/picnic/errorprone/refastertemplates/AssertJDoubleTemplatesTestInput.java

+  ImmutableSet<AbstractAssert<?, ?>> testAbstractDoubleAssertActualIsEqualToExpected() {
+    return ImmutableSet.of(
+        assertThat(1.0 == 2.0).isTrue(),
+        assertThat(1.0 != 2.0).isFalse(),
+        assertThat((byte) 1 == (byte) 2).isTrue(),
+        assertThat(1F == 2F).isTrue(),
+        assertThat(1 == 2).isTrue(),
+        assertThat(1L == 2L).isTrue(),
+        assertThat((short) 1 == (short) 2).isTrue());
+  }


Very nice that it applies to other types as well. In fact, I see that we can slightly simplify another check 👀

Stephan202

Added another commit :)

Stephan202 · 2022-09-08T17:07:02Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJDoubleTemplates.java

@@ -45,6 +49,26 @@ AbstractDoubleAssert<?> after(AbstractDoubleAssert<?> doubleAssert, double n) {
    }
  }

+  static final class AssertThatIsEqualTo {


Perhaps AssertJNumberTemplates is a better location for these checks; that matches e.g. the AssertThatIsOdd check. Will have a look.

Stephan202

Added two more commits. I'll circle back to this PR "soon", but it's close to done. Nice!

Stephan202 · 2022-09-08T20:12:59Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+    @BeforeTemplate
+    AbstractBooleanAssert<?> before(boolean actual, boolean expected) {
+      return Refaster.anyOf(
+          assertThat(actual == expected).isTrue(), assertThat(actual != expected).isFalse());
+    }


Here and below I added boolean support, but those cases don't belong in this class. Still pondering another setup.

Stephan202 · 2022-09-08T20:14:35Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+    @BeforeTemplate
+    AbstractBooleanAssert<?> before(double actual, double expected) {
+      return Refaster.anyOf(
+          assertThat(actual == expected).isTrue(), assertThat(actual != expected).isFalse());
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    AbstractBooleanAssert<?> after(boolean actual, boolean expected) {
+      return assertThat(actual).isEqualTo(expected);
+    }


These checks apply to primitive number types only, so arguably me moving them to AssertJNumberTemplates isn't correct either. Perhaps we need a synthetic AssertJPrimitiveTemplates class. TBD.

AssertJPrimitiveTemplates sounds like a good idea. I was thinking whether the convention is always clear as to when to put it in there, but yeah it is. So I'd say it's a good idea.

Stephan202 · 2022-09-08T20:15:51Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+  // XXX: This template also matches (unlikely) expressions of the form `assertThat(someChar %
+  // 2).isEqualTo(1)`, for which the replacement AssertJ expression is invalid.


Here and below: TBD whether I'll move the extra changes to a separate PR, or (i.c.w. a suitable commit message) keep them in this PR.

To be 100% correct we could add a Matcher for that? I'd be up for creating that in refaster-support. WDYT?

Indeed a Matcher could help here. It would be niche, but still. If you're up for this, we should first extract aa820c5 from this branch, as that will simplify the testing of such Matchers. I can open a PR for that ~later today.

^ I just opened #232; feel free to move the relevant changes to a PR on top of that one :)

rickie

Left some answers, the code looks good. Not yet approving because we need to settle on some things 😄.

Thanks for opening the PR @CoolTomatos 🚀 !

rickie · 2022-09-09T08:34:59Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+    @BeforeTemplate
+    AbstractBooleanAssert<?> before(double actual, double expected) {
+      return Refaster.anyOf(
+          assertThat(actual == expected).isTrue(), assertThat(actual != expected).isFalse());
+    }
+
+    @AfterTemplate
+    @UseImportPolicy(STATIC_IMPORT_ALWAYS)
+    AbstractBooleanAssert<?> after(boolean actual, boolean expected) {
+      return assertThat(actual).isEqualTo(expected);
+    }


AssertJPrimitiveTemplates sounds like a good idea. I was thinking whether the convention is always clear as to when to put it in there, but yeah it is. So I'd say it's a good idea.

rickie · 2022-09-09T08:36:00Z

...e-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJNumberTemplates.java

+  // XXX: This template also matches (unlikely) expressions of the form `assertThat(someChar %
+  // 2).isEqualTo(1)`, for which the replacement AssertJ expression is invalid.


To be 100% correct we could add a Matcher for that? I'd be up for creating that in refaster-support. WDYT?

rickie · 2022-09-09T09:49:40Z

Added a commit to move the templates to AssertJPrimitiveTemplates.

ferdinand-swoboda · 2022-09-09T15:50:54Z

Thanks for tagging, I think your PR is in very qualified hands, though 🙂

Let's get @CoolTomatos a shiny sticker to tell the world of this project 😄

rickie · 2022-09-10T13:24:20Z

Let's get @CoolTomatos a shiny sticker to tell the world of this project 😄

That sounds like a good idea 😄.

Stephan202

Rebased and added a commit. @rickie will extract some changes from this PR, but it LGTM otherwise.

Suggested commit message:

Introduce `AssertJComparableTemplates` and `AssertJPrimitiveTemplates` (#225)

Stephan202 · 2022-09-10T14:21:30Z

.../resources/tech/picnic/errorprone/refastertemplates/AssertJPrimitivesTemplatesTestInput.java

+import org.assertj.core.api.AbstractAssert;
+import tech.picnic.errorprone.refaster.test.RefasterTemplateTestCase;
+
+final class AssertJStringTemplatesTest implements RefasterTemplateTestCase {


^ We should update the class name. (Another thing to automate 😅)

Another thing to automate 😅

I filed #233 :)

Stephan202 · 2022-09-10T14:21:57Z

...ne-contrib/src/test/java/tech/picnic/errorprone/refastertemplates/RefasterTemplatesTest.java

@@ -28,6 +29,7 @@ final class RefasterTemplatesTest {
          AssertJMapTemplates.class,
          AssertJObjectTemplates.class,
          AssertJOptionalTemplates.class,
+          AssertJPrimitivesTemplates.class,


Seems we favour singular here:

Suggested change

AssertJPrimitivesTemplates.class,

AssertJPrimitiveTemplates.class,

rickie · 2022-09-13T09:49:27Z

Filed #237 and changed the base for this branch to that one. Now the diff looks clean 🚀 !

rickie · 2022-09-13T09:50:16Z

Thanks for tagging, I think your PR is in very qualified hands, though 🙂

I assume you don't want to review anymore @ferdinand-swoboda ? If not, we can remove the request for review.

rickie

Forgot to approve after moving out some changes.

The code looks nice, thanks for the contribution @CoolTomatos 🚀 !

Suggested commit message LGTM.

Stephan202

Rebased. We'll merge once #237 is merged. Thx for the PR @CoolTomatos!

…e evaluated as booleans.

Stephan202 · 2022-09-18T12:43:41Z

Rebased; will merge once built.

CoolTomatos self-assigned this Sep 6, 2022

CoolTomatos requested a review from rickie September 6, 2022 10:25

CoolTomatos force-pushed the CoolTomatos/abstract_comparable_assert branch 3 times, most recently from 0c73ba6 to b502fbf Compare September 6, 2022 15:34

CoolTomatos marked this pull request as ready for review September 6, 2022 15:44

CoolTomatos requested a review from Stephan202 September 6, 2022 15:45

CoolTomatos commented Sep 6, 2022

View reviewed changes

CoolTomatos force-pushed the CoolTomatos/abstract_comparable_assert branch 3 times, most recently from 2346fee to 264ee51 Compare September 6, 2022 20:00

CoolTomatos requested a review from ferdinand-swoboda September 8, 2022 07:03

Stephan202 force-pushed the CoolTomatos/abstract_comparable_assert branch from 264ee51 to 9d7b2fc Compare September 8, 2022 16:23

Stephan202 reviewed Sep 8, 2022

View reviewed changes

rickie reviewed Sep 9, 2022

View reviewed changes

rickie added the new feature label Sep 10, 2022

Stephan202 force-pushed the CoolTomatos/abstract_comparable_assert branch from 7885a2c to b4a1581 Compare September 10, 2022 14:28

Stephan202 reviewed Sep 10, 2022

View reviewed changes

Stephan202 mentioned this pull request Sep 10, 2022

Have RefasterTemplateCollection verify template test class names #233

Merged

rickie mentioned this pull request Sep 13, 2022

Introduce IsCharacter matcher and assorted test improvements #237

Merged

rickie force-pushed the CoolTomatos/abstract_comparable_assert branch from b4a1581 to 61ae225 Compare September 13, 2022 09:48

rickie changed the base branch from master to rossendrijver/char_matcher September 13, 2022 09:48

CoolTomatos removed the request for review from ferdinand-swoboda September 13, 2022 15:26

rickie approved these changes Sep 14, 2022

View reviewed changes

CoolTomatos requested a review from Stephan202 September 15, 2022 07:05

Stephan202 force-pushed the rossendrijver/char_matcher branch from 774f8b7 to cb9fc83 Compare September 17, 2022 17:43

Stephan202 force-pushed the CoolTomatos/abstract_comparable_assert branch from 61ae225 to d4f9dc4 Compare September 18, 2022 11:36

Stephan202 added this to the 0.2.1 milestone Sep 18, 2022

Stephan202 approved these changes Sep 18, 2022

View reviewed changes

Base automatically changed from rossendrijver/char_matcher to master September 18, 2022 12:39

CoolTomatos and others added 16 commits September 18, 2022 14:40

Switch to use AbstractComparableAsserts when comparison assertions ar…

df5e97d

…e evaluated as booleans.

Some test coverage.

73abd03

Some more test coverage.

db914b6

Statically import the Assertions.assertThat method.

4ebbd40

Update the test output.

ee37106

Fixing a typo.

df895c9

Trying to fix the build.

87f32c9

Covering the other primitive types.

7d9345a

Added two more templates.

06050fe

Suggestions for AssertJComparableTemplates

bf8e4e4

Other stuff

21d3a1f

More test cases and renaming

f7ee6c9

More tests, some sorting, flag issues

4312662

Move some checks (not necessarily happy with new location)

3d0b0ce

Introduce AssertJPrimitiveTemplates and move some templates

e4042dc

Suggestions

12518ec

Stephan202 force-pushed the CoolTomatos/abstract_comparable_assert branch from d4f9dc4 to 12518ec Compare September 18, 2022 12:43

Stephan202 merged commit 8e7d04a into master Sep 18, 2022

Stephan202 deleted the CoolTomatos/abstract_comparable_assert branch September 18, 2022 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Refaster templates for `AbstractComparableAssert`s #225

Introduce Refaster templates for `AbstractComparableAssert`s #225

CoolTomatos commented Sep 6, 2022 •

edited by rickie

Loading

CoolTomatos Sep 6, 2022

Stephan202 left a comment

Stephan202 Sep 8, 2022

CoolTomatos Sep 8, 2022

rickie Sep 9, 2022

Stephan202 Sep 9, 2022

Stephan202 Sep 8, 2022

Stephan202 left a comment

Stephan202 Sep 8, 2022

Stephan202 left a comment

Stephan202 Sep 8, 2022

Stephan202 Sep 8, 2022

rickie Sep 9, 2022

Stephan202 Sep 8, 2022

rickie Sep 9, 2022

Stephan202 Sep 9, 2022

Stephan202 Sep 10, 2022

rickie left a comment

rickie Sep 9, 2022

rickie Sep 9, 2022

rickie commented Sep 9, 2022

ferdinand-swoboda commented Sep 9, 2022

rickie commented Sep 10, 2022

Stephan202 left a comment

Stephan202 Sep 10, 2022

Stephan202 Sep 10, 2022

Stephan202 Sep 10, 2022

rickie commented Sep 13, 2022

rickie commented Sep 13, 2022

rickie left a comment

Stephan202 left a comment

Stephan202 commented Sep 18, 2022

		// XXX: This template also matches (unlikely) expressions of the form `assertThat(someChar %
		// 2).isEqualTo(1)`, for which the replacement AssertJ expression is invalid.

	AssertJPrimitivesTemplates.class,
	AssertJPrimitiveTemplates.class,

Introduce Refaster templates for AbstractComparableAsserts #225

Introduce Refaster templates for AbstractComparableAsserts #225

Conversation

CoolTomatos commented Sep 6, 2022 • edited by rickie Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Sep 9, 2022

ferdinand-swoboda commented Sep 9, 2022

rickie commented Sep 10, 2022

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

rickie commented Sep 13, 2022

rickie commented Sep 13, 2022

rickie left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 commented Sep 18, 2022

Introduce Refaster templates for `AbstractComparableAssert`s #225

Introduce Refaster templates for `AbstractComparableAssert`s #225

CoolTomatos commented Sep 6, 2022 •

edited by rickie

Loading