Expand JUnit3TestNotRun check to find non-public potential test methods.

RELNOTES: Expand JUnit3TestNotRun check to find non-public test methods. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=150481412
google · Mar 29, 2017 · c3bfaac · c3bfaac
1 parent 671f58c
commit c3bfaac
Show file tree

Hide file tree

Showing 5 changed files with 213 additions and 94 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java b/core/src/main/java/com/google/errorprone/bugpatterns/JUnit3TestNotRun.java
@@ -18,7 +18,12 @@
 
 import static com.google.errorprone.BugPattern.Category.JUNIT;
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.fixes.SuggestedFixes.addModifiers;
+import static com.google.errorprone.fixes.SuggestedFixes.removeModifiers;
+import static com.google.errorprone.fixes.SuggestedFixes.renameMethod;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.matchers.JUnitMatchers.isJUnit3TestClass;
+import static com.google.errorprone.matchers.JUnitMatchers.isJunit3TestCase;
 import static com.google.errorprone.matchers.JUnitMatchers.wouldRunInJUnit4;
 import static com.google.errorprone.matchers.Matchers.allOf;
 import static com.google.errorprone.matchers.Matchers.enclosingClass;
@@ -33,26 +38,21 @@
 import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
 import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFix.Builder;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
-import com.google.errorprone.matchers.Matchers;
 import com.sun.source.tree.MethodTree;
-import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.regex.Pattern;
 import javax.lang.model.element.Modifier;
 
 /** @author rburny@google.com (Radoslaw Burny) */
 @BugPattern(
   name = "JUnit3TestNotRun",
-  summary = "Test method will not be run; please prefix name with \"test\"",
+  summary =
-  explanation =
+      "Test method will not be run; please correct method signature "
-      "JUnit 3 requires that test method names start with \"test\". The method that"
+          + "(Should be public, non-static, and method name should begin with \"test\").",
-          + " triggered this error looks like it is supposed to be the test, but either"
-          + " misspells the required prefix, or has @Test annotation, but no prefix."
-          + " As a consequence, JUnit 3 will ignore it.\n\n"
-          + "If you want to disable test on purpose, change the name to something more descriptive,"
-          + " like \"disabledTestSomething()\". You don't need @Test annotation, but if you want to"
-          + " keep it, add @Ignore too.",
   category = JUNIT,
   severity = ERROR
 )
@@ -76,6 +76,13 @@ public class JUnit3TestNotRun extends BugChecker implements MethodTreeMatcher {
       "[tT][eE][sS][tT]"   // miscapitalized
       );
 
+  private static final Matcher<MethodTree> LOOKS_LIKE_TEST_CASE =
+      allOf(
+          enclosingClass(isJUnit3TestClass),
+          not(isJunit3TestCase),
+          methodReturns(VOID_TYPE),
+          methodHasParameters());
+
   /**
    * Matches if:
    * 1) Method's name begins with misspelled variation of "test".
@@ -85,32 +92,45 @@ public class JUnit3TestNotRun extends BugChecker implements MethodTreeMatcher {
    */
   @Override
   public Description matchMethod(MethodTree methodTree, VisitorState state) {
-    Matcher<MethodTree> methodMatcher = allOf(
+    if (!LOOKS_LIKE_TEST_CASE.matches(methodTree, state)) {
-        not(methodNameStartsWith("test")),
+      return NO_MATCH;
-        Matchers.<MethodTree>hasModifier(Modifier.PUBLIC),
+    }
-        methodReturns(VOID_TYPE),
+
-        methodHasParameters(),
+    List<SuggestedFix> fixes = new ArrayList<>(0);
-        enclosingClass(isJUnit3TestClass));
+
-    if (!methodMatcher.matches(methodTree, state)) {
+    if (not(methodNameStartsWith("test")).matches(methodTree, state)) {
-      return Description.NO_MATCH;
+      String fixedName = methodTree.getName().toString();
+      // N.B. regex.Matcher class name collides with errorprone.Matcher
+      java.util.regex.Matcher matcher = MISSPELLED_NAME.matcher(fixedName);
+      if (matcher.lookingAt()) {
+        fixedName = matcher.replaceFirst("test");
+      } else if (wouldRunInJUnit4.matches(methodTree, state)) {
+        fixedName = "test" + fixedName.substring(0, 1).toUpperCase() + fixedName.substring(1);
+      } else {
+        return NO_MATCH;
+      }
+      // Rename test method appropriately.
+      fixes.add(renameMethod(methodTree, fixedName, state));
     }
 
-    String name = methodTree.getName().toString();
+    // Make method public (if not already public).
-    String fixedName;
+    fixes.add(addModifiers(methodTree, state, Modifier.PUBLIC));
-    // regex.Matcher class name collides with errorprone.Matcher
+    // Remove any other visibility modifiers (if present).
-    java.util.regex.Matcher matcher = MISSPELLED_NAME.matcher(name);
+    fixes.add(removeModifiers(methodTree, state, Modifier.PRIVATE, Modifier.PROTECTED));
-    if (matcher.lookingAt()) {
+    // Remove static modifier (if present).
-      fixedName = matcher.replaceFirst("test");
+    // N.B. must occur in separate step because removeModifiers only removes one modifier at a time.
-    } else if (wouldRunInJUnit4.matches(methodTree, state)) {
+    fixes.add(removeModifiers(methodTree, state, Modifier.STATIC));
-      fixedName = "test" + name.substring(0, 1).toUpperCase() + name.substring(1);
+
-    } else {
+    return describeMatch(methodTree, mergeFixes(fixes));
-      return Description.NO_MATCH;
+  }
+
+  private static Fix mergeFixes(List<SuggestedFix> fixesToMerge) {
+    Builder builderForResult = SuggestedFix.builder();
+    for (SuggestedFix fix : fixesToMerge) {
+      if (fix != null) {
+        builderForResult.merge(fix);
+      }
     }
-    // We don't have start position for a method symbol, so we replace everything between result
+    return builderForResult.build();
-    // type and body.
-    JCMethodDecl decl = (JCMethodDecl) methodTree;
-    Fix fix = SuggestedFix.replace(
-        decl.restype.getStartPosition() + 4, decl.body.getStartPosition(), " " + fixedName + "() ");
-    return describeMatch(methodTree, fix);
   }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/JUnit3TestNotRunTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/JUnit3TestNotRunTest.java
@@ -16,8 +16,9 @@
 
 package com.google.errorprone.bugpatterns;
 
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
 import com.google.errorprone.CompilationTestHelper;
-import org.junit.Before;
+import java.io.IOException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -28,18 +29,147 @@
 @RunWith(JUnit4.class)
 public class JUnit3TestNotRunTest {
 
-  private CompilationTestHelper compilationHelper;
+  private final CompilationTestHelper compilationHelper =
-
+      CompilationTestHelper.newInstance(JUnit3TestNotRun.class, getClass());
-  @Before
+  private final BugCheckerRefactoringTestHelper refactorHelper =
-  public void setUp() {
+      BugCheckerRefactoringTestHelper.newInstance(new JUnit3TestNotRun(), getClass());
-    compilationHelper = CompilationTestHelper.newInstance(JUnit3TestNotRun.class, getClass());
-  }
 
   @Test
   public void testPositiveCases() throws Exception {
     compilationHelper.addSourceFile("JUnit3TestNotRunPositiveCases.java").doTest();
   }
 
+  @Test
+  public void misspelledTest() throws IOException {
+    refactorHelper
+        .addInputLines(
+            "in/PositiveCases.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class PositiveCases extends TestCase {",
+            "  public void tesName1() {}",
+            "  public void ttestName2() {}",
+            "  public void teestName3() {}",
+            "  public void tstName4() {}",
+            "  public void tetName5() {}",
+            "  public void etstName6() {}",
+            "  public void tsetName7() {}",
+            "  public void teatName8() {}",
+            "  public void TestName9() {}",
+            "  public void TEST_NAME_10() {}",
+            "  public void tesname11() {}",
+            "}")
+        .addOutputLines(
+            "out/PositiveCases.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class PositiveCases extends TestCase {",
+            "  public void testName1() {}",
+            "  public void testName2() {}",
+            "  public void testName3() {}",
+            "  public void testName4() {}",
+            "  public void testName5() {}",
+            "  public void testName6() {}",
+            "  public void testName7() {}",
+            "  public void testName8() {}",
+            "  public void testName9() {}",
+            "  public void test_NAME_10() {}",
+            "  public void testname11() {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void substitutionShouldBeWellFormed() throws IOException {
+    refactorHelper
+        .addInputLines(
+            "in/PositiveCases.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class PositiveCases extends TestCase {",
+            "  public void tesBasic() {}",
+            "  public    void    tesMoreSpaces(  )    {}",
+            "  @Test public void",
+            "      tesMultiline() {}",
+            "}")
+        .addOutputLines(
+            "out/PositiveCases.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class PositiveCases extends TestCase {",
+            "  public void testBasic() {}",
+            "  public void testMoreSpaces() {}",
+            "  @Test public void testMultiline() {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void privateNamedTest() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import junit.framework.TestCase;",
+            "public class Test extends TestCase {",
+            "  // BUG: Diagnostic contains: JUnit3TestNotRun",
+            "  private void testDoesStuff() {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void privateMisspelledTest() {
+    compilationHelper
+        .addSourceLines(
+            "Test.java",
+            "import junit.framework.TestCase;",
+            "public class Test extends TestCase {",
+            "  // BUG: Diagnostic contains: JUnit3TestNotRun",
+            "  private void tsetDoesStuff() {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void hasModifiersAndThrows() throws IOException {
+    refactorHelper
+        .addInputLines(
+            "in/DoesStuffTest.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class DoesStuffTest extends TestCase {",
+            "  @Test private static void tsetDoesStuff() throws Exception {}",
+            "}")
+        .addOutputLines(
+            "out/DoesStuffTest.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class DoesStuffTest extends TestCase {",
+            "  @Test public void testDoesStuff() throws Exception {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void noModifiers() throws IOException {
+    refactorHelper
+        .addInputLines(
+            "in/DoesStuffTest.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class DoesStuffTest extends TestCase {",
+            "  void tsetDoesStuff() {}",
+            "}")
+        .addOutputLines(
+            "out/DoesStuffTest.java",
+            "import junit.framework.TestCase;",
+            "import org.junit.Test;",
+            "public class DoesStuffTest extends TestCase {",
+            "  public void testDoesStuff() {}",
+            "}")
+        .doTest();
+  }
+
   @Test
   public void testNegativeCase1() throws Exception {
     compilationHelper.addSourceFile("JUnit3TestNotRunNegativeCase1.java").doTest();

diff --git a/...c/test/java/com/google/errorprone/bugpatterns/testdata/JUnit3TestNotRunNegativeCase1.java b/...c/test/java/com/google/errorprone/bugpatterns/testdata/JUnit3TestNotRunNegativeCase1.java
@@ -37,8 +37,10 @@ public void establish() {}
   public void estimate() {}
 
   // different signature
-  private void tesPrivateHelper() {}
+  public boolean teslaInventedLightbulb() {
-  public boolean teslaInventedLightbulb() {return true;}
+    return true;
+  }
+
   public void tesselate(float f) {}
 
   // surrounding class is not a JUnit3 TestCase

diff --git a/...c/test/java/com/google/errorprone/bugpatterns/testdata/JUnit3TestNotRunPositiveCases.java b/...c/test/java/com/google/errorprone/bugpatterns/testdata/JUnit3TestNotRunPositiveCases.java
@@ -23,69 +23,28 @@
  * @author rburny@google.com (Radoslaw Burny)
  */
 public class JUnit3TestNotRunPositiveCases extends TestCase {
-  // misspelled names
+  // BUG: Diagnostic contains: JUnit3TestNotRun
-  // BUG: Diagnostic contains: testName
-  public void tesName() {}
-
-  // BUG: Diagnostic contains: testNameStatic
   public static void tesNameStatic() {}
 
-  // BUG: Diagnostic contains: testName
-  public void ttestName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void teestName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void tstName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void tetName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void etstName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void tsetName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void teatName() {}
-
-  // BUG: Diagnostic contains: testName
-  public void TestName() {}
-
-  // BUG: Diagnostic contains: test_NAME
-  public void TEST_NAME() {}
-
   // These names are trickier to correct, but we should still indicate the bug
-  // BUG: Diagnostic contains: test
+  // BUG: Diagnostic contains: JUnit3TestNotRun
   public void tetsName() {}
 
-  // BUG: Diagnostic contains: test
+  // BUG: Diagnostic contains: JUnit3TestNotRun
   public void tesstName() {}
 
-  // BUG: Diagnostic contains: test
+  // BUG: Diagnostic contains: JUnit3TestNotRun
   public void tesetName() {}
 
-  // BUG: Diagnostic contains: test
+  // BUG: Diagnostic contains: JUnit3TestNotRun
   public void tesgName() {}
 
   // tentative - can cause false positives
-  // BUG: Diagnostic contains: testName
+  // BUG: Diagnostic contains: JUnit3TestNotRun
   public void textName() {}
 
   // test with @Test annotation not run by JUnit3
-  // BUG: Diagnostic contains: testName
+  @Test
-  @Test public void name() {}
+  // BUG: Diagnostic contains: JUnit3TestNotRun
-
+  public void name() {}
-  // a few checks to verify the substitution is well-formed
-  // BUG: Diagnostic contains: void testBasic() {
-  public void tesBasic() {}
-
-  // BUG: Diagnostic contains: void testMoreSpaces() {
-  public    void    tesMoreSpaces(  )    {}
-
-  @Test public void
-  // BUG: Diagnostic contains: void testMultiline() {
-      tesMultiline() {}
 }
diff --git a/docs/bugpattern/JUnit3TestNotRun.md b/docs/bugpattern/JUnit3TestNotRun.md
@@ -0,0 +1,8 @@
+JUnit 3 requires that test method names start with "`test`". The method that
+triggered this error looks like it is supposed to be a test, but misspells the
+required prefix; has `@Test` annotation, but no prefix; or has the wrong method
+signature. As a consequence, JUnit 3 will ignore it.
+
+If you meant to disable this test on purpose, or this is a helper method, change
+the name to something more descriptive, like "`disabledTestSomething()`". You
+don't need an `@Test` annotation, but if you want to keep it, add `@Ignore` too.