Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields #442

rickie · 2023-01-04T10:21:41Z

rickie · 2023-01-04T10:22:17Z

Suggested commit message:

Inline single-use `CompilationTestHelper` and `BugCheckerRefactoringTestHelper` variables (#442)

github-actions · 2023-01-04T10:23:34Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Ah, I think we had a misunderstanding. We can simply inline all helpers that are used in only a single test, right? (And perhaps even if they are reused?)

github-actions · 2023-01-06T14:30:35Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-06T14:32:54Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-01-06T14:34:25Z

The only one where it would make the test clearer is the RefasterTest. Now that test is a bit too big if you ask me. WDYT?

Stephan202

Quick Github UI-based review :)

Stephan202 · 2023-01-06T14:31:06Z

refaster-runner/src/test/java/tech/picnic/errorprone/refaster/runner/RefasterTest.java

-  private final CompilationTestHelper compilationHelper =
-      CompilationTestHelper.newInstance(Refaster.class, getClass())
-          .matchAllDiagnostics()
-          .expectErrorMessage(
-              "StringOfSizeZeroRule",
-              containsPattern(
-                  "\\[Refaster Rule\\] FooRules\\.StringOfSizeZeroRule: Refactoring opportunity\\s+.+\\s+"))
-          .expectErrorMessage(
-              "StringOfSizeOneRule",
-              containsPattern(
-                  "\\[Refaster Rule\\] FooRules\\.StringOfSizeOneRule: "
-                      + "A custom description about matching single-char strings\\s+.+\\s+"
-                      + "\\(see https://error-prone.picnic.tech/refasterrules/FooRules#StringOfSizeOneRule\\)"))
-          .expectErrorMessage(
-              "StringOfSizeTwoRule",
-              containsPattern(
-                  "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeTwoRule: "
-                      + "A custom subgroup description\\s+.+\\s+"
-                      + "\\(see https://example.com/rule/FooRules#ExtraGrouping.StringOfSizeTwoRule\\)"))
-          .expectErrorMessage(
-              "StringOfSizeThreeRule",
-              containsPattern(
-                  "\\[Refaster Rule\\] FooRules\\.ExtraGrouping\\.StringOfSizeThreeRule: "
-                      + "A custom description about matching three-char strings\\s+.+\\s+"
-                      + "\\(see https://example.com/custom\\)"));


This one is now duplicated 😬

Stephan202 · 2023-01-06T14:31:37Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ThirdPartyLibraryTest.java

-  private final CompilationTestHelper compilationTestHelper =
-      CompilationTestHelper.newInstance(TestChecker.class, getClass());


Here 100% of tests use this field; I'd keep this one as-is.

Stephan202 · 2023-01-06T14:32:08Z

...rone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StringCaseLocaleUsageTest.java

@@ -54,7 +48,7 @@ void identification() {

  @Test
  void replacementFirstSuggestedFix() {
-    refactoringTestHelper
+    newInstance(StringCaseLocaleUsage.class, getClass())


Suggested change

newInstance(StringCaseLocaleUsage.class, getClass())

BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())

Likewise below.

Started working on that upstream fix. Will do that after applying this feedback.

Opened: PicnicSupermarket/error-prone#58.

Stephan202 · 2023-01-06T14:33:26Z

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

-  // XXX: Can we perhaps work-around this by describing the fixes in reverse order?
-
-  // The logic for `char` and `short` is exactly analogous to the `byte` case.
+  /* The logic for `char` and `short` is exactly analogous to the `byte` case. */


Suggested change

/* The logic for `char` and `short` is exactly analogous to the `byte` case. */

/** @implNote The logic for `char` and `short` is exactly analogous to the `byte` case. */

Perhaps.

Stephan202 · 2023-01-06T14:34:21Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversionTest.java

@@ -10,19 +10,9 @@
 // `String.valueOf(null)` may not. That is because the latter matches `String#valueOf(char[])`. We
 // could special-case `null` arguments, but that doesn't seem worth the trouble.
 final class RedundantStringConversionTest {


This one is an edge case, w.r.t. field reuse 🤔

rickie

Added a commit with all feedback fixed.

github-actions · 2023-01-06T14:40:51Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

chamil-prabodha · 2023-01-06T16:08:03Z

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

@@ -73,7 +67,7 @@ void identification() {

  @Test
  void replacementFirstSuggestedFix() {
-    refactoringTestHelper
+    newInstance(FluxFlatMapUsage.class, getClass())


We can apply the same for the method below as well.

Suggested change

newInstance(FluxFlatMapUsage.class, getClass())

BugCheckerRefactoringTestHelper.newInstance(FluxFlatMapUsage.class, getClass())

Stephan202

Added one more commit. Nice!

Suggested commit message:

Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields (#442)

github-actions · 2023-01-06T23:22:38Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-01-08T11:20:52Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-01-08T11:31:47Z

Made a tweak to the suggested commit message and sorted the two entries. This fits the number of characters, WDYT?

Stephan202 · 2023-01-08T11:38:26Z

Works for me 👍

rickie added the improvement label Jan 4, 2023

rickie requested review from Stephan202 and chamil-prabodha January 4, 2023 10:21

rickie mentioned this pull request Jan 4, 2023

Have RequestParamType ignore parameter types with custom deserialization support #426

Merged

rickie added this to the 0.7.0 milestone Jan 4, 2023

Stephan202 reviewed Jan 4, 2023

View reviewed changes

chamil-prabodha approved these changes Jan 4, 2023

View reviewed changes

rickie modified the milestones: 0.7.0, 0.8.0 Jan 6, 2023

rickie force-pushed the rossendrijver/inline_restricted_compilation_helpers branch from 2e5527f to d4ce9e6 Compare January 6, 2023 14:30

rickie requested a review from chamil-prabodha January 6, 2023 14:34

Stephan202 reviewed Jan 6, 2023

View reviewed changes

rickie commented Jan 6, 2023

View reviewed changes

chamil-prabodha reviewed Jan 6, 2023

View reviewed changes

Stephan202 approved these changes Jan 6, 2023

View reviewed changes

rickie and others added 4 commits January 8, 2023 12:18

Inline CompilationTestHelpers with additional arguments

0a193a3

Inline all CompilationTestHelpers and BugCheckerRefactoringTestHelpers

b7e7c6c

Revert a few files and apply feedback

ea508b1

Suggestions

5077920

rickie force-pushed the rossendrijver/inline_restricted_compilation_helpers branch from d64afa2 to 5077920 Compare January 8, 2023 11:18

rickie changed the title ~~Inline CompilationTestHelpers that specify command-line arguments~~ Inline most {BugCheckerRefactoring,Compilation}TestHelper fields Jan 8, 2023

rickie merged commit 9bd4b16 into master Jan 8, 2023

rickie deleted the rossendrijver/inline_restricted_compilation_helpers branch January 8, 2023 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields #442

Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields #442

rickie commented Jan 4, 2023

rickie commented Jan 4, 2023 •

edited

github-actions bot commented Jan 4, 2023

Stephan202 left a comment

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

rickie commented Jan 6, 2023

Stephan202 left a comment

Stephan202 Jan 6, 2023

Stephan202 Jan 6, 2023

Stephan202 Jan 6, 2023

rickie Jan 6, 2023

rickie Jan 6, 2023

Stephan202 Jan 6, 2023

Stephan202 Jan 6, 2023

rickie left a comment

github-actions bot commented Jan 6, 2023

chamil-prabodha Jan 6, 2023

Stephan202 left a comment •

edited by rickie

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 8, 2023

rickie commented Jan 8, 2023

Stephan202 commented Jan 8, 2023

		private final CompilationTestHelper compilationTestHelper =
		CompilationTestHelper.newInstance(TestChecker.class, getClass());

	newInstance(StringCaseLocaleUsage.class, getClass())
	BugCheckerRefactoringTestHelper.newInstance(StringCaseLocaleUsage.class, getClass())

	/* The logic for `char` and `short` is exactly analogous to the `byte` case. */
	/** @implNote The logic for `char` and `short` is exactly analogous to the `byte` case. */

	newInstance(FluxFlatMapUsage.class, getClass())
	BugCheckerRefactoringTestHelper.newInstance(FluxFlatMapUsage.class, getClass())

Inline most {BugCheckerRefactoring,Compilation}TestHelper fields #442

Inline most {BugCheckerRefactoring,Compilation}TestHelper fields #442

Conversation

rickie commented Jan 4, 2023

rickie commented Jan 4, 2023 • edited

github-actions bot commented Jan 4, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 6, 2023

rickie commented Jan 6, 2023

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

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2023

Choose a reason for hiding this comment

Stephan202 left a comment • edited by rickie

Choose a reason for hiding this comment

github-actions bot commented Jan 6, 2023

github-actions bot commented Jan 8, 2023

rickie commented Jan 8, 2023

Stephan202 commented Jan 8, 2023

Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields #442

Inline most `{BugCheckerRefactoring,Compilation}TestHelper` fields #442

rickie commented Jan 4, 2023 •

edited

Stephan202 left a comment •

edited by rickie