From 97845fb72e6d1fcb79a9c7d03598d1f85a5ff11d Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Mon, 6 Jun 2022 07:47:52 -0700
Subject: [PATCH] testing: Have GrpcCleanupRule extend ExternalResource

This allows using GrpcCleanupRule with JUnit 5 when combined with
ExternalResourceSupport. We don't really lose anything important when
running with JUnit 4 and this eases migration to JUnit 5.

ExternalResource is now responsible for combining exceptions. after()
cannot throw checked exceptions, so we must now wrap the
InterruptedException. When used with JUnit 5 we are unable to detect the
test failed; we accept that for now but it may be fair to create a new
class for JUnit 5 to be used with `@RegisterExtension` that implements
BeforeEachCallback and AfterTestExecutionCallback to restore the JUnit 4
behavior.

See #5331
---
 .../java/io/grpc/testing/GrpcCleanupRule.java | 83 +++++++++----------
 .../io/grpc/testing/GrpcCleanupRuleTest.java  | 21 +++--
 2 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/testing/src/main/java/io/grpc/testing/GrpcCleanupRule.java b/testing/src/main/java/io/grpc/testing/GrpcCleanupRule.java
index 47a3416d4d3..fb111525704 100644
--- a/testing/src/main/java/io/grpc/testing/GrpcCleanupRule.java
+++ b/testing/src/main/java/io/grpc/testing/GrpcCleanupRule.java
@@ -26,20 +26,18 @@
 import io.grpc.ManagedChannel;
 import io.grpc.Server;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nonnull;
 import javax.annotation.concurrent.NotThreadSafe;
-import org.junit.rules.TestRule;
+import org.junit.rules.ExternalResource;
 import org.junit.runner.Description;
-import org.junit.runners.model.MultipleFailureException;
 import org.junit.runners.model.Statement;
 
 /**
- * A JUnit {@link TestRule} that can register gRPC resources and manages its automatic release at
- * the end of the test. If any of the resources registered to the rule can not be successfully
- * released, the test will fail.
+ * A JUnit {@link ExternalResource} that can register gRPC resources and manages its automatic
+ * release at the end of the test. If any of the resources registered to the rule can not be
+ * successfully released, the test will fail.
  *
  * <p>Example usage:
  * <pre>{@code @Rule public final GrpcCleanupRule grpcCleanup = new GrpcCleanupRule();
@@ -73,13 +71,13 @@
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/2488")
 @NotThreadSafe
-public final class GrpcCleanupRule implements TestRule {
+public final class GrpcCleanupRule extends ExternalResource {
 
   private final List<Resource> resources = new ArrayList<>();
   private long timeoutNanos = TimeUnit.SECONDS.toNanos(10L);
   private Stopwatch stopwatch = Stopwatch.createUnstarted();
 
-  private Throwable firstException;
+  private boolean abruptShutdown;
 
   /**
    * Sets a positive total time limit for the automatic resource cleanup. If any of the resources
@@ -144,71 +142,70 @@ void register(Resource resource) {
     resources.add(resource);
   }
 
+  // The class extends ExternalResource so it can be used in JUnit 5. But JUnit 5 will only call
+  // before() and after(), thus code cannot assume this method will be called.
   @Override
   public Statement apply(final Statement base, Description description) {
-    return new Statement() {
+    return super.apply(new Statement() {
       @Override
       public void evaluate() throws Throwable {
-        firstException = null;
+        abruptShutdown = false;
         try {
           base.evaluate();
         } catch (Throwable t) {
-          firstException = t;
-
-          try {
-            teardown();
-          } catch (Throwable t2) {
-            throw new MultipleFailureException(Arrays.asList(t, t2));
-          }
-
+          abruptShutdown = true;
           throw t;
         }
-
-        teardown();
-        if (firstException != null) {
-          throw firstException;
-        }
       }
-    };
+    }, description);
   }
 
   /**
    * Releases all the registered resources.
    */
-  private void teardown() {
+  @Override
+  protected void after() {
     stopwatch.reset();
     stopwatch.start();
 
-    if (firstException == null) {
+    InterruptedException interrupted = null;
+    if (!abruptShutdown) {
       for (int i = resources.size() - 1; i >= 0; i--) {
         resources.get(i).cleanUp();
       }
+
+      for (int i = resources.size() - 1; i >= 0; i--) {
+        try {
+          boolean released = resources.get(i).awaitReleased(
+              timeoutNanos - stopwatch.elapsed(TimeUnit.NANOSECONDS), TimeUnit.NANOSECONDS);
+          if (released) {
+            resources.remove(i);
+          }
+        } catch (InterruptedException e) {
+          Thread.currentThread().interrupt();
+          interrupted = e;
+          break;
+        }
+      }
     }
 
-    for (int i = resources.size() - 1; i >= 0; i--) {
-      if (firstException != null) {
+    if (!resources.isEmpty()) {
+      for (int i = resources.size() - 1; i >= 0; i--) {
         resources.get(i).forceCleanUp();
-        continue;
       }
 
       try {
-        boolean released = resources.get(i).awaitReleased(
-            timeoutNanos - stopwatch.elapsed(TimeUnit.NANOSECONDS), TimeUnit.NANOSECONDS);
-        if (!released) {
-          firstException = new AssertionError(
-              "Resource " + resources.get(i) + " can not be released in time at the end of test");
+        if (interrupted != null) {
+          throw new AssertionError(
+              "Thread interrupted before resources gracefully released", interrupted);
+        } else if (!abruptShutdown) {
+          throw new AssertionError(
+            "Resources could not be released in time at the end of test: " + resources);
         }
-      } catch (InterruptedException e) {
-        Thread.currentThread().interrupt();
-        firstException = e;
-      }
-
-      if (firstException != null) {
-        resources.get(i).forceCleanUp();
+      } finally {
+        resources.clear();
       }
     }
-
-    resources.clear();
   }
 
   @VisibleForTesting
diff --git a/testing/src/test/java/io/grpc/testing/GrpcCleanupRuleTest.java b/testing/src/test/java/io/grpc/testing/GrpcCleanupRuleTest.java
index 3be53ae8b8b..a5a6783d53f 100644
--- a/testing/src/test/java/io/grpc/testing/GrpcCleanupRuleTest.java
+++ b/testing/src/test/java/io/grpc/testing/GrpcCleanupRuleTest.java
@@ -248,13 +248,13 @@ public void multiResource_awaitReleasedFails() throws Throwable {
 
     inOrder.verify(resource3).awaitReleased(anyLong(), any(TimeUnit.class));
     inOrder.verify(resource2).awaitReleased(anyLong(), any(TimeUnit.class));
+    inOrder.verify(resource1).awaitReleased(anyLong(), any(TimeUnit.class));
     inOrder.verify(resource2).forceCleanUp();
-    inOrder.verify(resource1).forceCleanUp();
 
     inOrder.verifyNoMoreInteractions();
 
     verify(resource3, never()).forceCleanUp();
-    verify(resource1, never()).awaitReleased(anyLong(), any(TimeUnit.class));
+    verify(resource1, never()).forceCleanUp();
   }
 
   @Test
@@ -280,7 +280,7 @@ public void multiResource_awaitReleasedInterrupted() throws Throwable {
     boolean cleanupFailed = false;
     try {
       grpcCleanup.apply(statement, null /* description*/).evaluate();
-    } catch (InterruptedException e) {
+    } catch (Throwable e) {
       cleanupFailed = true;
     }
 
@@ -381,7 +381,8 @@ public void multiResource_timeoutCalculation_customTimeout() throws Throwable {
   @Test
   public void baseTestFailsThenCleanupFails() throws Throwable {
     // setup
-    Exception baseTestFailure = new Exception();
+    Exception baseTestFailure = new Exception("base test failure");
+    Exception cleanupFailure = new RuntimeException("force cleanup failed");
 
     Statement statement = mock(Statement.class);
     doThrow(baseTestFailure).when(statement).evaluate();
@@ -389,7 +390,7 @@ public void baseTestFailsThenCleanupFails() throws Throwable {
     Resource resource1 = mock(Resource.class);
     Resource resource2 = mock(Resource.class);
     Resource resource3 = mock(Resource.class);
-    doThrow(new RuntimeException()).when(resource2).forceCleanUp();
+    doThrow(cleanupFailure).when(resource2).forceCleanUp();
 
     InOrder inOrder = inOrder(statement, resource1, resource2, resource3);
     GrpcCleanupRule grpcCleanup = new GrpcCleanupRule();
@@ -407,8 +408,14 @@ public void baseTestFailsThenCleanupFails() throws Throwable {
     }
 
     // verify
-    assertThat(failure).isInstanceOf(MultipleFailureException.class);
-    assertSame(baseTestFailure, ((MultipleFailureException) failure).getFailures().get(0));
+    if (failure instanceof MultipleFailureException) {
+      // JUnit 4.13+
+      assertThat(((MultipleFailureException) failure).getFailures())
+          .containsExactly(baseTestFailure, cleanupFailure);
+    } else {
+      // JUnit 4.12. Suffers from https://github.com/junit-team/junit4/issues/1334
+      assertThat(failure).isSameInstanceAs(cleanupFailure);
+    }
 
     inOrder.verify(statement).evaluate();
     inOrder.verify(resource3).forceCleanUp();