JUnit4 now correctly labels filtered tests as "filtered".

--
PiperOrigin-RevId: 147362001
MOS_MIGRATED_REVID=147362001

4 files changed, 105 insertions, 15 deletions

diff --git a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/internal/junit4/JUnit4TestXmlListener.java b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/internal/junit4/JUnit4TestXmlListener.java
index 6d607256c9..052c0a757d 100644
--- a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/internal/junit4/JUnit4TestXmlListener.java
+++ b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/internal/junit4/JUnit4TestXmlListener.java
@@ -57,6 +57,13 @@ public class JUnit4TestXmlListener extends RunListener  {
   @Override
   public void testRunStarted(Description description) throws Exception {
     model = modelSupplier.get();
+
+    /*
+     * At this point, command line filtering has been applied. Mark all remaining tests as
+     * "pending"; any other tests will be considered "filtered".
+     */
+    model.testRunStarted(description);
+
     signalHandlers.installHandler(new Signal("TERM"), new WriteXmlSignalHandler());
   }
 
diff --git a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestCaseNode.java b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestCaseNode.java
index 7c2567434f..cdaaf953fe 100644
--- a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestCaseNode.java
+++ b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestCaseNode.java
@@ -22,6 +22,7 @@ import com.google.testing.junit.runner.util.TestIntegrationsExporter;
 import com.google.testing.junit.runner.util.TestPropertyExporter;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -36,6 +37,8 @@ import org.junit.runner.Description;
 /** A leaf in the test suite model. */
 class TestCaseNode extends TestNode
     implements TestPropertyExporter.Callback, TestIntegrationsExporter.Callback {
+  private static final Set<State> INITIAL_STATES = Collections.unmodifiableSet(
+      EnumSet.of(State.INITIAL, State.PENDING));
   private final TestSuiteNode parent;
   private final Map<String, String> properties = new ConcurrentHashMap<>();
   private final Map<String, Integer> repeatedPropertyNamesToRepetitions = new HashMap<>();
@@ -59,6 +62,12 @@ class TestCaseNode extends TestNode
     return Collections.emptyList();
   }
 
+  /**
+   * Indicates that the test represented by this node is scheduled to start.
+   */
+  void pending() {
+    compareAndSetState(State.INITIAL, State.PENDING, -1);
+  }
 
   /**
    * Indicates that the test represented by this node has started.
@@ -66,7 +75,7 @@ class TestCaseNode extends TestNode
    * @param now Time that the test started
    */
   public void started(long now) {
-    compareAndSetState(State.INITIAL, State.STARTED, now);
+    compareAndSetState(INITIAL_STATES, State.STARTED, now);
   }
 
   @Override
@@ -74,7 +83,7 @@ class TestCaseNode extends TestNode
     if (compareAndSetState(State.STARTED, State.INTERRUPTED, now)) {
       return;
     }
-    compareAndSetState(State.INITIAL, State.CANCELLED, now);
+    compareAndSetState(INITIAL_STATES, State.CANCELLED, now);
   }
 
   @Override
@@ -102,7 +111,7 @@ class TestCaseNode extends TestNode
 
   @Override
   public void testSuppressed(long now) {
-    compareAndSetState(State.INITIAL, State.SUPPRESSED, now);
+    compareAndSetState(INITIAL_STATES, State.SUPPRESSED, now);
   }
 
   /**
@@ -116,13 +125,13 @@ class TestCaseNode extends TestNode
 
   @Override
   public void testFailure(Throwable throwable, long now) {
-    compareAndSetState(State.INITIAL, State.FINISHED, now);
+    compareAndSetState(INITIAL_STATES, State.FINISHED, now);
     globalFailures.add(throwable);
   }
 
   @Override
   public void dynamicTestFailure(Description test, Throwable throwable, long now) {
-    compareAndSetState(State.INITIAL, State.FINISHED, now);
+    compareAndSetState(INITIAL_STATES, State.FINISHED, now);
     addThrowableToDynamicTestToFailures(test, throwable);
   }
 
@@ -156,15 +165,30 @@ class TestCaseNode extends TestNode
     return previousRepetitionsNr;
   }
 
-  private synchronized boolean compareAndSetState(State fromState, State toState, long now) {
-    if (fromState == null || toState == null || state == null) {
+  private boolean compareAndSetState(State fromState, State toState, long now) {
+    if (fromState == null) {
+      throw new NullPointerException();
+    }
+    return compareAndSetState(Collections.singleton(fromState), toState, now);
+  }
+
+  // TODO(bazel-team): Use AtomicReference instead of a synchronized method.
+  private synchronized boolean compareAndSetState(Set<State> fromStates, State toState, long now) {
+    if (fromStates == null || toState == null || state == null) {
       throw new NullPointerException();
     }
+    if (fromStates.isEmpty()) {
+      throw new IllegalArgumentException();
+    }
 
-    if (fromState == state && toState != state) {
+    if (fromStates.contains(state) && toState != state) {
       state = toState;
-      runTimeInterval =
-          runTimeInterval == null ? new TestInterval(now, now) : runTimeInterval.withEndMillis(now);
+      if (toState != State.PENDING) {
+        runTimeInterval =
+            runTimeInterval == null
+            ? new TestInterval(now, now)
+            : runTimeInterval.withEndMillis(now);
+      }
       return true;
     }
     return false;
@@ -247,7 +271,8 @@ class TestCaseNode extends TestNode
    * States of a TestCaseNode (see (link) for all the transitions and states descriptions).
    */
   private static enum State {
-    INITIAL(TestResult.Status.SKIPPED),
+    INITIAL(TestResult.Status.FILTERED),
+    PENDING(TestResult.Status.CANCELLED),
     STARTED(TestResult.Status.INTERRUPTED),
     SKIPPED(TestResult.Status.SKIPPED),
     SUPPRESSED(TestResult.Status.SUPPRESSED),
diff --git a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestSuiteModel.java b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestSuiteModel.java
index 92193460ad..f9fb7a0934 100644
--- a/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestSuiteModel.java
+++ b/src/java_tools/junitrunner/java/com/google/testing/junit/runner/model/TestSuiteModel.java
@@ -67,6 +67,11 @@ public class TestSuiteModel {
     return rootNode.getChildren();
   }
 
+  // VisibleForTesting
+  Description getTopLevelDescription() {
+    return rootNode.getDescription();
+  }
+
   /**
    * Gets the sharding filter to use; {@link Filter#ALL} if not sharding.
    */
@@ -83,12 +88,12 @@ public class TestSuiteModel {
    * listener!
    */
   private TestCaseNode getTestCase(Description description) {
-    // TODO(cpovirk): Is it legitimate to pass null here?
+    // The description shouldn't be null, but in the test runner code we avoid throwing exceptions.
     return description == null ? null : testCaseMap.get(description);
   }
 
   private TestNode getTest(Description description) {
-    // TODO(cpovirk): Is it legitimate to pass null here?
+    // The description shouldn't be null, but in the test runner code we avoid throwing exceptions.
     return description == null ? null : testsMap.get(description);
   }
 
@@ -98,6 +103,38 @@ public class TestSuiteModel {
   }
 
   /**
+   * Indicate that the test run has started. This should be called after all
+   * filtering has been completed.
+   *
+   * @param topLevelDescription the root {@link Description} node.
+   */
+  public void testRunStarted(Description topLevelDescription) {
+    markChildrenAsPending(topLevelDescription);
+  }
+
+  private void markChildrenAsPending(Description node) {
+    if (node.isTest()) {
+      testPending(node);
+    } else {
+      for (Description child : node.getChildren()) {
+        markChildrenAsPending(child);
+      }
+    }
+  }
+
+  /**
+   * Indicate that the test case with the given key is scheduled to start.
+   *
+   * @param description key for a test case
+   */
+  private void testPending(Description description) {
+    TestCaseNode testCase = getTestCase(description);
+    if (testCase != null) {
+      testCase.pending();
+    }
+  }
+
+  /**
    * Indicate that the test case with the given key has started.
    *
    * @param description key for a test case
@@ -266,6 +303,10 @@ public class TestSuiteModel {
       this.xmlResultWriter = xmlResultWriter;
     }
 
+    /**
+     * Build a model with the given name, including the given suites. This method
+     * should be called before any command line filters are applied.
+     */
     public TestSuiteModel build(String suiteName, Description... topLevelSuites) {
       if (buildWasCalled) {
         throw new IllegalStateException("Builder.build() was already called");
@@ -277,6 +318,7 @@ public class TestSuiteModel {
       rootNode = new TestSuiteNode(Description.createSuiteDescription(suiteName));
       for (Description topLevelSuite : topLevelSuites) {
         addTestSuite(rootNode, topLevelSuite);
+        rootNode.getDescription().addChild(topLevelSuite);
       }
       return new TestSuiteModel(this);
     }
diff --git a/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/model/TestCaseNodeTest.java b/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/model/TestCaseNodeTest.java
index ed313c1da8..07a768cbff 100644
--- a/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/model/TestCaseNodeTest.java
+++ b/src/java_tools/junitrunner/javatests/com/google/testing/junit/runner/model/TestCaseNodeTest.java
@@ -48,14 +48,22 @@ public class TestCaseNodeTest {
   }
 
   @Test
-  public void assertIsSkippedIfNotStarted() {
+  public void assertIsFilteredIfNeverPending() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
-    assertStatusWithoutTiming(testCaseNode, TestResult.Status.SKIPPED);
+    assertStatusWithoutTiming(testCaseNode, TestResult.Status.FILTERED);
+  }
+
+  @Test
+  public void assertIsCancelledIfNotStarted() {
+    TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
+    assertStatusWithoutTiming(testCaseNode, TestResult.Status.CANCELLED);
   }
 
   @Test
   public void assertIsCancelledIfInterruptedBeforeStart() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.testInterrupted(NOW);
     assertStatusAndTiming(testCaseNode, TestResult.Status.CANCELLED, NOW, 0);
   }
@@ -63,6 +71,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertIsCompletedIfFailedBeforeStart() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.testFailure(new Exception(), NOW);
     assertStatusAndTiming(testCaseNode, TestResult.Status.COMPLETED, NOW, 0);
   }
@@ -70,6 +79,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertInterruptedIfStartedAndNotFinished() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     assertStatusAndTiming(testCaseNode, TestResult.Status.INTERRUPTED, NOW, 0);
     // Notice: This is an unexpected ending state, as even interrupted test executions should go
@@ -79,6 +89,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertInterruptedIfStartedAndInterrupted() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     testCaseNode.testInterrupted(NOW + 1);
     assertStatusAndTiming(testCaseNode, TestResult.Status.INTERRUPTED, NOW, 1);
@@ -87,6 +98,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertSkippedIfStartedAndSkipped() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     testCaseNode.testSkipped(NOW + 1);
     assertStatusAndTiming(testCaseNode, TestResult.Status.SKIPPED, NOW, 1);
@@ -95,6 +107,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertCompletedIfStartedAndFinished() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     testCaseNode.finished(NOW + 1);
     assertStatusAndTiming(testCaseNode, TestResult.Status.COMPLETED, NOW, 1);
@@ -103,6 +116,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertCompletedIfStartedAndFailedAndFinished() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     testCaseNode.testFailure(new Exception(), NOW + 1);
     testCaseNode.finished(NOW + 2);
@@ -112,6 +126,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertInterruptedIfStartedAndFailedAndInterrupted() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.started(NOW);
     testCaseNode.testFailure(new Exception(), NOW + 1);
     testCaseNode.testInterrupted(NOW + 2);
@@ -121,6 +136,7 @@ public class TestCaseNodeTest {
   @Test
   public void assertTestSuppressedIfNotStartedAndSuppressed() {
     TestCaseNode testCaseNode = new TestCaseNode(testCase, new TestSuiteNode(suite));
+    testCaseNode.pending();
     testCaseNode.testSuppressed(NOW);
     assertStatusAndTiming(testCaseNode, TestResult.Status.SUPPRESSED, NOW, 0);
   }