Introduce assorted AssertJ templates #37

rickie · 2022-02-02T09:24:34Z

Introduce new file AssertJMapTemplates
Introduce templates for assertThat(string).{matches,doesNotMatch}(regex);
Introduce templates to improve expressions assertThat(number % 2).isEqualTo({0,1});

EnricSala

Just a couple of questions but LGTM 👍

EnricSala · 2022-02-03T14:23:35Z

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

+    @BeforeTemplate
+    AbstractLongAssert<?> before(long number) {
+      return assertThat(number % 2).isEqualTo(1);


Just checking, do we need to check also the boxed variant of all of these or are they covered?

Good one!

Refaster does autoboxing for us when it is trying to match. You can turn that off by using @NoAutoboxing, see a check for that here. When the type of a variable is going to be inferred, there is a resolution phase where this method is called.

Let's add test cases to make it clear that this is also included! Pushing something in a bit.

EnricSala · 2022-02-03T14:27:50Z

...rone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJMapTemplates.java

+    @BeforeTemplate
+    AbstractMapAssert<?, ?, K, V> before(AbstractMapAssert<?, ?, K, V> mapAssert, K key, V value) {
+      return mapAssert.containsExactlyInAnyOrderEntriesOf(ImmutableMap.of(key, value));
+    }
+
+    @AfterTemplate
+    AbstractMapAssert<?, ?, K, V> after(AbstractMapAssert<?, ?, K, V> mapAssert, K key, V value) {
+      return mapAssert.containsOnly(Map.entry(key, value));


Not sure I'm a fan of this rule, and special cases for singletons in general 🤔

IMO it leads to non-uniform assertions, for example my OCD is triggered by this:

assertThat(obj.getA()).containsOnly(Map.entry("1", 1)); assertThat(obj.getB()).containsExactlyInAnyOrderEntriesOf(ImmutableMap.of("2", 2, "3", 3));

I think I would prefer using the multi case as the default, so:

assertThat(obj.getA()).containsExactlyInAnyOrderEntriesOf(ImmutableMap.of("1", 1)); assertThat(obj.getB()).containsExactlyInAnyOrderEntriesOf(ImmutableMap.of("2", 2, "3", 3));

Proposing something else now, PTAL 😄. @EnricSala @werli

In many other cases (e.g. for Lists and Sets) we do something similar to: containsExactlyInAnyOrderEntriesOf.
When we know there is only one element, we generally make the assertion stronger by ommiting the InAnyOrder part, such that we can also verify that it is in "in order". In the case of the Map we can use the containsExactlyEntriesOf which: Verifies that the actual map contains only the entries of the given map and nothing else, *in order*.

What do you guys think of this?

In many other cases (e.g. for Lists and Sets) we do something similar to: containsExactlyInAnyOrderEntriesOf.

This is what I'm not sure I agree with 🤔 having special cases for the singletons leads to assertions looking different, and if I need to make a change where a test is now supposed to return more elements then I need to make even more changes. Would be curious to the reasoning to better understand why not just use the multi-element form everywhere :)

When we know there is only one element, we generally make the assertion stronger by ommiting the InAnyOrder part, such that we can also verify that it is in "in order".

Just for my understanding, if we expect to find only one element anyway, why does it matter that we omit the InAnyOrder?

You mention good points here.

if I need to make a change where a test is now supposed to return more elements then I need to make even more changes.

Yeah, that is indeed true.

Would be curious to the reasoning to better understand why not just use the multi-element form everywhere :)

To be frank, I consulted @Stephan202 about this and I think it is good to tag him so he can explain it in his words 😛.

Anyway, I'm going to give it a try and explain some parts of his reasoning (makes at least a good practice for me):
To start, the method name with InAnyOrder is unnecessary long for the single element case as it does not add any information about what is being asserted. So in a way, it obfuscates the (intention of the) code.

Feel like how I'm explaining it doesn't make a strong case 😅.

werli

I couldn't add anything to Enric's nice comments.

Nice to see these getting added. 👍

rickie · 2022-02-16T13:28:35Z

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

+    }
+
+    @BeforeTemplate
+    AbstractIntegerAssert<?> before(short number) {


Ai, this one needs to be a AbstractShortAssert...

I was wrong; the isEqualTo(1) returns a AbstractIntegerAssert, so in this case it is correct.

mussatto

LGTM, I don't have anything extra to add other than the other discussions about the map assertions :)

Badbond

Nice 👍 Got one question and one edge case to consider.

...rone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJMapTemplates.java

Badbond

Thanks for the answers! 👍 LGTM.

rickie · 2022-03-03T14:45:25Z

Suggested commit message:

Introduce `AssertJMapTemplates` and assorted AssertJ templates

This is for `byte`, `int`, `long` and `short`.

Stephan202

Rebased and added a commit.

Tweaked suggested commit message:

Introduce assorted AssertJ Refaster templates (#37)

(The fact that one of the files is new doesn't seem so relevant.)

@mussatto anything else from your side? :)

Stephan202 · 2022-04-10T17:41:52Z

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

+    }
+
+    @AfterTemplate
+    @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)


Suggested change

@UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)

@UseImportPolicy(STATIC_IMPORT_ALWAYS)

Stephan202 · 2022-04-10T17:43:28Z

...b/src/test/resources/tech/picnic/errorprone/bugpatterns/AssertJNumberTemplatesTestInput.java

+    return ImmutableSet.of(
+        assertThat((byte) 1 % 2).isEqualTo(1),
+        assertThat(Byte.valueOf((byte) 1) % 2).isEqualTo(1),
+        assertThat((int) 1 % 2).isEqualTo(1),


int is the default.

Suggested change

assertThat((int) 1 % 2).isEqualTo(1),

assertThat(1 % 2).isEqualTo(1),

Stephan202 · 2022-04-10T17:44:14Z

...b/src/test/resources/tech/picnic/errorprone/bugpatterns/AssertJNumberTemplatesTestInput.java

+        assertThat(Byte.valueOf((byte) 1) % 2).isEqualTo(1),
+        assertThat((int) 1 % 2).isEqualTo(1),
+        assertThat(Integer.valueOf(1) % 2).isEqualTo(1),
+        assertThat((long) 1 % 2).isEqualTo(1),


Suggested change

assertThat((long) 1 % 2).isEqualTo(1),

assertThat(1L % 2).isEqualTo(1),

Stephan202 · 2022-04-10T17:44:18Z

...b/src/test/resources/tech/picnic/errorprone/bugpatterns/AssertJNumberTemplatesTestInput.java

+        assertThat((int) 1 % 2).isEqualTo(1),
+        assertThat(Integer.valueOf(1) % 2).isEqualTo(1),
+        assertThat((long) 1 % 2).isEqualTo(1),
+        assertThat(Long.valueOf(1L) % 2).isEqualTo(1),


Suggested change

assertThat(Long.valueOf(1L) % 2).isEqualTo(1),

assertThat(Long.valueOf(1) % 2).isEqualTo(1),

Stephan202 · 2022-04-10T17:55:19Z

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

@@ -221,4 +225,60 @@ private AssertJNumberTemplates() {}
      return numberAssert.isNotNegative();
    }
  }
+
+  static final class AssertThatIsOdd {


Related to a discussion in another PR: I suppose this should be AssertThatNumberIsOdd. One rule could be to identify method invocations comprising multiple overloads, and to add parameter type(s) in those cases.

Stephan202 · 2022-04-10T18:07:23Z

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

+
+    @AfterTemplate
+    @UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)
+    NumberAssert<?, ?> after(Integer number) {


We can just use long here :)

Stephan202 · 2022-04-10T18:11:28Z

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

+    @BeforeTemplate
+    AbstractIntegerAssert<?> before(byte number) {
+      return assertThat(number % 2).isEqualTo(1);
+    }


Since this calls assertThat(int), and since Refaster is smart enough to match byte to int, we can leave out this case. Same for short below.

rickie · 2022-04-11T13:25:44Z

Changes LGTM.

rickie requested review from Stephan202, EnricSala and werli February 2, 2022 09:24

rickie changed the title ~~Introduce AssertJMapTemplates~~ Introduce assorted AssertJ templates Feb 2, 2022

EnricSala approved these changes Feb 3, 2022

View reviewed changes

werli approved these changes Feb 15, 2022

View reviewed changes

rickie commented Feb 16, 2022

View reviewed changes

rickie requested review from Badbond and mussatto February 16, 2022 13:29

mussatto reviewed Feb 17, 2022

View reviewed changes

Badbond reviewed Feb 17, 2022

View reviewed changes

...rone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJMapTemplates.java Show resolved Hide resolved

...rone-contrib/src/main/java/tech/picnic/errorprone/refastertemplates/AssertJMapTemplates.java Show resolved Hide resolved

Badbond approved these changes Feb 18, 2022

View reviewed changes

rickie and others added 6 commits April 10, 2022 18:23

Introduce AssertJMapTemplates

fe95813

Introduce templates String#{matches,doesNotMatch}

409666b

Introduce templates for NumberAssert{isOdd,IsEven}

db944d0

This is for `byte`, `int`, `long` and `short`.

Tweaks

0d10eaa

Apply suggestions

c2b3db7

Suggestions

0e42b2c

Stephan202 added this to the 0.1.0 milestone Apr 10, 2022

Stephan202 force-pushed the rossendrijver/assertj_template_improvements branch from ddbf8c7 to 0e42b2c Compare April 10, 2022 18:17

Stephan202 approved these changes Apr 10, 2022

View reviewed changes

mussatto approved these changes Apr 11, 2022

View reviewed changes

Stephan202 merged commit eaa98c9 into master Apr 11, 2022

Stephan202 deleted the rossendrijver/assertj_template_improvements branch April 11, 2022 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce assorted AssertJ templates #37

Introduce assorted AssertJ templates #37

rickie commented Feb 2, 2022

EnricSala left a comment

EnricSala Feb 3, 2022

rickie Feb 15, 2022

EnricSala Feb 3, 2022

rickie Feb 15, 2022

EnricSala Feb 15, 2022

rickie Feb 15, 2022

werli left a comment

rickie Feb 16, 2022 •

edited

Loading

rickie Feb 18, 2022 •

edited

Loading

mussatto left a comment

Badbond left a comment

Badbond left a comment

rickie commented Mar 3, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

Stephan202 Apr 10, 2022

rickie commented Apr 11, 2022

	@UseImportPolicy(ImportPolicy.STATIC_IMPORT_ALWAYS)
	@UseImportPolicy(STATIC_IMPORT_ALWAYS)

	assertThat((int) 1 % 2).isEqualTo(1),
	assertThat(1 % 2).isEqualTo(1),

	assertThat((long) 1 % 2).isEqualTo(1),
	assertThat(1L % 2).isEqualTo(1),

	assertThat(Long.valueOf(1L) % 2).isEqualTo(1),
	assertThat(Long.valueOf(1) % 2).isEqualTo(1),

Introduce assorted AssertJ templates #37

Introduce assorted AssertJ templates #37

Conversation

rickie commented Feb 2, 2022

EnricSala 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

werli left a comment

Choose a reason for hiding this comment

rickie Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

rickie Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

mussatto left a comment

Choose a reason for hiding this comment

Badbond left a comment

Choose a reason for hiding this comment

Badbond left a comment

Choose a reason for hiding this comment

rickie commented Mar 3, 2022 • edited Loading

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 commented Apr 11, 2022

rickie Feb 16, 2022 •

edited

Loading

rickie Feb 18, 2022 •

edited

Loading

rickie commented Mar 3, 2022 •

edited

Loading