From a23e5d064be2a4d62003a0a3cbe863985389da59 Mon Sep 17 00:00:00 2001 From: Weimin Yu Date: Tue, 14 Apr 2020 12:48:21 -0400 Subject: [PATCH] Disable JpaEntityCoverageCheck by default (#555) * Disable JpaEntityCoverageCheck by default Only members of SqlIntegrationTestSuite should enable the check, which incurs per-test overhead. --- .../google/registry/model/EntityTestCase.java | 17 +++++-- .../model/domain/DomainBaseSqlTest.java | 4 ++ .../model/registry/RegistryLockDaoTest.java | 6 ++- .../registry/schema/cursor/CursorDaoTest.java | 6 ++- .../schema/registrar/RegistrarDaoTest.java | 4 ++ .../schema/tld/PremiumListDaoTest.java | 6 ++- .../registry/testing/AppEngineRule.java | 45 ++++++++++++++++--- 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/core/src/test/java/google/registry/model/EntityTestCase.java b/core/src/test/java/google/registry/model/EntityTestCase.java index 4d88050b3..8e4227bbf 100644 --- a/core/src/test/java/google/registry/model/EntityTestCase.java +++ b/core/src/test/java/google/registry/model/EntityTestCase.java @@ -50,12 +50,23 @@ public abstract class EntityTestCase { protected FakeClock fakeClock = new FakeClock(DateTime.now(UTC)); - @Rule @RegisterExtension - public final AppEngineRule appEngine = - AppEngineRule.builder().withDatastoreAndCloudSql().withClock(fakeClock).build(); + @Rule @RegisterExtension public final AppEngineRule appEngine; @Rule @RegisterExtension public InjectRule inject = new InjectRule(); + protected EntityTestCase() { + this(false); + } + + protected EntityTestCase(boolean enableJpaEntityCheck) { + appEngine = + AppEngineRule.builder() + .withDatastoreAndCloudSql() + .enableJpaEntityCoverageCheck(enableJpaEntityCheck) + .withClock(fakeClock) + .build(); + } + @Before @BeforeEach public void injectClock() { diff --git a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java index e7c7bc7d2..04500eda9 100644 --- a/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java +++ b/core/src/test/java/google/registry/model/domain/DomainBaseSqlTest.java @@ -38,6 +38,10 @@ public class DomainBaseSqlTest extends EntityTestCase { Key contactKey; Key contact2Key; + public DomainBaseSqlTest() { + super(true); + } + @BeforeEach public void setUp() { contactKey = Key.create(ContactResource.class, "contact_id1"); diff --git a/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java b/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java index 3b9d0ceb6..0fccfae81 100644 --- a/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java +++ b/core/src/test/java/google/registry/model/registry/RegistryLockDaoTest.java @@ -41,7 +41,11 @@ public final class RegistryLockDaoTest { @RegisterExtension public final AppEngineRule appEngine = - AppEngineRule.builder().withDatastoreAndCloudSql().withClock(fakeClock).build(); + AppEngineRule.builder() + .withDatastoreAndCloudSql() + .enableJpaEntityCoverageCheck(true) + .withClock(fakeClock) + .build(); @Test public void testSaveAndLoad_success() { diff --git a/core/src/test/java/google/registry/schema/cursor/CursorDaoTest.java b/core/src/test/java/google/registry/schema/cursor/CursorDaoTest.java index 06c3e4141..bb2dbe68a 100644 --- a/core/src/test/java/google/registry/schema/cursor/CursorDaoTest.java +++ b/core/src/test/java/google/registry/schema/cursor/CursorDaoTest.java @@ -43,7 +43,11 @@ public class CursorDaoTest { @RegisterExtension public final AppEngineRule appEngine = - AppEngineRule.builder().withDatastoreAndCloudSql().withClock(fakeClock).build(); + AppEngineRule.builder() + .withDatastoreAndCloudSql() + .enableJpaEntityCoverageCheck(true) + .withClock(fakeClock) + .build(); @Test public void save_worksSuccessfullyOnNewCursor() { diff --git a/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java b/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java index 82222ed29..724ae5778 100644 --- a/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java +++ b/core/src/test/java/google/registry/schema/registrar/RegistrarDaoTest.java @@ -33,6 +33,10 @@ public class RegistrarDaoTest extends EntityTestCase { private Registrar testRegistrar; + public RegistrarDaoTest() { + super(true); + } + @BeforeEach public void setUp() { testRegistrar = diff --git a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java index b0ad16a33..085003372 100644 --- a/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java +++ b/core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java @@ -45,7 +45,11 @@ public class PremiumListDaoTest { @RegisterExtension public final AppEngineRule appEngine = - AppEngineRule.builder().withDatastoreAndCloudSql().withClock(fakeClock).build(); + AppEngineRule.builder() + .withDatastoreAndCloudSql() + .enableJpaEntityCoverageCheck(true) + .withClock(fakeClock) + .build(); private static final ImmutableMap TEST_PRICES = ImmutableMap.of( diff --git a/core/src/test/java/google/registry/testing/AppEngineRule.java b/core/src/test/java/google/registry/testing/AppEngineRule.java index 622911bc8..8c5f33713 100644 --- a/core/src/test/java/google/registry/testing/AppEngineRule.java +++ b/core/src/test/java/google/registry/testing/AppEngineRule.java @@ -41,6 +41,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.JpaIntegrationTestRule; import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageExtension; import google.registry.persistence.transaction.JpaTestRules.JpaIntegrationWithCoverageRule; import google.registry.util.Clock; @@ -104,10 +105,19 @@ public final class AppEngineRule extends ExternalResource /** A rule-within-a-rule to provide a temporary folder for AppEngineRule's internal temp files. */ TemporaryFolder temporaryFolder = new TemporaryFolder(); - // Sets up a SQL database when running on JUnit 5. + /** + * Sets up a SQL database when running on JUnit 5. This is for test classes that are not member of + * the {@code SqlIntegrationTestSuite}. + */ + JpaIntegrationTestRule jpaIntegrationTestRule = null; + /** + * Sets up a SQL database when running on JUnit 5 and records the JPA entities tested by each test + * class. This is for {@code SqlIntegrationTestSuite} members. + */ JpaIntegrationWithCoverageExtension jpaIntegrationWithCoverageExtension = null; private boolean withDatastoreAndCloudSql; + private boolean enableJpaEntityCoverageCheck; private boolean withLocalModules; private boolean withTaskQueue; private boolean withUserService; @@ -127,6 +137,14 @@ public final class AppEngineRule extends ExternalResource rule.withDatastoreAndCloudSql = true; return this; } + /** + * Enables JPA entity coverage check if {@code enabled} is true. This should only be enabled for + * members of SqlIntegrationTestSuite. + */ + public Builder enableJpaEntityCoverageCheck(boolean enabled) { + rule.enableJpaEntityCoverageCheck = enabled; + return this; + } /** Turn on the use of local modules. */ public Builder withLocalModules() { @@ -164,6 +182,9 @@ public final class AppEngineRule extends ExternalResource } public AppEngineRule build() { + checkState( + !rule.enableJpaEntityCoverageCheck || rule.withDatastoreAndCloudSql, + "withJpaEntityCoverageCheck enabled without Cloud SQL"); return rule; } } @@ -279,8 +300,13 @@ public final class AppEngineRule extends ExternalResource if (clock != null) { builder.withClock(clock); } - jpaIntegrationWithCoverageExtension = builder.buildIntegrationWithCoverageExtension(); - jpaIntegrationWithCoverageExtension.beforeEach(context); + if (enableJpaEntityCoverageCheck) { + jpaIntegrationWithCoverageExtension = builder.buildIntegrationWithCoverageExtension(); + jpaIntegrationWithCoverageExtension.beforeEach(context); + } else { + jpaIntegrationTestRule = builder.buildIntegrationTestRule(); + jpaIntegrationTestRule.before(); + } } } @@ -288,9 +314,11 @@ public final class AppEngineRule extends ExternalResource @Override public void afterEach(ExtensionContext context) throws Exception { if (withDatastoreAndCloudSql) { - checkState( - jpaIntegrationWithCoverageExtension != null, "Null jpaIntegrationWithCoverageExtension"); - jpaIntegrationWithCoverageExtension.afterEach(context); + if (enableJpaEntityCoverageCheck) { + jpaIntegrationWithCoverageExtension.afterEach(context); + } else { + jpaIntegrationTestRule.after(); + } } after(); } @@ -310,7 +338,10 @@ public final class AppEngineRule extends ExternalResource if (clock != null) { builder.withClock(clock); } - statement = builder.buildIntegrationWithCoverageRule().apply(base, description); + checkState( + !enableJpaEntityCoverageCheck, + "JUnit4 tests must not enable withJpaEntityCoverageCheck."); + statement = builder.buildIntegrationTestRule().apply(base, description); } return super.apply(statement, description); }