From dee559baee034e9ca383d81f47b2cc426e3e4f25 Mon Sep 17 00:00:00 2001 From: weiminyu Date: Thu, 15 Nov 2018 11:18:48 -0800 Subject: [PATCH] Define multiple test suites under Gradle for better test performance Tests are divided into three test suites without intra-suite conflicts. This allows us to unset forkEvery=1 (4x improvement on my desktop) and increase execution parallelism (additional 2x improvement on my desktop). ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=221656972 --- gradle/README.md | 24 +++++---- gradle/core/build.gradle | 105 +++++++++++++++++++++++++++++++-------- 2 files changed, 98 insertions(+), 31 deletions(-) diff --git a/gradle/README.md b/gradle/README.md index 62c7a557c..57801a7cf 100644 --- a/gradle/README.md +++ b/gradle/README.md @@ -15,8 +15,6 @@ the existing Nomulus source tree. Dependencies are mostly the same as in Bazel, with a few exceptions: -* org.slf4j:slf4j-simple is added to provide a logging implementation in - tests. Bazel does not need this. * com.googlecode.java-diff-utils:diffutils is not included. Bazel needs it for Truth's equality check, but Gradle works fine without it. * jaxb 2.2.11 is used instead of 2.3 in Bazel, since the latter breaks the @@ -27,18 +25,22 @@ Dependencies are mostly the same as in Bazel, with a few exceptions: ### Notable Issues -Only single-threaded test execution is allowed, due to race condition over -global resources, such as the local Datastore instance, or updates to the System -properties. This is a new problem with Gradle, which does not provide as much -test isolation as Bazel. We are exploring solutions to this problem. - Test suites (RdeTestSuite and TmchTestSuite) are ignored to avoid duplicate execution of tests. Neither suite performs any shared test setup routine, so it -is easier to exclude the suite classes than individual test classes. +is easier to exclude the suite classes than individual test classes. This is the +reason why all test tasks in the :core project contain the exclude pattern +'"**/*TestCase.*", "**/*TestSuite.*"' -Since Gradle does not support hierarchical build files, all file sets (e.g., -resources) must be declared at the top, in root project config or the -sub-project configs. +Many Nomulus tests are not hermetic: they modify global state (e.g., the shared +local instance of Datastore) but do not clean up on completion. This becomes a +problem with Gradle. In the beginning we forced Gradle to run every test class +in a new process, and incurred heavy overheads. Since then, we have fixed some +tests, and manged to divide all tests into three suites that do not have +intra-suite conflicts. We will revisit the remaining tests soon. + +Note that it is unclear if all conflicting tests have been identified. More may +be exposed if test execution order changes, e.g., when new tests are added or +execution parallelism level changes. ## Initial Setup diff --git a/gradle/core/build.gradle b/gradle/core/build.gradle index 35d04c23e..4cd14ac98 100644 --- a/gradle/core/build.gradle +++ b/gradle/core/build.gradle @@ -15,6 +15,46 @@ def aptGeneratedTestDir = "${project.buildDir}/generated/source/apt/test" // used for easy inspection. def generatedDir = "${project.buildDir}/generated/source/custom/main" +// Tests that conflict with (mostly unidentified) members of the main test +// suite. It is unclear if they are offenders (i.e., those that pollute global +// state) or victims. +// TODO(weiminyu): identify cause and fix offending tests. +def outcastTestPatterns = [ + "google/registry/batch/DeleteContactsAndHostsActionTest.*", + "google/registry/batch/RefreshDnsOnHostRenameActionTest.*", + "google/registry/flows/CheckApiActionTest.*", + "google/registry/flows/EppLifecycleHostTest.*", + "google/registry/flows/domain/DomainAllocateFlowTest.*", + "google/registry/flows/domain/DomainApplicationCreateFlowTest.*", + "google/registry/flows/domain/DomainApplicationUpdateFlowTest.*", + "google/registry/flows/domain/DomainCreateFlowTest.*", + "google/registry/flows/domain/DomainUpdateFlowTest.*", + "google/registry/tools/server/CreatePremiumListActionTest.*", + // Conflicts with WhoisActionTest + "google/registry/whois/WhoisHttpActionTest.*", +] + +// Tests that conflict with members of both the main test suite and the +// outcast suite. +// TODO(weiminyu): identify cause and fix offending tests. +def fragileTestPatterns = [ + "google/registry/cron/TldFanoutActionTest.*" +] + +// Test source directories shared by all test suites. +def testSourceDirs = [ + "${javatestsDir}", + "${aptGeneratedTestDir}" +] + +// Test resource directories shared by all test suites. +def testResourceDirs = [ + "${javatestsDir}" +] + +// Exclusion pattern for test source directories shared by all test suites. +def testResourceExclude = ['**/*.java', '**/*.xsd', '**/*.xjb'] + sourceSets { main { java { @@ -33,16 +73,29 @@ sourceSets { } test { java { - srcDirs = [ - "${javatestsDir}", - "${aptGeneratedTestDir}" - ] + srcDirs testSourceDirs } resources { - srcDirs = [ - "${javatestsDir}" - ] - exclude '**/*.java', '**/*.xsd', '**/*.xjb' + srcDirs testResourceDirs + exclude testResourceExclude + } + } + fragileTest { + java { + srcDirs testSourceDirs + } + resources { + srcDirs testResourceDirs + exclude testResourceExclude + } + } + outcastTest { + java { + srcDirs testSourceDirs + } + resources { + srcDirs testResourceDirs + exclude testResourceExclude } } } @@ -398,22 +451,34 @@ compileJava.dependsOn soyToJava stylesheetsToJavascript.dependsOn processResources classes.dependsOn stylesheetsToJavascript - -test { - // Test exclusion patterns: - // - *TestCase.java are inherited by concrete test classes. - // - *TestSuite.java are excluded to avoid duplicate execution of suite - // members. See README in this directory for more information. +task fragileTest(type: Test) { + // Common exclude pattern. See README in parent directory for explanation. exclude "**/*TestCase.*", "**/*TestSuite.*" + include fragileTestPatterns - // Use a single JVM to execute all tests. See README in this directory for - // more information. - maxParallelForks 1 - - // Use a single thread to execute all tests in a JVM. See README in this - // directory for more information. + // Run every test class in a freshly started process. forkEvery 1 // Uncomment to see test outputs in stdout. //testLogging.showStandardStreams = true } + +task outcastTest(type: Test) { + // Common exclude pattern. See README in parent directory for explanation. + exclude "**/*TestCase.*", "**/*TestSuite.*" + include outcastTestPatterns + + // Sets the maximum number of test executors that may exist at the same time. + maxParallelForks 5 +} + +test { + // Common exclude pattern. See README in parent directory for explanation. + exclude "**/*TestCase.*", "**/*TestSuite.*" + exclude fragileTestPatterns + exclude outcastTestPatterns + + // Sets the maximum number of test executors that may exist at the same time. + maxParallelForks 5 +}.dependsOn(fragileTest, outcastTest) +