Introduce JUnit factory method BugChecker #319
Conversation
eric-picnic
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
2 times, most recently
from
77f83b4
to
aeb0beb
Compare
There was a problem hiding this comment.
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
JUnitClassModifierscheck #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
JUnitinto 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
Outdated
Show resolved
Hide resolved
| return Optional.empty(); | ||
| } | ||
|
|
||
| private static boolean isMethodInEnclosingClass(String methodName, VisitorState state) { |
There was a problem hiding this comment.
This is useful in a much bigger context. I'd suggest moving this to MoreASTHelpers.
| import java.util.Optional; | ||
| import javax.lang.model.type.TypeKind; | ||
|
|
||
| /** A set of JUnit-specific helpers for working with the AST. */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| * 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 |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
Outdated
Show resolved
Hide resolved
...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
Outdated
Show resolved
Hide resolved
eric-picnic
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
from
93576b4
to
cdd48ef
Compare
|
I have broken out some of the changes to a separate PR (#335), and rebased this branch on top of them. |
eric-picnic
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
from
cdd48ef
to
732eb79
Compare
eric-picnic
changed the base branch from
master
to
eric-picnic/more-matcher-classes
| anyOf( | ||
| annotations(AT_LEAST_ONE, isType("java.lang.Override")), | ||
| allOf( | ||
| not(hasModifier(FINAL)), |
There was a problem hiding this comment.
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.
| ImmutableList<MethodTree> factoryMethods = | ||
| Optional.ofNullable(state.findEnclosing(ClassTree.class)) | ||
| .map( | ||
| enclosingClass -> |
There was a problem hiding this comment.
We are not using the enclosingclass, so we can simplify here.
| ASTHelpers.getAnnotationWithSimpleName( | ||
| tree.getModifiers().getAnnotations(), "MethodSource"); | ||
|
|
||
| if (methodSourceAnnotation == null) { |
There was a problem hiding this comment.
We can assume by now that this is true. The first if statement verifies that it is present.
| } | ||
|
|
||
| Optional<String> factoryMethodName = | ||
| MoreJUnitMatchers.extractSingleFactoryMethodName(methodSourceAnnotation); |
There was a problem hiding this comment.
Using the Optional#map we can combine this call and the following, and therefore omit one of the if statements :).
| return Description.NO_MATCH; | ||
| } | ||
|
|
||
| private ImmutableList<Description> getSuggestedFixes( |
There was a problem hiding this comment.
I think inlining this method is as readable and makes the code significantly shorter. LMKWYT :)
| } | ||
|
|
||
| private ImmutableList<Description> getFactoryMethodNameFixes( | ||
| MethodTree tree, |
There was a problem hiding this comment.
We can also provide "just" the Name of the MethodTree.
| "import org.junit.jupiter.params.provider.Arguments;", | ||
| "import org.junit.jupiter.params.provider.MethodSource;", | ||
| "", | ||
| "class A {", |
There was a problem hiding this comment.
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
force-pushed
the
eric-picnic/more-matcher-classes
branch
2 times, most recently
from
5e2ab41
to
c841481
Compare
Stephan202
force-pushed
the
eric-picnic/more-matcher-classes
branch
from
541df42
to
6bcc3bd
Compare
rickie
force-pushed
the
eric-picnic/more-matcher-classes
branch
from
3494a88
to
36c710d
Compare
Stephan202
force-pushed
the
eric-picnic/more-matcher-classes
branch
from
7f1dd5e
to
0e7276b
Compare
rickie
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
from
fe8a4c2
to
fff620a
Compare
|
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. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| /** | ||
| * A set of helper methods for finding conflicts which would be caused by applying certain fixes. | ||
| */ | ||
| public final class ConflictDetection { |
There was a problem hiding this comment.
Isn't it about MethodNameConflictDetection? I think, especially for the current state, ConflictDetection is a bit too generic 🤔.
| * @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) { |
There was a problem hiding this comment.
This can probably be a CharSequence, we also use that in other Utils.
| if (methodExistsInEnclosingClass(methodName, state)) { | ||
| return Optional.of( | ||
| String.format("a method named `%s` already exists in this class", methodName)); | ||
| } |
There was a problem hiding this comment.
Maybe a matter of taste, but using else if would make it shorter 👀.
|
@rickie, I have pushed a commit for refining some of the tests to get better mutation testing coverage. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
eric-picnic
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
2 times, most recently
from
cf726c2
to
b338a54
Compare
rickie
force-pushed
the
eric-picnic/canonical-junit-factory-methods
branch
from
b338a54
to
d7adfbc
Compare
...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitFactoryMethodDeclaration.java
Outdated
Show resolved
Hide resolved
| import tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers; | ||
|
|
||
| /** | ||
| * A {@link BugChecker} that flags non-canonical JUnit factory method declarations. |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR) | ||
| public static final class RenameBlockerFlagger extends BugChecker implements MethodTreeMatcher { |
...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java
Outdated
Show resolved
Hide resolved
| * @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( |
There was a problem hiding this comment.
We should add a test case for this in the MoreJUnitMatchersTest, are you up for that?
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
| List<? extends StatementTree> statements = factoryMethod.getBody().getStatements(); | ||
|
|
||
| Stream<? extends StatementTree> returnStatementsNeedingComment = | ||
| Streams.mapWithIndex(statements.stream(), IndexedStatement::new) |
There was a problem hiding this comment.
Can you elaborate a bit on this part? Why do we need an IndexedStatement exactly? What are we trying to achieve here?
| .map(Object::toString) | ||
| .collect(toImmutableList()); | ||
|
|
||
| String expectedComment = parameterNames.stream().collect(joining(", ", "/* { ", " } */")); |
There was a problem hiding this comment.
We can use the collect in the statement above directly.
There was a problem hiding this comment.
Maybe extracting to a method would make the intent a bit more clear.
|
|
||
| List<? extends StatementTree> statements = factoryMethod.getBody().getStatements(); | ||
|
|
||
| Stream<? extends StatementTree> returnStatementsNeedingComment = |
There was a problem hiding this comment.
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.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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.