Add a bugpattern to catch test helpers which require a terminating

method to be called, but it appears to have been missed.

RELNOTES: [MissingTestCall] Flag missing terminating calls to helpers like EqualsTester.

MOE_MIGRATED_REVID=208017024

check_api/src/main/java/com/google/errorprone/matchers/JUnitMatchers.java

@@ -195,6 +195,9 @@ public class JUnitMatchers {
           hasAnnotationOnAnyOverriddenMethod(JUNIT4_TEST_ANNOTATION),
           not(hasAnnotationOnAnyOverriddenMethod(JUNIT4_IGNORE_ANNOTATION)));
 
+  /** Matches a JUnit 3 or 4 test case. */
+  public static final Matcher<MethodTree> TEST_CASE =
+      anyOf(isJunit3TestCase, hasAnnotation(JUNIT4_TEST_ANNOTATION));
   /**
    * A list of test runners that this matcher should look for in the @RunWith annotation. Subclasses
    * of the test runners are also matched.

core/src/main/java/com/google/errorprone/bugpatterns/CatchFail.java

@@ -20,11 +20,8 @@
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
-import static com.google.errorprone.matchers.JUnitMatchers.JUNIT4_TEST_ANNOTATION;
-import static com.google.errorprone.matchers.JUnitMatchers.isJunit3TestCase;
 import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.expressionStatement;
-import static com.google.errorprone.matchers.Matchers.hasAnnotation;
 import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod;
 import static java.util.stream.Collectors.joining;
 
@@ -84,9 +81,6 @@ public class CatchFail extends BugChecker implements TryTreeMatcher {
               staticMethod().onClass("junit.framework.Assert").named("fail"),
               staticMethod().onClass("junit.framework.TestCase").named("fail")));
 
-  public static final Matcher<MethodTree> TEST_CASE =
-      anyOf(isJunit3TestCase, hasAnnotation(JUNIT4_TEST_ANNOTATION));
-
   @Override
   public Description matchTry(TryTree tree, VisitorState state) {
     if (tree.getCatches().isEmpty()) {
@@ -202,7 +196,7 @@ Optional<Fix> deleteFix(TryTree tree, ImmutableList<CatchTree> catchBlocks, Visi
             .filter(t -> thrownTypes.stream().noneMatch(x -> types.isAssignable(t, x)))
             .collect(toImmutableList());
     if (!toThrow.isEmpty()) {
-      if (!TEST_CASE.matches(enclosing, state)) {
+      if (!JUnitMatchers.TEST_CASE.matches(enclosing, state)) {
         // Don't add throws declarations to methods that don't look like test cases, since it may
         // not be a safe local refactoring.
         return Optional.empty();

core/src/main/java/com/google/errorprone/bugpatterns/MissingTestCall.java

@@ -0,0 +1,167 @@
+/*
+ * Copyright 2018 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.common.collect.Iterables.getLast;
+import static com.google.errorprone.BugPattern.ProvidesFix.NO_FIX;
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+import static com.google.errorprone.util.ASTHelpers.getReceiver;
+import static com.google.errorprone.util.ASTHelpers.getSymbol;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.JUnitMatchers;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.method.MethodMatchers.MethodClassMatcher;
+import com.sun.source.tree.BlockTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.MethodTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.util.TreePath;
+import com.sun.source.util.TreePathScanner;
+import com.sun.tools.javac.code.Symbol;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.regex.Pattern;
+import javax.annotation.Nullable;
+import javax.lang.model.element.ElementKind;
+
+/**
+ * Matches test helpers which require a terminating method to be called.
+ *
+ * @author ghm@google.com (Graeme Morgan)
+ */
+@BugPattern(
+    name = "MissingTestCall",
+    summary = "A terminating method call is required for a test helper to have any effect.",
+    severity = ERROR,
+    providesFix = NO_FIX)
+public final class MissingTestCall extends BugChecker implements MethodTreeMatcher {
+
+  private static final MethodClassMatcher EQUALS_TESTER =
+      instanceMethod().onDescendantOf("com.google.common.testing.EqualsTester");
+  private static final MethodClassMatcher REFACTORING_HELPER =
+      instanceMethod().onDescendantOf("com.google.errorprone.BugCheckerRefactoringTestHelper");
+  private static final MethodClassMatcher COMPILATION_HELPER =
+      instanceMethod().onDescendantOf("com.google.errorprone.CompilationTestHelper");
+
+  private static final ImmutableSet<MethodPairing> PAIRINGS =
+      ImmutableSet.of(
+          MethodPairing.of(
+              "EqualsTester",
+              EQUALS_TESTER.named("addEqualityGroup"),
+              EQUALS_TESTER.named("testEquals")),
+          MethodPairing.of(
+              "BugCheckerRefactoringTestHelper",
+              REFACTORING_HELPER.withNameMatching(Pattern.compile("add(Input|Output)(File)?")),
+              REFACTORING_HELPER.named("doTest")),
+          MethodPairing.of(
+              "CompilationTestHelper",
+              COMPILATION_HELPER.withNameMatching(Pattern.compile("addSourceLines|addSourceFile")),
+              COMPILATION_HELPER.named("doTest")));
+
+  @Override
+  public Description matchMethod(MethodTree tree, VisitorState state) {
+    if (!JUnitMatchers.TEST_CASE.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    Set<MethodPairing> required = new HashSet<>();
+    Set<MethodPairing> called = new HashSet<>();
+    new TreePathScanner<Void, Void>() {
+      @Override
+      public Void visitMethodInvocation(MethodInvocationTree node, Void unused) {
+        for (MethodPairing pairing : PAIRINGS) {
+          if (pairing.ifCall().matches(node, state)) {
+            if (!isField(getUltimateReceiver(node))
+                || isLastStatementInBlock(state.findPathToEnclosing(StatementTree.class))) {
+              required.add(pairing);
+            }
+          }
+          if (pairing.mustCall().matches(node, state)) {
+            called.add(pairing);
+          }
+        }
+        return super.visitMethodInvocation(node, null);
+      }
+    }.scan(state.getPath(), null);
+    return Sets.difference(required, called)
+        .stream()
+        .findFirst()
+        .map(
+            p ->
+                buildDescription(tree)
+                    .setMessage(
+                        String.format(
+                            "%s requires a terminating method call to have any effect.", p.name()))
+                    .build())
+        .orElse(NO_MATCH);
+  }
+
+  @Nullable
+  private static ExpressionTree getUltimateReceiver(ExpressionTree tree) {
+    ExpressionTree receiver = getReceiver(tree);
+    while (receiver instanceof MemberSelectTree || receiver instanceof MethodInvocationTree) {
+      tree = receiver;
+      receiver = getReceiver(receiver);
+    }
+    return receiver;
+  }
+
+  private static boolean isField(@Nullable ExpressionTree tree) {
+    if (!(tree instanceof IdentifierTree)) {
+      return false;
+    }
+    Symbol symbol = getSymbol(tree);
+    return symbol != null && symbol.getKind() == ElementKind.FIELD;
+  }
+
+  private static boolean isLastStatementInBlock(@Nullable TreePath pathToStatement) {
+    if (pathToStatement == null) {
+      return false;
+    }
+    Tree parent = pathToStatement.getParentPath().getLeaf();
+    if (!(parent instanceof BlockTree)) {
+      return false;
+    }
+    return getLast(((BlockTree) parent).getStatements()).equals(pathToStatement.getLeaf());
+  }
+
+  @AutoValue
+  abstract static class MethodPairing {
+    abstract String name();
+
+    abstract Matcher<ExpressionTree> ifCall();
+
+    abstract Matcher<ExpressionTree> mustCall();
+
+    private static MethodPairing of(
+        String name, Matcher<ExpressionTree> ifCall, Matcher<ExpressionTree> mustCall) {
+      return new AutoValue_MissingTestCall_MethodPairing(name, ifCall, mustCall);
+    }
+  }
+}

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -146,6 +146,7 @@
 import com.google.errorprone.bugpatterns.MissingFail;
 import com.google.errorprone.bugpatterns.MissingOverride;
 import com.google.errorprone.bugpatterns.MissingSuperCall;
+import com.google.errorprone.bugpatterns.MissingTestCall;
 import com.google.errorprone.bugpatterns.MisusedWeekYear;
 import com.google.errorprone.bugpatterns.MixedArrayDimensions;
 import com.google.errorprone.bugpatterns.MockitoCast;
@@ -447,6 +448,7 @@ public static ScannerSupplier errorChecks() {
           MathRoundIntLong.class,
           MislabeledAndroidString.class,
           MissingSuperCall.class,
+          MissingTestCall.class,
           MisusedWeekYear.class,
           MockitoCast.class,
           MockitoUsage.class,

core/src/test/java/com/google/errorprone/bugpatterns/MissingTestCallTest.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright 2018 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for {@link MissingTestCallTest} bugpattern.
+ *
+ * @author ghm@google.com (Graeme Morgan)
+ */
+@RunWith(JUnit4.class)
+public final class MissingTestCallTest {
+  private final CompilationTestHelper helper =
+      CompilationTestHelper.newInstance(MissingTestCall.class, getClass());
+
+  @Test
+  public void positive() {
+    helper
+        .addSourceLines(
+            "Case.java",
+            "import com.google.common.testing.EqualsTester;",
+            "import com.google.errorprone.BugCheckerRefactoringTestHelper;",
+            "import com.google.errorprone.CompilationTestHelper;",
+            "import org.junit.Test;",
+            "class Case {",
+            "  @Test",
+            "  // BUG: Diagnostic contains:",
+            "  void test() {",
+            "    new EqualsTester().addEqualityGroup(this, this);",
+            "    hashCode();",
+            "  }",
+            "  @Test",
+            "  // BUG: Diagnostic contains:",
+            "  void test2(CompilationTestHelper helper) {",
+            "    helper.addSourceFile(\"Foo.java\");",
+            "  }",
+            "  @Test",
+            "  // BUG: Diagnostic contains:",
+            "  void test3(BugCheckerRefactoringTestHelper helper) {",
+            "    helper.addInput(\"Foo.java\");",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negative() {
+    helper
+        .addSourceLines(
+            "Case.java",
+            "import com.google.common.testing.EqualsTester;",
+            "import com.google.errorprone.CompilationTestHelper;",
+            "import org.junit.Test;",
+            "class Case {",
+            "  CompilationTestHelper helper;",
+            "  @Test",
+            "  void test() {",
+            "    new EqualsTester().addEqualityGroup(this, this).testEquals();",
+            "  }",
+            "  @Test",
+            "  void doesNotMatchIfNotAtEnd() {",
+            "    helper.addSourceFile(\"Foo.java\");",
+            "    hashCode();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeNotTest() {
+    helper
+        .addSourceLines(
+            "Case.java",
+            "import com.google.common.testing.EqualsTester;",
+            "class Case {",
+            "  private EqualsTester et;",
+            "  void add() {",
+            "    et.addEqualityGroup(this, this);",
+            "  }",
+            "}")
+        .doTest();
+  }
+}

docs/bugpattern/MissingTestCall.md

@@ -0,0 +1,13 @@
+Some test helpers such as `EqualsTester` require a terminating method call to be
+of any use.
+
+```java {.bad}
+  @Test
+  public void string() {
+    new EqualsTester()
+        .addEqualityGroup("hello", new String("hello"))
+        .addEqualityGroup("world", new String("world"))
+        .addEqualityGroup(2, new Integer(2));
+    // Oops: forgot to call `testEquals()`
+  }
+```