From 904f16c8b55eb287c0bcbc4c004b08c0c246b574 Mon Sep 17 00:00:00 2001
From: Weimin Yu <weiminyu@google.com>
Date: Mon, 6 Apr 2020 13:26:38 -0400
Subject: [PATCH] Actually run JUnit 5 tests (#545)

* Actually run JUnit 5 tests

In Gradle, JUnit 5 must be explicitly enabled with a call to
test.useJUnitPlatform().

The FilteringTest used in :core must also enable JUnit5 separately.

Fixed AppEngineRule to work with the few tests that have migrated
to JUnit5.

More work is needed with AppEngine before we can migrate tests
that actually use Cloud SQL.

For context, with @EnableRuleMigrationSupport, JUnit 5 runner calls
an external resource's before() and after() methods. TestRule.apply()
is not called, therefore any setup done their will be bypassed with
JUnit 5.
---
 core/build.gradle                             |  4 ++++
 .../registry/testing/AppEngineRule.java       | 21 ++++++++++++++-----
 java_common.gradle                            |  4 ++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/core/build.gradle b/core/build.gradle
index c8f5ad631..e2eafbecf 100644
--- a/core/build.gradle
+++ b/core/build.gradle
@@ -646,6 +646,10 @@ artifacts {
  */
 class FilteringTest extends Test {
 
+  FilteringTest() {
+    useJUnitPlatform();
+  }
+
   private void applyTestFilter() {
     if (project.testFilter) {
       testNameIncludePatterns = project.testFilter.split(',')
diff --git a/core/src/test/java/google/registry/testing/AppEngineRule.java b/core/src/test/java/google/registry/testing/AppEngineRule.java
index 52af8442f..a5fc89d42 100644
--- a/core/src/test/java/google/registry/testing/AppEngineRule.java
+++ b/core/src/test/java/google/registry/testing/AppEngineRule.java
@@ -40,6 +40,7 @@ import google.registry.model.registrar.Registrar.State;
 import google.registry.model.registrar.RegistrarAddress;
 import google.registry.model.registrar.RegistrarContact;
 import google.registry.persistence.transaction.JpaTestRules;
+import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageRule;
 import google.registry.util.Clock;
 import java.io.ByteArrayInputStream;
 import java.io.File;
@@ -256,7 +257,14 @@ public final class AppEngineRule extends ExternalResource {
         .build();
   }
 
-  /** Hack to make sure AppEngineRule is always wrapped in a TemporaryFolder rule. */
+  /**
+   * Hack to make sure AppEngineRule is always wrapped in a {@link JpaIntegrationWithCoverageRule}.
+   */
+  // Note: Even with @EnableRuleMigrationSupport, JUnit5 runner does not call this method.
+  // Note 2: Do not migrate any members of SqlIntegrationTestSuite to JUnit5 before
+  // calls to JpaIntegrationWithCoverageRule can be made elsewhere.
+  // TODO(weiminyu): make JpaIntegrationWithCoverageRule implement ExternaResource and invoke it in
+  // before() and after(), then drop this method.
   @Override
   public Statement apply(Statement base, Description description) {
     Statement statement = base;
@@ -267,12 +275,13 @@ public final class AppEngineRule extends ExternalResource {
       }
       statement = builder.buildIntegrationWithCoverageRule().apply(base, description);
     }
-    return temporaryFolder.apply(super.apply(statement, description), description);
+    return super.apply(statement, description);
   }
 
   @Override
   protected void before() throws IOException {
     setupLogging();
+    temporaryFolder.create();
     Set<LocalServiceTestConfig> configs = new HashSet<>();
     if (withUrlFetch) {
       configs.add(new LocalURLFetchServiceTestConfig());
@@ -346,10 +355,10 @@ public final class AppEngineRule extends ExternalResource {
     helper = null;
     // Test that Datastore didn't need any indexes we don't have listed in our index file.
     File indexFile = new File(temporaryFolder.getRoot(), "datastore-indexes-auto.xml");
-    if (!indexFile.exists()) {
-      return;
-    }
     try {
+      if (!indexFile.exists()) {
+        return;
+      }
       String indexFileContent = Files.asCharSource(indexFile, UTF_8).read();
       if (indexFileContent.trim().isEmpty()) {
         return;
@@ -361,6 +370,8 @@ public final class AppEngineRule extends ExternalResource {
       }
     } catch (IOException e) {
       throw new RuntimeException(e);
+    } finally {
+      temporaryFolder.delete();
     }
   }
 
diff --git a/java_common.gradle b/java_common.gradle
index 4d6fdd749..94105056d 100644
--- a/java_common.gradle
+++ b/java_common.gradle
@@ -72,6 +72,10 @@ dependencies {
     errorprone("com.google.errorprone:error_prone_core:2.3.3")
 }
 
+test {
+  useJUnitPlatform()
+}
+
 tasks.withType(JavaCompile).configureEach {
     // The -Werror flag causes Intellij to fail on deprecated api use.
     // Allow IDE user to turn off this flag by specifying a Gradle VM