Introduce JUnit factory method BugChecker #319

eric-picnic · 2022-10-27T14:48:58Z

The BugChecker flags JUnit factory methods which do not have a comment with the parameter names of the test method, or which do not follow the naming convention of suffixing the test method name with TestCases.

rickie

It's a very interesting check with some questions we (either deferred) or haven't had to ask ourselves yet. Additionally, this is quite a Picnic-opinionated check 😄.

To make it easier to review this PR + getting it merged, I think we can do two things:

Change the base from this PR to Introduce JUnitClassModifiers check #214 there we have some first changes that we should do either way and make this PR a bit easier.
Secondly, extract the things from JUnit into a different PR.

Added a commit with some suggestions. Didn't really review functionality wise yet but wanted to start the discussion already.

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

rickie · 2022-11-03T10:55:51Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+    return Optional.empty();
+  }
+
+  private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) {


This is useful in a much bigger context. I'd suggest moving this to MoreASTHelpers.

rickie · 2022-11-03T11:43:59Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/JUnit.java

+import java.util.Optional;
+import javax.lang.model.type.TypeKind;
+
+/** A set of JUnit-specific helpers for working with the AST. */


I'm not yet sure what we should do with this... Ideally this would live in google/error-prone in the JUnitMatchers class.

For the reason that class exists, I'd suggest we either name this class MoreJUnitMatchers or more it to MoreMatchers till we see a reason to split it...? WDYT?

I'm fine with either option, but I think it makes sense to parallel JUnitMatchers here. Let's start with renaming it MoreJUnitMatchers, and get back to it when we have a clearer idea of in which order the currently open JUnit-related BugChecker PRs should be merged and what code we want to share between the BugCheckers.

rickie · 2022-11-03T16:11:08Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+ * A {@link BugChecker} that flags non-canonical JUnit factory method declarations.
+ *
+ * <p>A canonical JUnit factory method is one which - has the same name as the test method it
+ * provides test cases for, but with a `TestCases` suffix, and - has a comment which connects the


It makes more sense to list the requirements in an (un)ordered list. As this is quite an opinionated check we could decide to write something like "we consider a canonical". WDYT?

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

eric-picnic · 2022-11-04T16:00:15Z

I have broken out some of the changes to a separate PR (#335), and rebased this branch on top of them.

rickie

Added a commit.

Simplified some parts of the code. Flushing what I already have to show some progress :).
Will need more time for this nice check 😄.

rickie · 2022-11-10T07:51:24Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+      anyOf(
+          annotations(AT_LEAST_ONE, isType("java.lang.Override")),
+          allOf(
+              not(hasModifier(FINAL)),


Although we could prefer to always statically import these, until now, we always didn't in hasModifier calls, so I'd suggest to keep it consistent.

rickie · 2022-11-10T07:55:23Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+    ImmutableList<MethodTree> factoryMethods =
+        Optional.ofNullable(state.findEnclosing(ClassTree.class))
+            .map(
+                enclosingClass ->


We are not using the enclosingclass, so we can simplify here.

rickie · 2022-11-10T12:44:56Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+        ASTHelpers.getAnnotationWithSimpleName(
+            tree.getModifiers().getAnnotations(), "MethodSource");
+
+    if (methodSourceAnnotation == null) {


We can assume by now that this is true. The first if statement verifies that it is present.

rickie · 2022-11-10T12:51:19Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+    }
+
+    Optional<String> factoryMethodName =
+        MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation);


Using the Optional#map we can combine this call and the following, and therefore omit one of the if statements :).

rickie · 2022-11-10T13:04:53Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+    return Description.NO_MATCH;
+  }
+
+  private ImmutableList<Description> getSuggestedFixes(


I think inlining this method is as readable and makes the code significantly shorter. LMKWYT :)

rickie · 2022-11-10T13:05:23Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+  }
+
+  private ImmutableList<Description> getFactoryMethodNameFixes(
+      MethodTree tree,


We can also provide "just" the Name of the MethodTree.

rickie · 2022-11-10T20:30:33Z

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java

+            "import org.junit.jupiter.params.provider.Arguments;",
+            "import org.junit.jupiter.params.provider.MethodSource;",
+            "",
+            "class A {",


In the identification test we already nicely show what exact cases are flagged. What we usually do in the replacement test is verify the actual rewrite for one or two cases. In this example I think we can get away with using only one example in the replacement test that shows all the rewrites. This means we can simplify the test code a lot.

rickie · 2022-12-08T20:36:28Z

So went through a big rebase. Not sure what made it harder, but I think because there were a lot of overlapping changes.

Let's see what Pitest has to say and let's focus on this one again.

github-actions · 2022-12-08T20:37:28Z

Surviving mutants in this change: 6
Killed mutants in this change: 63

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration`	4	43
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	16
🧟`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	1	2
🎉`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration$IndexedStatement`	0	2

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

rickie

Just flushing some comments I have.

Introducing a new utility (ConflictDetection) comes with the requirement to add a test for that as well 😉.

rickie · 2022-12-09T08:05:01Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+/**
+ * A set of helper methods for finding conflicts which would be caused by applying certain fixes.
+ */
+public final class ConflictDetection {


Isn't it about MethodNameConflictDetection? I think, especially for the current state, ConflictDetection is a bit too generic 🤔.

rickie · 2022-12-09T08:05:36Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+   * @return A human-readable argument against assigning the proposed name to an existing method, or
+   *     {@link Optional#empty()} if no blocker was found.
+   */
+  public static Optional<String> findMethodRenameBlocker(String methodName, VisitorState state) {


This can probably be a CharSequence, we also use that in other Utils.

rickie · 2022-12-09T08:06:04Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+    if (methodExistsInEnclosingClass(methodName, state)) {
+      return Optional.of(
+          String.format("a method named `%s` already exists in this class", methodName));
+    }


Maybe a matter of taste, but using else if would make it shorter 👀.

eric-picnic · 2022-12-12T15:40:28Z

@rickie, I have pushed a commit for refining some of the tests to get better mutation testing coverage.

github-actions · 2022-12-12T15:41:24Z

Surviving mutants in this change: 3
Killed mutants in this change: 66

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration`	1	46
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	16
🧟`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	1	2
🎉`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration$IndexedStatement`	0	2

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

rickie

Added a commit and left some comments. Had to first go over the code with style things before diving in deeper. Cool stuff!

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

rickie · 2023-01-12T20:42:36Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+import tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers;
+
+/**
+ * A {@link BugChecker} that flags non-canonical JUnit factory method declarations.


It actually does more than that right 👀. It also kind of flags things about it's usages.

For that exact reason we might want to drop the Declaration part of this class name.
Which kind of makes me doubt, does the check do too many things? Should we split it 🤔.

...trib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclarationTest.java

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

rickie · 2023-01-12T20:57:04Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

+  }
+
+  @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR)
+  public static final class RenameBlockerFlagger extends BugChecker implements MethodTreeMatcher {


Should have a Javadoc :).

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

rickie · 2023-01-13T09:44:35Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java

+   * @return The name of the factory method referred to in the annotation. Alternatively, {@link
+   *     Optional#empty()} if there is more than one.
+   */
+  public static Optional<String> extractSingleFactoryMethodName(


We should add a test case for this in the MoreJUnitMatchersTest, are you up for that?

github-actions · 2023-01-13T09:48:26Z

Surviving mutants in this change: 3
Killed mutants in this change: 69

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration`	1	46
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19
🧟`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	1	2
🎉`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration$IndexedStatement`	0	2

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

rickie

Some more comments and a commit. PTAL :).

rickie · 2023-01-13T11:50:35Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+    List<? extends StatementTree> statements = factoryMethod.getBody().getStatements();
+
+    Stream<? extends StatementTree> returnStatementsNeedingComment =
+        Streams.mapWithIndex(statements.stream(), IndexedStatement::new)


Can you elaborate a bit on this part? Why do we need an IndexedStatement exactly? What are we trying to achieve here?

rickie · 2023-01-13T11:53:15Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+            .map(Object::toString)
+            .collect(toImmutableList());
+
+    String expectedComment = parameterNames.stream().collect(joining(", ", "/* { ", " } */"));


We can use the collect in the statement above directly.

Maybe extracting to a method would make the intent a bit more clear.

rickie · 2023-01-13T11:56:33Z

...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java

+
+    List<? extends StatementTree> statements = factoryMethod.getBody().getStatements();
+
+    Stream<? extends StatementTree> returnStatementsNeedingComment =


Can there be more than one statement that needs a comment? Don't think so right? If so, it would require an extra test for sure (or I am missing something). Otherwise, we can improve the code a bit I think.

github-actions · 2023-01-13T12:00:48Z

Surviving mutants in this change: 3
Killed mutants in this change: 70

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration`	1	47
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19
🧟`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	1	2
🎉`tech.picnic.errorprone.bugpatterns.JUnitFactoryMethodDeclaration$IndexedStatement`	0	2

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

eric-picnic requested review from rickie and oxkitsune October 27, 2022 14:48

eric-picnic force-pushed the eric-picnic/canonical-junit-factory-methods branch 2 times, most recently from 77f83b4 to aeb0beb Compare October 31, 2022 07:54

rickie mentioned this pull request Nov 1, 2022

Introduce JUnitClassModifiers check #214

Merged

rickie requested changes Nov 3, 2022

View reviewed changes

eric-picnic force-pushed the eric-picnic/canonical-junit-factory-methods branch from 93576b4 to cdd48ef Compare November 4, 2022 15:56

eric-picnic mentioned this pull request Nov 4, 2022

Introduce More{ASTHelpers,JUnitMatchers,Matchers} utility classes #335

Merged

eric-picnic requested a review from rickie November 7, 2022 12:41

oxkitsune approved these changes Nov 8, 2022

View reviewed changes

eric-picnic force-pushed the eric-picnic/canonical-junit-factory-methods branch from cdd48ef to 732eb79 Compare November 9, 2022 12:08

eric-picnic changed the base branch from master to eric-picnic/more-matcher-classes November 9, 2022 12:08

rickie reviewed Nov 10, 2022

View reviewed changes

rickie force-pushed the eric-picnic/more-matcher-classes branch 2 times, most recently from 5e2ab41 to c841481 Compare November 10, 2022 21:15

Stephan202 force-pushed the eric-picnic/more-matcher-classes branch from 541df42 to 6bcc3bd Compare November 13, 2022 14:08

rickie force-pushed the eric-picnic/more-matcher-classes branch from 3494a88 to 36c710d Compare November 18, 2022 13:38

Stephan202 force-pushed the eric-picnic/more-matcher-classes branch from 7f1dd5e to 0e7276b Compare November 22, 2022 07:23

Base automatically changed from eric-picnic/more-matcher-classes to master November 22, 2022 12:35

rickie force-pushed the eric-picnic/canonical-junit-factory-methods branch from fe8a4c2 to fff620a Compare December 8, 2022 20:34

rickie added this to the 0.7.0 milestone Dec 8, 2022

rickie added the new feature label Dec 8, 2022

rickie reviewed Dec 12, 2022

View reviewed changes

eric-picnic force-pushed the eric-picnic/canonical-junit-factory-methods branch 2 times, most recently from cf726c2 to b338a54 Compare December 19, 2022 14:34

More minor tweaks

a2626ee

rickie requested changes Jan 13, 2023

View reviewed changes

Extract and improve method

fc6af33

rickie requested changes Jan 13, 2023

View reviewed changes

rickie modified the milestones: 0.8.0, 0.9.0 Jan 27, 2023

rickie modified the milestones: 0.9.0, 0.10.0 Feb 20, 2023

rickie modified the milestones: 0.10.0, 0.11.0 Apr 26, 2023

Stephan202 modified the milestones: 0.11.0, 0.12.0 May 14, 2023

Stephan202 modified the milestones: 0.12.0, 0.13.0 Jun 21, 2023

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

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

rickie modified the milestones: 0.15.0, 0.16.0 Jan 28, 2024

rickie modified the milestones: 0.16.0, 0.17.0 Mar 8, 2024

Stephan202 modified the milestones: 0.17.0, 0.18.0 Jul 20, 2024

rickie modified the milestones: 0.18.0, 0.19.0 Aug 7, 2024

rickie modified the milestones: 0.19.0, 0.20.0 Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce JUnit factory method BugChecker #319

Introduce JUnit factory method BugChecker #319

eric-picnic commented Oct 27, 2022

rickie left a comment

rickie Nov 3, 2022

rickie Nov 3, 2022

eric-picnic Nov 4, 2022

rickie Nov 3, 2022

eric-picnic commented Nov 4, 2022

rickie left a comment

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie Nov 10, 2022

rickie commented Dec 8, 2022

github-actions bot commented Dec 8, 2022

rickie left a comment

rickie Dec 9, 2022

rickie Dec 9, 2022

rickie Dec 9, 2022

eric-picnic commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

rickie left a comment

rickie Jan 12, 2023

rickie Jan 12, 2023

rickie Jan 13, 2023

github-actions bot commented Jan 13, 2023

rickie left a comment

rickie Jan 13, 2023

rickie Jan 13, 2023

rickie Jan 13, 2023

rickie Jan 13, 2023

github-actions bot commented Jan 13, 2023


		List<? extends StatementTree> statements = factoryMethod.getBody().getStatements();

		Stream<? extends StatementTree> returnStatementsNeedingComment =

Introduce JUnit factory method BugChecker #319

Are you sure you want to change the base?

Introduce JUnit factory method BugChecker #319

Conversation

eric-picnic commented Oct 27, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-picnic commented Nov 4, 2022

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

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 Dec 8, 2022

github-actions bot commented Dec 8, 2022

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

Choose a reason for hiding this comment

eric-picnic commented Dec 12, 2022

github-actions bot commented Dec 12, 2022

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

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 13, 2023