diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java index 6a070146aa2..efc7d08a551 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -220,7 +220,7 @@ private static Optional tryConstructValueSourceFix( .replace( methodSourceAnnotation, String.format( - "@ValueSource(%s = {%s})", + "@ValueSource(%s = %s)", toValueSourceAttributeName(parameterType), valueSourceAttributeValue)) .delete(valueFactoryMethod) .build()); @@ -265,14 +265,15 @@ private static Optional tryExtractValueSourceAttributeValue( } return Optional.of( - arguments.stream() - .map( - arg -> - arg instanceof MethodInvocationTree - ? Iterables.getOnlyElement(((MethodInvocationTree) arg).getArguments()) - : arg) - .map(argument -> SourceCode.treeToString(argument, state)) - .collect(joining(", "))); + arguments.stream() + .map( + arg -> + arg instanceof MethodInvocationTree + ? Iterables.getOnlyElement(((MethodInvocationTree) arg).getArguments()) + : arg) + .map(argument -> SourceCode.treeToString(argument, state)) + .collect(joining(", "))) + .map(value -> String.format(arguments.size() > 1 ? "{%s}" : "%s", value)); } private static String toValueSourceAttributeName(Type type) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java index 7649e9221a4..8c342c361f7 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java @@ -13,11 +13,6 @@ final class JUnitValueSourceTest { // XXX: Combine `identification` tests, focussing on what should _not_ be flagged. // XXX: Still to test (likely not exhaustive): - // - Make sure multi-dim arrays aren't flagged. - // - Verify that same-name factory methods are handled properly. - // - Verify that redundant curly braces in `@ValueSource` are handled properly. - // - Make sure that `return` statement inside local classes, anonymous classes and lambda - // expressions inside value factories are ignored. // - Run PITest and check what other cases should be covered. @Test @@ -25,9 +20,9 @@ void identification() { compilationTestHelper .addSourceLines( "A.java", - "import static org.assertj.core.api.Assertions.assertThat;", "import static org.junit.jupiter.params.provider.Arguments.arguments;", "", + "import java.util.Optional;", "import java.util.stream.Stream;", "import org.junit.jupiter.params.ParameterizedTest;", "import org.junit.jupiter.params.provider.Arguments;", @@ -41,9 +36,7 @@ void identification() { " @ParameterizedTest", " // BUG: Diagnostic contains:", " @MethodSource(\"identificationTestCases\")", - " void identification(int foo) {", - " assertThat(foo).isNotNull();", - " }", + " void identification(int foo) {}", "", " private static Stream multipleParametersTestCases() {", " return Stream.of(arguments(1, 2), arguments(3, 4));", @@ -51,10 +44,16 @@ void identification() { "", " @ParameterizedTest", " @MethodSource(\"multipleParametersTestCases\")", - " void multipleParameters(int first, int second) {", - " assertThat(first).isNotNull();", + " void multipleParameters(int first, int second) {}", + "", + " private static Stream multiDimArrayTestCases() {", + " return Stream.of(arguments(new int[] {1, 2}), arguments(new int[] {3, 4}));", " }", "", + " @ParameterizedTest", + " @MethodSource(\"multiDimArrayTestCases\")", + " void multiDimArray(int[] foo) {}", + "", " private static Stream runtimeValueTestCases() {", " int second = 1 + 2;", " return Stream.of(arguments(1), arguments(second));", @@ -62,9 +61,7 @@ void identification() { "", " @ParameterizedTest", " @MethodSource(\"runtimeValueTestCases\")", - " void runtimeValue(int foo) {", - " assertThat(foo).isNotNull();", - " }", + " void runtimeValue(int foo) {}", "", " private static Stream streamChainTestCases() {", " return Stream.of(1, 2).map(Arguments::arguments);", @@ -72,9 +69,7 @@ void identification() { "", " @ParameterizedTest", " @MethodSource(\"streamChainTestCases\")", - " void streamChain(int number) {", - " assertThat(number).isNotNull();", - " }", + " void streamChain(int number) {}", "", " private static Stream multipleReturnsTestCases() {", " if (10 > 0) {", @@ -86,9 +81,7 @@ void identification() { "", " @ParameterizedTest", " @MethodSource(\"multipleReturnsTestCases\")", - " void multipleReturns(int number) {", - " assertThat(number).isNotNull();", - " }", + " void multipleReturns(int number) {}", "", " private static Stream multipleFactoriesFooTestCases() {", " return Stream.of(arguments(1));", @@ -100,30 +93,43 @@ void identification() { "", " @ParameterizedTest", " @MethodSource({\"multipleFactoriesFooTestCases\", \"multipleFactoriesBarTestCases\"})", - " void multipleFactories(int i) {", - " assertThat(i).isNotNull();", - " }", + " void multipleFactories(int i) {}", "", - " private static Stream extraArgsTestCasts() {", + " private static Stream extraArgsTestCases() {", " return Stream.of(arguments(1), arguments(1, 2));", " }", "", " @ParameterizedTest", - " @MethodSource(\"extraArgsTestCasts\")", - " void extraArgsTestCasts(int... i) {", - " assertThat(i).isNotNull();", - " }", + " @MethodSource(\"extraArgsTestCases\")", + " void extraArgs(int... i) {}", "", " private static Stream localClassTestCases() {", - " class Foo {}", + " class Foo {", + " Stream foo() {", + " return Stream.of(arguments(1), arguments(2));", + " }", + " }", " return Stream.of(arguments(1), arguments(1, 2));", " }", "", " @ParameterizedTest", - " @MethodSource(\"extraArgsTestCasts\")", - " void extraArgsTestCasts(int... i) {", - " assertThat(i).isNotNull();", + " @MethodSource(\"localClassTestCases\")", + " void localClass(int... i) {}", + "", + " private static Stream lambdaReturnTestCases() {", + " int foo =", + " Optional.of(10)", + " .map(", + " i -> {", + " return i / 2;", + " })", + " .orElse(0);", + " return Stream.of(arguments(1), arguments(1, 2));", " }", + "", + " @ParameterizedTest", + " @MethodSource(\"lambdaReturnTestCases\")", + " void lambdaReturn(int... i) {}", "}") .doTest(); } @@ -301,6 +307,18 @@ void replacement() { " @ParameterizedTest", " @MethodSource(\"streamOfClassesAndClassArguments\")", " void string(Class c) {}", + "", + " private static Stream sameNameFactoryTestCases() {", + " return Stream.of(arguments(1));", + " }", + "", + " private static Stream sameNameFactoryTestCases(int overload) {", + " return Stream.of(arguments(overload));", + " }", + "", + " @ParameterizedTest", + " @MethodSource(\"sameNameFactoryTestCases\")", + " void sameNameFactory(int i) {}", "}") .addOutputLines( "A.java", @@ -401,6 +419,14 @@ void replacement() { " @ParameterizedTest", " @ValueSource(classes = {Stream.class, java.util.Map.class})", " void string(Class c) {}", + "", + " private static Stream sameNameFactoryTestCases(int overload) {", + " return Stream.of(arguments(overload));", + " }", + "", + " @ParameterizedTest", + " @ValueSource(ints = 1)", + " void sameNameFactory(int i) {}", "}") .doTest(TestMode.TEXT_MATCH); }