Introduce `JUnitValueSource` check #188

oxkitsune · 2022-08-11T12:57:23Z

Implement BugPattern to migrate parameterized JUnit tests with a single argument that's known at compile time to the @ValueSource annotation.

The @ValueSource annotation supports:

all primitives and their java.lang wrapper
java.lang.String
java.lang.Class

Example:

- @ParameterizedTest
- @MethodSource("fooProvider")
- void foo (int number) {
-   assertThat(number).isNotEqualTo(0);
- }
- 
- private static Stream<Arguments> fooProvider () {
-   return Stream.of(
-     arguments(1),
-     arguments(2)
-   );
- }

+ @ParameterizedTest
+ @ValueSource(ints = {1, 2})
+ void foo (int number) {
+   assertThat(number).isNotEqualTo(0);
+ }

Suggested commit message:

Introduce `JUnitValueSource` check (#188)

rickie

A parameterized test to check the parameterized test is some mind blowing stuff 🤯.
Pretty cool stuff man. This definitely is a nice check!

I'm in doubt whether it's the right approach for this PR. To be honest, to make the test easier to comprehend it'd actually lean towards not using this setup.

For the identification test it'll be quite some code but make the test way easier to read.
In the identification we can show all the variants it can flag. In the replacement test, we don't need to test every variant that we can rewrite. It'll be enough to show how the replacement is performed and that it works.

If we do it like that, I think the size will be reasonable still.

Perhaps @Stephan202 can give some input as well.

WDYT of this proposal @oxkitsune?

I left some other comments as well :).

A general note on method names:
Try to look at the method names in "isolation" do they explain what they are really doing? Is it clear what we're trying to achieve with that method?

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

rickie

Flushing some comments.

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

rickie · 2022-08-24T11:53:44Z

(Note, this was a WIP commit)

rickie

By changing the way we use the Matcher at the start we can already rule out some checks. Now we know it is a method that is eligible to rewrite. That makes it easier. We can make some more assumptions (that we are sure of), to make the code more elegant.

Let's discuss tomorrow morning, would be nice to add some extra explanation here and there.

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java

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

rickie

Hmm, one big open point for me is how we currently handle the Stream.of(1, 2).map(Arguments::arguments) in the code. It is an edge case that prevents us from making the code quite a bit cleaner. Will have to get back to that 🤔.

Added a commit and left some comments.

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

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java

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

rickie

Forgot to remove this and the comment above that.

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

rickie

Added a few commits. PTAL @oxkitsune 😄.

There are two XXX's which I consider to be out of scope for now but important to write down.

The PR is now ready for your 👀 @Stephan202.

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

Stephan202

Rebased and added a commit. Cool feature!

You will notice that I refactored quite a bit. While I do agree that we should be reasonable when defining the scope of a test, we should:

Err on the side of correctness, but to avoid false-positives and to ensure a good first impression of users who try out the library.
(As a result) aim for very high test coverage.
(Where reasonable) try to support code bases which aren't as uniform as Picnic's.

Still TODO:

Extend test coverage, primarily "should not match" cases. (See the XXX comments I left.)
~One more full pass over the code.

Stephan202 · 2022-10-29T11:55:22Z

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

+    AnnotationTree annotationTree =
+        ASTHelpers.getAnnotationWithSimpleName(
+            tree.getModifiers().getAnnotations(), "MethodSource");


We can avoid this if we use the annotations matcher defined above; will push a proposal.

Stephan202 · 2022-10-29T11:56:30Z

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

+            tree.getModifiers().getAnnotations(), "MethodSource");
+
+    return tryConstructValueSourceFix(parameterType, annotationTree, state)
+        .map(fix -> describeMatch(tree, fix.build()))


Since there's only one caller, we can push the .build() into tryConstructValueSourceFix.

Right now we're flagging the issue on tree, but I think it's more intuitive if annotationTree is flagged.

Stephan202 · 2022-10-29T12:00:39Z

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

+      case "Integer":
+        return "ints";
+      default:
+        return type.toLowerCase() + "s";


Suggested change

return type.toLowerCase() + "s";

return type.toLowerCase(Locale.ROOT) + "s";

(We should have a check for this 🤔.)

Stephan202 · 2022-10-29T12:05:51Z

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

+            .filter(MethodInvocationTree.class::isInstance)
+            .map(MethodInvocationTree.class::cast)


This surprised me a bit, until I realized that this is about unwrapping the argument(...) call. Let's make that more explicit.

Edit: the code I'll push doesn't have this limitation.

Stephan202 · 2022-10-29T12:07:41Z

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

+            .flatMap(mit -> mit.getArguments().stream())
+            .filter(JUnitValueSource::isCompileTimeConstant)


In combination with the test below we're assuming that the arguments(...) call accepts precisely one argument, while there could be multiple (for example if the value factory is used in multiple places (and those places may be in another class)).

Stephan202 · 2022-10-29T14:02:27Z

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

+                    methodSourceAnnotation,
+                    String.format(
+                        "@ValueSource(%s = {%s})",
+                        toValueSourceAttributeName(parameterType.toString()), attributeValue))


The .toString() here will yield e.g. java.lang.String, which is not what is intended; the resultant code won't compile. (There should be test cases for each case in toValueSourceAttributeName.)

NB: For cases like these it's useful to run ./run-mutation-tests.sh, then inspect error-prone-contrib/target/pit-reports/index.html. Integrating PIT with the build is on the TODO list.

For generic types such as Class<?> this will also create an invalid @ValueSource argument.

Stephan202 · 2022-10-29T14:46:20Z

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

+  private static Optional<ReturnTree> getReturnTree(MethodTree methodTree) {
+    return methodTree.getBody().getStatements().stream()
+        .filter(ReturnTree.class::isInstance)
+        .findFirst()


If there is more than one we should back out, not just take the first one.

Stephan202 · 2022-10-30T08:15:52Z

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

+        .filter(method -> method.getName().contentEquals(factoryMethodName))
+        .findFirst()


There may be overloads; we need to select the nullary version.

Stephan202 · 2022-10-30T10:55:52Z

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

+  private static final Matcher<ExpressionTree> STREAM_OF_ARGUMENTS =
+      staticMethod().onClass(Stream.class.getName()).named("of");


This doesn't actually test that the values are arguments 🤔 (While the code below does assume that the Stream.of arguments are method invocations.)

Stephan202 · 2022-10-30T14:53:09Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java

+            "",
+            "  private static Stream<Arguments> fooTestCases() {",
+            "    return Stream.of(arguments('a'), arguments('b'), arguments(CONST_CHAR));",
+            "  }",


Internally we generally place the value factory methods before the associated test method; suggest we do the same here.

rickie · 2022-11-01T08:43:20Z

Oops, just meant to re-request review from myself 😬.

Stephan202

@oxkitsune have you started working on the tests? Asking to prevent double work :).

Stephan202 · 2022-11-05T19:18:15Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java

+            "",
+            "  @ParameterizedTest",
+            "  @MethodSource(\"streamOfClassesAndClassArguments\")",
+            "  void string(Class<?> c) {}",


Suggested change

" void string(Class<?> c) {}",

" void clazz(Class<?> c) {}",

(Don't have the code checked out right now; something to fix later.)

oxkitsune · 2022-11-05T19:29:21Z

@oxkitsune have you started working on the tests? Asking to prevent double work :).

I have!

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

github-actions · 2023-02-22T21:09:45Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	1	72
🧟`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	1	7
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource$1`	1	0

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

Stephan202

So a bit more work is needed. Can have a closer look later.

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

Stephan202

Documenting some additional caveats; will try to cover these in one go.

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

Stephan202

Rebased and added one more commit. Documented as many limitations as I could identify; those are for another day.

Suggested commit message:

Introduce `JUnitValueSource` check (#188)

This new check replaces JUnit `@MethodSource` usages with an equivalent
`@ValueSource` annotation where possible.

Stephan202 · 2023-02-26T13:45:35Z

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

+  public JUnitValueSource() {}
+
+  @Override
+  public Description matchMethod(MethodTree tree, VisitorState state) {


Had another look at this. It's easier not to special-case the nullary () syntax; instead we can back-off if there are overloads.

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

github-actions · 2023-02-26T14:06:35Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	2	69
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	0	8

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

Stephan202

Added one more commit.

Stephan202 · 2023-02-26T19:24:30Z

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

+
+  private static Matcher<ExpressionTree> isArrayArgumentValueCandidate() {
+    return anyOf(
+        classLiteral((tree, state) -> true), (tree, state) -> ASTHelpers.constValue(tree) != null);


Just realized we can write this in a nicer way.

github-actions · 2023-02-26T19:27:27Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	2	67
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	0	8

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

rickie

Yoooooo the quality and epicness of this check 🤯🤯🤯.

Great improvements @Stephan202 , let's get this beast merged and see the impact 😄.

Added a commit to drop a space and will rebase. After that when its 🍏 ==> merge!

github-actions · 2023-03-02T09:35:38Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	2	67
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	0	8

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

github-actions · 2023-03-02T09:36:56Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource`	2	67
🧟`tech.picnic.errorprone.bugpatterns.JUnitValueSource$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.util.MoreJUnitMatchers`	0	8

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

rickie self-requested a review August 11, 2022 12:59

rickie requested changes Aug 11, 2022

View reviewed changes

rickie requested changes Aug 24, 2022

View reviewed changes

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitValueSourceTest.java Outdated Show resolved Hide resolved

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

rickie added the new feature label Aug 25, 2022

rickie added this to the 0.2.1 milestone Sep 9, 2022

rickie force-pushed the gdejong/PSM-1552 branch from f76cdc8 to 893f65b Compare September 13, 2022 12:10

rickie reviewed Sep 13, 2022

View reviewed changes

rickie requested changes Sep 16, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

rickie requested changes Sep 17, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

oxkitsune requested a review from rickie September 19, 2022 09:21

rickie modified the milestones: 0.3.0, 0.4.0 Sep 19, 2022

rickie modified the milestones: 0.4.0, 0.5.0 Oct 5, 2022

oxkitsune force-pushed the gdejong/PSM-1552 branch from 0f0786e to a3b8fb7 Compare October 19, 2022 07:57

rickie approved these changes Oct 24, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

rickie requested a review from Stephan202 October 24, 2022 12:57

Stephan202 force-pushed the gdejong/PSM-1552 branch from 3214894 to fcfd455 Compare October 30, 2022 19:21

Stephan202 reviewed Oct 30, 2022

View reviewed changes

rickie requested review from Stephan202 and rickie November 1, 2022 08:42

rickie modified the milestones: 0.5.0, 0.6.0 Nov 1, 2022

eric-picnic mentioned this pull request Nov 4, 2022

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

Merged

Stephan202 reviewed Nov 5, 2022

View reviewed changes

Stephan202 reviewed Nov 13, 2022

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

Stephan202 force-pushed the gdejong/PSM-1552 branch from 07cdbb1 to 08c6784 Compare February 22, 2023 21:06

Stephan202 reviewed Feb 23, 2023

View reviewed changes

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreJUnitMatchers.java Outdated Show resolved Hide resolved

Stephan202 reviewed Feb 23, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Show resolved Hide resolved

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java Outdated Show resolved Hide resolved

Stephan202 force-pushed the gdejong/PSM-1552 branch from 08c6784 to 7e25353 Compare February 26, 2023 14:03

Stephan202 approved these changes Feb 26, 2023

View reviewed changes

Stephan202 reviewed Feb 26, 2023

View reviewed changes

rickie approved these changes Mar 2, 2023

View reviewed changes

oxkitsune and others added 15 commits March 2, 2023 10:33

Introduce JUnitValueSource check

158ccd8

Add missing javadoc

417e806

Improve Javadoc

f0b2b3f

Apply suggestion

f0f1789

Minor suggestion

7403cfc

Kill some surviving mutants

178eff9

Kill more pesky mutants

16987bb

Kill RemoveConditionalMutator_EQUAL_IF mutant

8c31296

Kill some more mutants

8024e70

Suggestions

f1da35e

Post-rebase fix.

4902b07

PSM-1552 Suggestions

6ec9454

More suggestions

696df90

Some more tweaks

84a4ff1

Suggestion

e03f680

rickie force-pushed the gdejong/PSM-1552 branch from 05ba782 to e03f680 Compare March 2, 2023 09:34

rickie merged commit 2d972fd into master Mar 2, 2023

rickie deleted the gdejong/PSM-1552 branch March 2, 2023 09:45

	return type.toLowerCase() + "s";
	return type.toLowerCase(Locale.ROOT) + "s";

		.filter(MethodInvocationTree.class::isInstance)
		.map(MethodInvocationTree.class::cast)

		.flatMap(mit -> mit.getArguments().stream())
		.filter(JUnitValueSource::isCompileTimeConstant)

		.filter(method -> method.getName().contentEquals(factoryMethodName))
		.findFirst()

		private static final Matcher<ExpressionTree> STREAM_OF_ARGUMENTS =
		staticMethod().onClass(Stream.class.getName()).named("of");

Introduce JUnitValueSource check #188

Introduce JUnitValueSource check #188

Conversation

oxkitsune commented Aug 11, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie commented Aug 24, 2022

rickie left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

rickie left a 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

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

Choose a reason for hiding this comment

rickie commented Nov 1, 2022 • edited Loading

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

oxkitsune commented Nov 5, 2022

github-actions bot commented Feb 22, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 left a 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

github-actions bot commented Feb 26, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2023

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 2, 2023

github-actions bot commented Mar 2, 2023

Introduce `JUnitValueSource` check #188

Introduce `JUnitValueSource` check #188

rickie commented Nov 1, 2022 •

edited

Loading