Introduce `IsEmpty` matcher for use by Refaster templates #744

Stephan202 · 2023-08-06T14:54:19Z

Suggested commit message:

Introduce `IsEmpty` matcher for use by Refaster templates (#744)

As discussed offline with @rickie: this matcher could be used for #733 (assuming that it's extended to support Mono<Void>; that's not part of this PR.)

github-actions · 2023-08-06T14:56:26Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-06T15:01:39Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Some context.

Stephan202 · 2023-08-06T15:01:51Z

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

-        StepVerifier.create(Mono.just("foo")).expectNextMatches(s -> s.equals("bar")),
-        StepVerifier.create(Mono.just("baz")).expectNextMatches("qux"::equals));
+        Mono.just("foo").as(StepVerifier::create).expectNextMatches(s -> s.equals("bar")),
+        Mono.just("baz").as(StepVerifier::create).expectNextMatches("qux"::equals));


These .as(StepVerifier::create) changes were not previously flagged by the tests, because the actual rule under test suggested a smaller replacement. The reason I noticed this now, is that for the StepVerifierStepIdentity test cases I added a negative entry, in order to validate the @Matches usage.

Stephan202 · 2023-08-06T15:03:18Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java

+            "import reactor.util.context.Context;",
+            "",
+            "class A {",
+            "  int[] negative1() {",


Keeping these counters in sync negatively impacts maintenance. Maybe in a separate PR we should consider a more compact test format for these matchers. (But for this PR I followed the established approach.)

Stephan202 · 2023-08-06T15:04:12Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java

+            "    // BUG: Diagnostic contains:",
+            "    return new ArrayList<>(",
+            "        // BUG: Diagnostic contains:",
+            "        ImmutableList.of());",


Here and below: expressions split across multiple lines, to prove that the outer expression is (also) recognized as an empty collection.

github-actions · 2023-08-06T15:43:06Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2023-08-29T06:59:44Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

98.8% Coverage
0.0% Duplication

github-actions · 2023-08-29T06:59:45Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-10-08T11:36:40Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Rebased and added another commit: now even more empty collections are recognized, and AssertJMapRules and AssertJRules have been simplified accordingly.

Stephan202 · 2023-10-08T11:34:35Z

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

-                  ImmutableMultiset.of(),
-                  ImmutableSortedMultiset.of())),
+          mapAssert.containsExactlyEntriesOf(wellTypedMap),
+          mapAssert.containsExactlyInAnyOrderEntriesOf(wellTypedMap),


This case was previously not covered.

Stephan202 · 2023-10-08T11:35:23Z

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

    @BeforeTemplate
-    AbstractMapAssert<?, ?, K, V> before(AbstractMapAssert<?, ?, K, V> mapAssert, Map<K, V> map) {
+    AbstractMapAssert<?, ?, K, V> before(
+        AbstractMapAssert<?, ?, K, V> mapAssert, Map<? extends K, ? extends V> map) {


Here and below: I generalized the matching a bit, just because I noticed. This likely applies to many other rules; eventually we'll write an Error Prone check for this.

Stephan202 · 2023-10-08T11:39:59Z

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java

+    assertThat(ImmutableMap.of(4, 0))
+        .containsExactlyInAnyOrderEntriesOf(ImmutableMap.of(1, 2, 3, 4));


This one doesn't just use ImmutableMap.of(1, 2), because then another simplification kicks in.

Stephan202 · 2023-10-08T11:40:06Z

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/AssertJMapRulesTestInput.java

+    assertThat(ImmutableMap.of(5, 0)).hasSameSizeAs(ImmutableMap.of());
+    assertThat(ImmutableMap.of(6, 0)).hasSameSizeAs(ImmutableMap.of(1, 2));
+    assertThat(ImmutableMap.of(7, 0)).isEqualTo(ImmutableMap.of());
+    assertThat(ImmutableMap.of(8, 0)).isEqualTo(ImmutableMap.of("foo", "bar"));


Likewise :)

github-actions · 2023-10-08T11:41:27Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-10-08T14:29:42Z

Rebased and resolved conflicts.

github-actions · 2023-10-08T14:31:46Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Venorcis

Hard to review in detail, but overall LGTM 👍🏻

rickie

Pfff this is an insane PR! Really nice work @Stephan202 , the Matcher is amazing ✨ 🤯 !

Got some minor comments, let's merge this soon!

rickie · 2023-10-22T19:04:26Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java

+            "",
+            "  int[][] positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return new int[0][1];",


~~And the [1][0] would maybe be interesting as that will fall through the first check of .get(0) == 0~~ Lol, maybe not worth changing 50+ method names for something that is actually already covered.

I'll have a closer look at your suggestion, but to the general point of method naming: that was quite painful for this PR indeed. I tried to remain consistent with other tests, but it did make me wonder: perhaps we should drop all those number suffixes in favour of something more descriptive.

(W.r.t. this PR: at some point I renumbered using a Vim macro ;).)

W.r.t. the suggestion itself: return new int[1][0] is used above, in method negative2 :)

rickie · 2023-10-22T19:16:15Z

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

+    void before(
+        AbstractMapAssert<?, ?, K, V> mapAssert,
+        @Matches(IsEmpty.class) Map<? extends K, ? extends V> wellTypedMap,
+        @Matches(IsEmpty.class) Map<?, ?> arbitraryMap,


arbitraryTypedMap maybe? To be inline with the previous one :)? arbitraryMap sounds a bit odd IMO.

Btw, this template looks so clean now ✨ !

Grammatically it'd then be arbitrarilyTypedMap. And that would be fine, I suppose, though this parameter stands in for "really any arbitrary map", and the current name covers that. (Not sure about raw Map types; let's ignore those.)

But in the interest of getting this merged: will rename :)

(We should then do a similar rename in AssertThatObjectEnumerableIsEmpty.)

Stephan202

Rebased and added a commit. @rickie feel free to merge.

Updated suggested commit message:

Introduce `IsEmpty` matcher for use by Refaster templates (#744)

While there, generalize a number of Refaster rules using this new matcher.

Stephan202 · 2023-10-23T05:01:55Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java

+            "",
+            "  int[][] positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return new int[0][1];",


I'll have a closer look at your suggestion, but to the general point of method naming: that was quite painful for this PR indeed. I tried to remain consistent with other tests, but it did make me wonder: perhaps we should drop all those number suffixes in favour of something more descriptive.

(W.r.t. this PR: at some point I renumbered using a Vim macro ;).)

Stephan202 · 2023-10-23T05:05:24Z

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

+    void before(
+        AbstractMapAssert<?, ?, K, V> mapAssert,
+        @Matches(IsEmpty.class) Map<? extends K, ? extends V> wellTypedMap,
+        @Matches(IsEmpty.class) Map<?, ?> arbitraryMap,


Grammatically it'd then be arbitrarilyTypedMap. And that would be fine, I suppose, though this parameter stands in for "really any arbitrary map", and the current name covers that. (Not sure about raw Map types; let's ignore those.)

But in the interest of getting this merged: will rename :)

Stephan202 · 2023-10-23T05:37:41Z

refaster-support/src/test/java/tech/picnic/errorprone/refaster/matchers/IsEmptyTest.java

+            "",
+            "  int[][] positive2() {",
+            "    // BUG: Diagnostic contains:",
+            "    return new int[0][1];",


W.r.t. the suggestion itself: return new int[1][0] is used above, in method negative2 :)

Stephan202 · 2023-10-23T05:38:58Z

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

+    void before(
+        AbstractMapAssert<?, ?, K, V> mapAssert,
+        @Matches(IsEmpty.class) Map<? extends K, ? extends V> wellTypedMap,
+        @Matches(IsEmpty.class) Map<?, ?> arbitraryMap,


(We should then do a similar rename in AssertThatObjectEnumerableIsEmpty.)

sonarqubecloud · 2023-10-23T05:43:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

98.8% Coverage
0.0% Duplication

github-actions · 2023-10-23T05:44:26Z

Surviving mutants in this change: 2
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.matchers.IsEmpty`	2	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 requested a review from rickie August 6, 2023 14:54

Stephan202 added this to the 0.13.0 milestone Aug 6, 2023

Stephan202 commented Aug 6, 2023

View reviewed changes

rickie modified the milestones: 0.13.0, 0.14.0 Aug 25, 2023

Stephan202 force-pushed the sschroevers/introduce-is-empty-matcher branch from 8c6a0ac to 3f6e017 Compare August 29, 2023 06:57

rickie added the new feature label Sep 1, 2023

rickie modified the milestones: 0.14.0, 0.15.0 Oct 4, 2023

Stephan202 mentioned this pull request Oct 6, 2023

Introduce FluxEmpty Refaster rule #815

Merged

Stephan202 force-pushed the sschroevers/introduce-is-empty-matcher branch from 3f6e017 to cdd2e3d Compare October 8, 2023 11:34

Stephan202 force-pushed the sschroevers/introduce-is-empty-matcher branch from cdd2e3d to afb5064 Compare October 8, 2023 11:39

Stephan202 commented Oct 8, 2023

View reviewed changes

Stephan202 force-pushed the sschroevers/introduce-is-empty-matcher branch from afb5064 to 2d64df8 Compare October 8, 2023 14:29

Venorcis approved these changes Oct 8, 2023

View reviewed changes

rickie approved these changes Oct 22, 2023

View reviewed changes

Stephan202 added 5 commits October 23, 2023 06:57

Introduce IsEmpty matcher for use by Refaster templates

6fccf36

Doh

ba1462e

One more

03ba253

Cover and simplify more cases

375fe15

Rename parameters

140895c

Stephan202 force-pushed the sschroevers/introduce-is-empty-matcher branch from 2d64df8 to 140895c Compare October 23, 2023 05:41

Stephan202 commented Oct 23, 2023

View reviewed changes

rickie merged commit fa026de into master Oct 23, 2023
17 checks passed

rickie deleted the sschroevers/introduce-is-empty-matcher branch October 23, 2023 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `IsEmpty` matcher for use by Refaster templates #744

Introduce `IsEmpty` matcher for use by Refaster templates #744

Stephan202 commented Aug 6, 2023 •

edited

Loading

github-actions bot commented Aug 6, 2023

github-actions bot commented Aug 6, 2023

Stephan202 left a comment

Stephan202 Aug 6, 2023

Stephan202 Aug 6, 2023

Stephan202 Aug 6, 2023

github-actions bot commented Aug 6, 2023

sonarqubecloud bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Oct 8, 2023

Stephan202 left a comment

Stephan202 Oct 8, 2023

Stephan202 Oct 8, 2023

Stephan202 Oct 8, 2023

Stephan202 Oct 8, 2023

github-actions bot commented Oct 8, 2023

Stephan202 commented Oct 8, 2023

github-actions bot commented Oct 8, 2023

Venorcis left a comment

rickie left a comment

rickie Oct 22, 2023 •

edited

Loading

Stephan202 Oct 23, 2023

Stephan202 Oct 23, 2023

rickie Oct 22, 2023

rickie Oct 22, 2023

Stephan202 Oct 23, 2023

Stephan202 Oct 23, 2023

Stephan202 left a comment

Stephan202 Oct 23, 2023

Stephan202 Oct 23, 2023

Stephan202 Oct 23, 2023

Stephan202 Oct 23, 2023

sonarqubecloud bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

		assertThat(ImmutableMap.of(4, 0))
		.containsExactlyInAnyOrderEntriesOf(ImmutableMap.of(1, 2, 3, 4));

Introduce IsEmpty matcher for use by Refaster templates #744

Introduce IsEmpty matcher for use by Refaster templates #744

Conversation

Stephan202 commented Aug 6, 2023 • edited Loading

github-actions bot commented Aug 6, 2023

github-actions bot commented Aug 6, 2023

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

github-actions bot commented Aug 6, 2023

sonarqubecloud bot commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Oct 8, 2023

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 Oct 8, 2023

Stephan202 commented Oct 8, 2023

github-actions bot commented Oct 8, 2023

Venorcis left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie Oct 22, 2023 • edited Loading

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

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

sonarqubecloud bot commented Oct 23, 2023

github-actions bot commented Oct 23, 2023

Introduce `IsEmpty` matcher for use by Refaster templates #744

Introduce `IsEmpty` matcher for use by Refaster templates #744

Stephan202 commented Aug 6, 2023 •

edited

Loading

rickie Oct 22, 2023 •

edited

Loading