Skip to content

Commit

Permalink
PSM-1552 Suggestions
Browse files Browse the repository at this point in the history
  • Loading branch information
@Stephan202
Stephan202 committed Feb 22, 2023
1 parent 981f777 commit 08c6784
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 35 deletions.
6 changes: 6 additions & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.ALL;
import static com.google.errorprone.matchers.ChildMultiMatcher.MatchType.AT_LEAST_ONE;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.anyOf;
import static com.google.errorprone.matchers.Matchers.argument;
Expand Down Expand Up @@ -101,6 +102,7 @@ public final class JUnitValueSource extends BugChecker implements MethodTreeMatc
"com.google.common.collect.ImmutableList",
"com.google.common.collect.ImmutableSet")
.named("of"),
hasArguments(AT_LEAST_ONE, (tree, state) -> true),
hasArguments(ALL, SUPPORTED_VALUE_FACTORY_VALUES)));
private static final Matcher<MethodTree> IS_UNARY_METHOD_WITH_SUPPORTED_PARAMETER =
methodHasParameters(
Expand Down Expand Up @@ -241,6 +243,10 @@ private static Optional<String> tryExtractValueSourceAttributeValue(
return Optional.empty();
}

/*
* Join the values into a comma-separated string, unwrapping `Arguments` factory method
* invocations if applicable.
*/
return Optional.of(
arguments.stream()
.map(
Expand Down
34 changes: 31 additions & 3 deletions ...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,26 +55,54 @@ private MoreJUnitMatchers() {}
* Returns the names of the JUnit value factory methods specified by the given {@link
* org.junit.jupiter.params.provider.MethodSource} annotation.
*
* <p>This method differs from {@link #getMethodSourceFactoryDescriptors(AnnotationTree,
* MethodTree)} in that it drops any parenthesized method parameter type enumerations. That is,
* method descriptors such as {@code factoryMethod()} and {@code factoryMethod(java.lang.String)}
* are both simplified to just {@code factoryMethod}.
*
* @param methodSourceAnnotation The annotation from which to extract value factory method names.
* @param method The method on which the annotation is located.
* @return One or more value factory names.
* @see #getMethodSourceFactoryDescriptors(AnnotationTree, MethodTree)
*/
// XXX: Test this method.
public static ImmutableSet<String> getMethodSourceFactoryNames(
AnnotationTree methodSourceAnnotation, MethodTree method) {
return getMethodSourceFactoryDescriptors(methodSourceAnnotation, method).stream()
.map(
descriptor -> {
int index = descriptor.indexOf('(');
return index < 0 ? descriptor : descriptor.substring(0, index);
})
.collect(toImmutableSet());
}

/**
* Returns the descriptors of the JUnit value factory methods specified by the given {@link
* org.junit.jupiter.params.provider.MethodSource} annotation.
*
* @param methodSourceAnnotation The annotation from which to extract value factory method
* descriptors.
* @param method The method on which the annotation is located.
* @return One or more value factory descriptors.
* @see #getMethodSourceFactoryNames(AnnotationTree, MethodTree)
*/
public static ImmutableSet<String> getMethodSourceFactoryDescriptors(
AnnotationTree methodSourceAnnotation, MethodTree method) {
String methodName = method.getName().toString();
ExpressionTree value = AnnotationMatcherUtils.getArgument(methodSourceAnnotation, "value");

if (!(value instanceof NewArrayTree)) {
return ImmutableSet.of(toMethodSourceFactoryName(value, methodName));
return ImmutableSet.of(toMethodSourceFactoryDescriptor(value, methodName));
}

return ((NewArrayTree) value)
.getInitializers().stream()
.map(name -> toMethodSourceFactoryName(name, methodName))
.map(name -> toMethodSourceFactoryDescriptor(name, methodName))
.collect(toImmutableSet());
}

private static String toMethodSourceFactoryName(
private static String toMethodSourceFactoryDescriptor(
@Nullable ExpressionTree tree, String annotatedMethodName) {
return requireNonNullElse(
Strings.emptyToNull(ASTHelpers.constValue(tree, String.class)), annotatedMethodName);
Expand Down
74 changes: 46 additions & 28 deletions ...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,9 @@
import org.junit.jupiter.api.Test;

final class JUnitValueSourceTest {
private final CompilationTestHelper compilationTestHelper =
CompilationTestHelper.newInstance(JUnitValueSource.class, getClass());
private final BugCheckerRefactoringTestHelper refactoringTestHelper =
BugCheckerRefactoringTestHelper.newInstance(JUnitValueSource.class, getClass());

@Test
void identification() {
compilationTestHelper
CompilationTestHelper.newInstance(JUnitValueSource.class, getClass())
.addSourceLines(
"A.java",
"import static org.junit.jupiter.params.provider.Arguments.arguments;",
Expand All @@ -34,6 +29,19 @@ void identification() {
" @MethodSource(\"identificationTestCases\")",
" void identification(int foo) {}",
"",
" private static int[] identificationWithParensTestCases() {",
" return new int[] {1, 2};",
" }",
"",
" @ParameterizedTest",
" // BUG: Diagnostic contains:",
" @MethodSource(\"identificationWithParensTestCases()\")",
" void identificationWithParens(int foo) {}",
"",
" @ParameterizedTest",
" @MethodSource(\"valueFactoryMissingTestCases\")",
" void valueFactoryMissing(int foo) {}",
"",
" private static Stream<Arguments> multipleUsagesTestCases() {",
" return Stream.of(arguments(1), Arguments.of(2));",
" }",
Expand All @@ -43,7 +51,7 @@ void identification() {
" void multipleUsages1(int foo) {}",
"",
" @ParameterizedTest",
" @MethodSource(\"multipleUsagesTestCases\")",
" @MethodSource(\"multipleUsagesTestCases()\")",
" void multipleUsages2(int bar) {}",
"",
" private static Stream<Arguments> multipleParametersTestCases() {",
Expand All @@ -54,16 +62,16 @@ void identification() {
" @MethodSource(\"multipleParametersTestCases\")",
" void multipleParameters(int first, int second) {}",
"",
" private static Stream<Arguments> multiDimArrayTestCases() {",
" return Stream.of(arguments(new int[] {1, 2}), arguments(new int[] {3, 4}));",
" private static int[] arrayWithoutInitializersTestCases() {",
" return new int[1];",
" }",
"",
" @ParameterizedTest",
" @MethodSource(\"multiDimArrayTestCases\")",
" void multiDimArray(int[] foo) {}",
" @MethodSource(\"arrayWithoutInitializersTestCases\")",
" void arrayWithoutInitializers(int foo) {}",
"",
" private static Stream<Arguments> runtimeValueTestCases() {",
" int second = 1 + 2;",
" int second = 2;",
" return Stream.of(arguments(1), arguments(second));",
" }",
"",
Expand All @@ -80,7 +88,7 @@ void identification() {
" void streamChain(int number) {}",
"",
" private static Stream<Arguments> multipleReturnsTestCases() {",
" if (10 > 0) {",
" if (true) {",
" return Stream.of(arguments(1), arguments(2));",
" } else {",
" return Stream.of(arguments(3), arguments(4));",
Expand Down Expand Up @@ -117,12 +125,13 @@ void identification() {
" return Stream.of(arguments(1), arguments(2));",
" }",
" }",
" return Stream.of(arguments(1), arguments(1, 2));",
" return Stream.of(arguments(1), arguments(2));",
" }",
"",
" @ParameterizedTest",
" // BUG: Diagnostic contains:",
" @MethodSource(\"localClassTestCases\")",
" void localClass(int... i) {}",
" void localClass(int i) {}",
"",
" private static Stream<Arguments> lambdaReturnTestCases() {",
" int foo =",
Expand All @@ -132,32 +141,41 @@ void identification() {
" return i / 2;",
" })",
" .orElse(0);",
" return Stream.of(arguments(1), arguments(1, 2));",
" return Stream.of(arguments(1), arguments(1));",
" }",
"",
" @ParameterizedTest",
" // BUG: Diagnostic contains:",
" @MethodSource(\"lambdaReturnTestCases\")",
" void lambdaReturn(int... i) {}",
" void lambdaReturn(int i) {}",
"",
" @ParameterizedTest",
" @MethodSource(\"tech.picnic.errorprone.foo.FooTestCases#fooTestCases\")",
" @MethodSource(\"tech.picnic.errorprone.Foo#fooTestCases\")",
" void staticMethodReference(int foo) {}",
"",
" private static Stream<Arguments> argumentsInValueFactoryTestCases(int amount) {",
" return Stream.of(arguments(1), Arguments.of(2));",
" private static Stream<Arguments> valueFactoryWithArgumentTestCases(int amount) {",
" return Stream.of(arguments(1), arguments(2));",
" }",
"",
" @ParameterizedTest",
" @MethodSource(\"argumentsInValueFactoryTestCases\")",
" void argumentsInValueFactory(int foo) {}",
" @MethodSource(\"valueFactoryWithArgumentTestCases\")",
" void valueFactoryWithArgument(int foo) {}",
"",
" private static Arguments[] emptyValueFactoryTestCases() {",
" private static Arguments[] emptyArrayValueFactoryTestCases() {",
" return new Arguments[] {};",
" }",
"",
" @ParameterizedTest",
" @MethodSource(\"emptyValueFactoryTestCases\")",
" void emptyValueFactory(int foo) {}",
" @MethodSource(\"emptyArrayValueFactoryTestCases\")",
" void emptyArrayValueFactory(int foo) {}",
"",
" private static Stream<Arguments> emptyStreamValueFactoryTestCases() {",
" return Stream.of();",
" }",
"",
" @ParameterizedTest",
" @MethodSource(\"emptyStreamValueFactoryTestCases\")",
" void emptyStreamValueFactory(int foo) {}",
"",
" private static Arguments[] invalidValueFactoryArgumentsTestCases() {",
" return new Arguments[] {arguments(1), arguments(new Object() {})};",
Expand All @@ -172,7 +190,7 @@ void identification() {

@Test
void replacement() {
refactoringTestHelper
BugCheckerRefactoringTestHelper.newInstance(JUnitValueSource.class, getClass())
.addInputLines(
"A.java",
"import static org.junit.jupiter.params.provider.Arguments.arguments;",
Expand Down Expand Up @@ -336,12 +354,12 @@ void replacement() {
" @MethodSource(\"immutableSetOfStringArguments\")",
" void string(String s) {}",
"",
" private static Stream<Class<?>> streamOfClassesAndClassArguments() {",
" private static Stream<Class<?>> streamOfClasses() {",
" return Stream.of(Stream.class, java.util.Map.class);",
" }",
"",
" @ParameterizedTest",
" @MethodSource(\"streamOfClassesAndClassArguments\")",
" @MethodSource(\"streamOfClasses\")",
" void clazz(Class<?> c) {}",
"",
" private static Stream<Arguments> sameNameFactoryTestCases() {",
Expand Down
9 changes: 5 additions & 4 deletions ...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchersTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ void methodMatchers() {

@Test
void getMethodSourceFactoryNames() {
CompilationTestHelper.newInstance(MethodSourceFactoryNamesTestChecker.class, getClass())
CompilationTestHelper.newInstance(MethodSourceFactoryDescriptorsTestChecker.class, getClass())
.addSourceLines(
"A.java",
"import org.junit.jupiter.params.provider.MethodSource;",
Expand Down Expand Up @@ -153,10 +153,10 @@ public Description matchMethod(MethodTree tree, VisitorState state) {

/**
* A {@link BugChecker} that flags methods with a JUnit {@code @MethodSource} annotation by
* enumerating the associated value factory method names.
* enumerating the associated value factory method descriptors.
*/
@BugPattern(summary = "Interacts with `MoreJUnitMatchers` for testing purposes", severity = ERROR)
public static final class MethodSourceFactoryNamesTestChecker extends BugChecker
public static final class MethodSourceFactoryDescriptorsTestChecker extends BugChecker
implements MethodTreeMatcher {
private static final long serialVersionUID = 1L;

Expand All @@ -166,7 +166,8 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
Iterables.getOnlyElement(HAS_METHOD_SOURCE.multiMatchResult(tree, state).matchingNodes());

return buildDescription(tree)
.setMessage(MoreJUnitMatchers.getMethodSourceFactoryNames(annotation, tree).toString())
.setMessage(
MoreJUnitMatchers.getMethodSourceFactoryDescriptors(annotation, tree).toString())
.build();
}
}
Expand Down

0 comments on commit 08c6784

Please sign in to comment.