Introduce `ErrorProneTestHelperSourceFormat` check #147

Stephan202 · 2022-06-26T19:20:57Z

I've seen this being a topic of discussion several times (most recently here), so decided to do something about it. PTAL.

Suggested commit message:

Introduce `ErrorProneTestHelperSourceFormat` check (#147)

This new checker inspects inline code passed to `CompilationTestHelper` and
`BugCheckerRefactoringTestHelper` instances. It requires that this code is
properly formatted and that its imports are organized. Only code that
represents the expected output of a refactoring operation is allowed to have
unused imports, as most `BugChecker`s do not (and are not able to) remove
imports that become obsolete as a result of applying their suggested fix(es).

Stephan202

While working on the tests for this new checker I already saved a lot of time just running patch.sh. 😄

Stephan202 · 2022-06-26T19:24:00Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListingCheck.java

@@ -52,7 +52,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) {

    Optional<Fix> fix = tryFixOrdering(originalOrdering, sortedAnnotations, state);

-    Description.Builder description = buildDescription(tree);
+    Description.Builder description = buildDescription(originalOrdering.get(0));


This way this check reports the violation on the top-most annotation, which arguably is a better location. (Even better could be to report on the first out-of-order annotation, but not sure that's worth the hassle.)

Stephan202 · 2022-06-26T19:25:04Z

...rib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversionCheckTest.java

@@ -124,7 +124,7 @@ void identificationWithinBinaryOperation() {
            "      s + String.valueOf((String) null),",
            "      s + String.valueOf(null),",
            "      s + String.valueOf(new char[0]),",
-            "",
+            "      //",


This one and the one below are introduced to retain the original grouping. Seemed worthwhile.

rickie

Still couldn't figure out how to get the patch command to work with a self-apply on EPS. Would really love to see that working first. Would be nice if we could add that in the README.

This will make quite some people happy.

The code is really clean and nice IMO. Could find anything 🚀

rickie · 2022-07-04T12:48:21Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+import java.util.Optional;
+
+/**
+ * A {@link BugChecker} which flags and corrects improperly formatted Error Prone test code.


Since this is quite a specific check, I'd say it makes sense to at least mention GJF and perhaps refer to their docs?
Could be like a second paragraph? I like the explanation but it feels that GJF is missing from here.

Good point; added a sentence. (Linking to the correct/least ambiguous docs is tricky, so opted for mere Javadoc instead.

rickie · 2022-07-04T13:00:16Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+    int startPos = ASTHelpers.getStartPosition(sourceLines.get(0));
+    int endPos = state.getEndPosition(sourceLines.get(sourceLines.size() - 1));
+
+    /* Attempt to format the source code only if it fully consists of constant expression. */


Suggested change

/* Attempt to format the source code only if it fully consists of constant expression. */

/* Attempt to format the source code only if it fully consists of constant expressions. */

Right? As in, every line needs to be a constant.

rickie · 2022-07-06T06:49:23Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+      return Description.NO_MATCH;
+    }
+
+    List<? extends ExpressionTree> sourceLines =


Don't want to validate the name in a way? This is valid with the current setup:
.addSourceLines(\"D.java\", \"class C {}\")"

OTOH, I can see why the in/ and out/ setup can make things a bit harder. (Perhaps it's out of scope)

I like the idea of validating this, but indeed would like to defer that to another checker.

rickie · 2022-07-06T07:23:08Z

...ontrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java

@@ -26,71 +26,137 @@ void identification() {
            "import org.junit.jupiter.params.ParameterizedTest;",
            "",
            "class A {",
-            "  @BeforeAll void setUp1() {}",
+            "  {",


I fail to understand where this is coming from 👀.

It's to make sure that the static arguments import isn't removed by the formatter. We need to keep it because it's important for one of the tests below:

// BUG: Diagnostic contains: (but note that arguments is already statically imported)

Stephan202

Rebased and added a commit.

Still couldn't figure out how to get the patch command to work with a self-apply on EPS. Would really love to see that working first.

I filed #171.

Stephan202 · 2022-07-31T10:02:34Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+import java.util.Optional;
+
+/**
+ * A {@link BugChecker} which flags and corrects improperly formatted Error Prone test code.


Good point; added a sentence. (Linking to the correct/least ambiguous docs is tricky, so opted for mere Javadoc instead.

Stephan202 · 2022-07-31T10:04:18Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+      return Description.NO_MATCH;
+    }
+
+    List<? extends ExpressionTree> sourceLines =


I like the idea of validating this, but indeed would like to defer that to another checker.

Stephan202 · 2022-07-31T10:08:04Z

...ontrib/src/test/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclarationCheckTest.java

@@ -26,71 +26,137 @@ void identification() {
            "import org.junit.jupiter.params.ParameterizedTest;",
            "",
            "class A {",
-            "  @BeforeAll void setUp1() {}",
+            "  {",


It's to make sure that the static arguments import isn't removed by the formatter. We need to keep it because it's important for one of the tests below:

// BUG: Diagnostic contains: (but note that arguments is already statically imported)

rickie

Still think this is a really cool check 🚀🚀🚀.

Verified that it works by using the new .patch.sh 😄, works like a charm!

rickie · 2022-08-02T07:54:12Z

Made two tiny tweaks to the release note. Without the s it doesn't flow as good IMO.

Stephan202 · 2022-08-02T08:53:30Z

Made two tiny tweaks to the release note. Without the s it doesn't flow as good IMO.

As-is the text needs to be reflowed. The s-es were there for a reason, though: we're passing the data to instances of these types, not to static methods.

rickie · 2022-08-02T09:29:20Z

^ Made a new tweak, PTAL :).

Stephan202 · 2022-08-02T09:45:31Z

With "instances of" we should not have the s-es, since the grammar is is then "instances of <type>"`. Tweaked again 🙃

Badbond

Super super nice that we're going to have this now 👍 No more formatting discussions on test code content! Tried very hard to squeeze two comments out but approving regardless 👍

Badbond · 2022-08-03T13:04:36Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+import java.util.Optional;
+
+/**
+ * A {@link BugChecker} which flags and corrects improperly formatted Error Prone test code.


It is -XepPatchChecks that does the correcting right? IIUC, this BugChecker just flags it. Comitted suggestion.

Suggested change

* A {@link BugChecker} which flags and corrects improperly formatted Error Prone test code.

* A {@link BugChecker} which flags improperly formatted Error Prone test code.

Nice, this is consistent with other checks indeed.

Badbond · 2022-08-03T14:06:40Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+    String withReorderedImports = ImportOrderer.reorderImports(source, Style.GOOGLE);
+    String withRemovedImports =


The name of withReorderedImports makes sense, but withRemovedImports felt odd at the formatSource line as it intuitively indicates that imports were removed if applicable, while this is not the case if retainUnusedImports is true. However, I really don't have an alternative apart from adding 'optionally' to it, which I don't like either. Placing comment to poke your brains 🧠

Suggested change

String withReorderedImports = ImportOrderer.reorderImports(source, Style.GOOGLE);

String withRemovedImports =

String withReorderedImports = ImportOrderer.reorderImports(source, Style.GOOGLE);

String withOptionallyRemovedImports =

TBH, withOptionallyRemovedImports explains exactly what is going on here, so I kinda like it.

One other option could be something like: resultingImports or finalImports. To show that the variable will hold the "end result (a set of imports) that will be passed to the actual formatter".
Perhaps a synonym of final or result would be better here.

Like those suggestions, as long as we keep the with part, as it involves the entire source code. We could also do withExpectedReorderedImports where what we expect depends on the value of the parameter.

I think I like withOptionallyRemovedImports the most actually. That one is most clear/accurate/honest about what is happening IMO.

Committed. Feel free to further tweak @Stephan202.

Badbond · 2022-08-03T14:12:27Z

.../src/main/java/tech/picnic/errorprone/bugpatterns/ErrorProneTestHelperSourceFormatCheck.java

+            Splitter.on('\n')
+                .splitToStream(formatted)
+                .map(state::getConstantExpression)
+                .collect(joining(", "))));


Hehe ironically, this leaves potentially unformatted code 😄 But nice that apply-error-prone-suggestion.sh will also run fmt:format to tackle this 💪

Hmm, I think I'm missing that case, when will that happen 👀? Anyway, indeed nice it's already caught by the .sh.

This replaces the entire existing argument array with the result of the formatted source code split on newlines, Stringified (constant expression), joined with ,. This puts it all on a single line. You can test this combining line 60 - 62 in RequestParamTypeCheckTest.java (so new statement straight after ;). Then run apply-error-prone-suggestions.sh but without the fmt:format in there. Now you can see the output is that the entire argument array is on a single line. After running mvn fmt:format, everything looks as expected.

Ahh ofcourse, nice, good catch 😄.

This new checker inspects inline code passed to `CompilationTestHelper` and `BugCheckerRefactoringTestHelper`. It requires that this code is properly formatted and that its imports are organized. Only code that represents the expected output of a refactoring operation is allowed to have unused imports, as most `BugChecker`s do not (and are not able to) remove imports that become obsolete as a result of applying their suggested fix(es).

Stephan202 · 2022-08-03T17:59:57Z

I can live with that new name ;). Rebased; I think we can build if the PR passes.

Stephan202 added this to the 0.1.0 milestone Jun 26, 2022

Stephan202 requested review from Badbond and rickie June 26, 2022 19:20

Stephan202 mentioned this pull request Jun 26, 2022

Introduce ImmutablesSortedSetComparator check #102

Merged

Stephan202 commented Jun 26, 2022

View reviewed changes

rickie reviewed Jul 6, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/enforce-test-code-formatting branch from b91b16f to 223a393 Compare July 31, 2022 10:58

Stephan202 commented Jul 31, 2022

View reviewed changes

Stephan202 force-pushed the sschroevers/enforce-test-code-formatting branch from 223a393 to 99771b2 Compare August 1, 2022 18:55

rickie force-pushed the sschroevers/enforce-test-code-formatting branch from 99771b2 to 1789863 Compare August 2, 2022 07:44

rickie approved these changes Aug 2, 2022

View reviewed changes

Badbond approved these changes Aug 3, 2022

View reviewed changes

Stephan202 and others added 6 commits August 3, 2022 19:59

Slightly more realistic...

e3dbe3d

Suggestion

4d4d450

Suggestion

aab5ee2

Suggestion

d42b0e9

Rename local variable suggestion

37be08a

Stephan202 force-pushed the sschroevers/enforce-test-code-formatting branch from c785a02 to 37be08a Compare August 3, 2022 17:59

Stephan202 merged commit 38a57db into master Aug 3, 2022

Stephan202 deleted the sschroevers/enforce-test-code-formatting branch August 3, 2022 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `ErrorProneTestHelperSourceFormat` check #147

Introduce `ErrorProneTestHelperSourceFormat` check #147

Stephan202 commented Jun 26, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Jun 26, 2022

Stephan202 Jun 26, 2022

rickie left a comment

rickie Jul 4, 2022

Stephan202 Jul 31, 2022

rickie Jul 4, 2022

rickie Jul 6, 2022

rickie Jul 6, 2022

Stephan202 Jul 31, 2022

rickie Jul 6, 2022

Stephan202 Jul 31, 2022

Stephan202 left a comment

Stephan202 Jul 31, 2022

Stephan202 Jul 31, 2022

Stephan202 Jul 31, 2022

rickie left a comment

rickie commented Aug 2, 2022

Stephan202 commented Aug 2, 2022

rickie commented Aug 2, 2022

Stephan202 commented Aug 2, 2022 •

edited

Loading

Badbond left a comment •

edited

Loading

Badbond Aug 3, 2022 •

edited

Loading

rickie Aug 3, 2022

Badbond Aug 3, 2022 •

edited

Loading

rickie Aug 3, 2022

Badbond Aug 3, 2022 •

edited

Loading

rickie Aug 3, 2022

Badbond Aug 3, 2022

Badbond Aug 3, 2022

rickie Aug 3, 2022

Badbond Aug 3, 2022 •

edited

Loading

rickie Aug 3, 2022

Stephan202 commented Aug 3, 2022

	/* Attempt to format the source code only if it fully consists of constant expression. */
	/* Attempt to format the source code only if it fully consists of constant expressions. */

	* A {@link BugChecker} which flags and corrects improperly formatted Error Prone test code.
	* A {@link BugChecker} which flags improperly formatted Error Prone test code.

		String withReorderedImports = ImportOrderer.reorderImports(source, Style.GOOGLE);
		String withRemovedImports =

Introduce ErrorProneTestHelperSourceFormat check #147

Introduce ErrorProneTestHelperSourceFormat check #147

Conversation

Stephan202 commented Jun 26, 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

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

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

rickie left a comment

Choose a reason for hiding this comment

rickie commented Aug 2, 2022

Stephan202 commented Aug 2, 2022

rickie commented Aug 2, 2022

Stephan202 commented Aug 2, 2022 • edited Loading

Badbond left a comment • edited Loading

Choose a reason for hiding this comment

Badbond Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Badbond Aug 3, 2022 • edited Loading

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

Badbond Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Aug 3, 2022

Introduce `ErrorProneTestHelperSourceFormat` check #147

Introduce `ErrorProneTestHelperSourceFormat` check #147

Stephan202 commented Jun 26, 2022 •

edited

Loading

Stephan202 commented Aug 2, 2022 •

edited

Loading

Badbond left a comment •

edited

Loading

Badbond Aug 3, 2022 •

edited

Loading

Badbond Aug 3, 2022 •

edited

Loading

Badbond Aug 3, 2022 •

edited

Loading

Badbond Aug 3, 2022 •

edited

Loading