Restore original System Properties after tests

Many registry tools tests modify system properties but do not
restore them to original state. These tests must be isolated
from each other and cannot share the same test execution process.

This has a huge impact on test performance under Gradle, which
seems to have higher process startup overhead. Current Gradle
test config has to set 'forEvery' to 1, i.e., every test class
must be run in a freshly started process.

This change significantly reduces the number of tests that need
isolation, making it easier to optimize test config for the
remaining tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=221350284

java/google/registry/config/RegistryEnvironment.java

@@ -47,11 +47,25 @@ public enum RegistryEnvironment {
    */
   UNITTEST;
 
+  /** Sets this enum as the name of the registry environment. */
+  public RegistryEnvironment setup() {
+    return setup(SystemPropertySetter.PRODUCTION_IMPL);
+  }
+
+  /**
+   * Sets this enum as the name of the registry environment using specified {@link
+   * SystemPropertySetter}.
+   */
+  public RegistryEnvironment setup(SystemPropertySetter systemPropertySetter) {
+    systemPropertySetter.setProperty(PROPERTY, name());
+    return this;
+  }
+
   /** Returns environment configured by system property {@value #PROPERTY}. */
   public static RegistryEnvironment get() {
     return valueOf(Ascii.toUpperCase(System.getProperty(PROPERTY, UNITTEST.name())));
   }
 
   /** System property for configuring which environment we should use. */
-  public static final String PROPERTY = "google.registry.environment";
+  private static final String PROPERTY = "google.registry.environment";
 }

java/google/registry/config/SystemPropertySetter.java

@@ -0,0 +1,47 @@
+// Copyright 2018 The Nomulus Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package google.registry.config;
+
+import javax.annotation.Nullable;
+
+/**
+ * Wrapper interface around {@link System#setProperty(String, String)} and {@link
+ * System#clearProperty(String)}. Tests that modify system properties may provide custom
+ * implementations that keeps track of changes and restores original property values on test
+ * completion.
+ */
+public interface SystemPropertySetter {
+
+  /**
+   * Updates the system property specified by {@code key}. If {@code value} is not null, {@link
+   * System#setProperty(String, String)} is invoked; otherwise {@link System#clearProperty(String)}
+   * is invoked.
+   */
+  SystemPropertySetter setProperty(String key, @Nullable String value);
+
+  /** Production implementation of {@link SystemPropertySetter}. */
+  SystemPropertySetter PRODUCTION_IMPL =
+      new SystemPropertySetter() {
+        @Override
+        public SystemPropertySetter setProperty(String key, @Nullable String value) {
+          if (value == null) {
+            System.clearProperty(key);
+          } else {
+            System.setProperty(key, value);
+          }
+          return this;
+        }
+      };
+}

java/google/registry/tools/RegistryToolEnvironment.java

@@ -17,10 +17,12 @@ package google.registry.tools;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkState;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Ascii;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import google.registry.config.RegistryEnvironment;
+import google.registry.config.SystemPropertySetter;
 
 /** Enum of production environments, used for the {@code --environment} flag. */
 enum RegistryToolEnvironment {
@@ -75,12 +77,24 @@ enum RegistryToolEnvironment {
     return instance;
   }
 
-  /** Setup execution environment. Call this method before any classes are loaded. */
+  /** Resets static class state to uninitialized state. */
+  @VisibleForTesting
+  static void reset() {
+    instance = null;
+  }
+
+  /** Sets up execution environment. Call this method before any classes are loaded. */
   void setup() {
+    setup(SystemPropertySetter.PRODUCTION_IMPL);
+  }
+
+  /** Sets up execution environment. Call this method before any classes are loaded. */
+  @VisibleForTesting
+  void setup(SystemPropertySetter systemPropertySetter) {
     instance = this;
-    System.setProperty(RegistryEnvironment.PROPERTY, actualEnvironment.name());
+    actualEnvironment.setup(systemPropertySetter);
     for (ImmutableMap.Entry<String, String> entry : extraProperties.entrySet()) {
-      System.setProperty(entry.getKey(), entry.getValue());
+      systemPropertySetter.setProperty(entry.getKey(), entry.getValue());
     }
   }
 

javatests/google/registry/config/BUILD

@@ -12,6 +12,7 @@ java_library(
     srcs = glob(["*.java"]),
     deps = [
         "//java/google/registry/config",
+        "//javatests/google/registry/testing",
         "@com_google_auto_value",
         "@com_google_guava",
         "@com_google_truth",

javatests/google/registry/testing/SystemPropertyRule.java

@@ -17,6 +17,7 @@ package google.registry.testing;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
+import google.registry.config.SystemPropertySetter;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -25,10 +26,8 @@ import java.util.Optional;
 import javax.annotation.Nullable;
 import org.junit.rules.ExternalResource;
 
-/**
+/** JUnit Rule for overriding the values Java system properties during tests. */
- * JUnit Rule for overriding the values Java system properties during tests.
+public final class SystemPropertyRule extends ExternalResource implements SystemPropertySetter {
- */
-public final class SystemPropertyRule extends ExternalResource {
 
   /** Class representing a system property key value pair. */
   private static class Property {
@@ -56,14 +55,15 @@ public final class SystemPropertyRule extends ExternalResource {
   /**
    * Change the value of a system property which is restored to its original value after the test.
    *
-   * <p>It's safe to call this method multiple times with the same {@code key} within a single
+   * <p>It's safe to call this method multiple times with the same {@code key} within a single test.
-   * test. Only the truly original property value will be restored at the end.
+   * Only the truly original property value will be restored at the end.
    *
    * <p>This method can be called fluently when declaring the Rule field, or within a Test method.
    *
    * @see java.lang.System#setProperty(String, String)
    */
-  public SystemPropertyRule override(String key, @Nullable String value) {
+  @Override
+  public SystemPropertyRule setProperty(String key, @Nullable String value) {
     originals.computeIfAbsent(
         key, k -> new Property(k, Optional.ofNullable(System.getProperty(k))));
     Property property = new Property(key, Optional.ofNullable(value));

javatests/google/registry/tools/CommandTestCase.java

@@ -32,6 +32,7 @@ import google.registry.model.poll.PollMessage;
 import google.registry.testing.AppEngineRule;
 import google.registry.testing.CertificateSamples;
 import google.registry.testing.MockitoJUnitRule;
+import google.registry.testing.SystemPropertyRule;
 import google.registry.tools.params.ParameterFactory;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
@@ -61,6 +62,8 @@ public abstract class CommandTestCase<C extends Command> {
   public final AppEngineRule appEngine =
       AppEngineRule.builder().withDatastore().withTaskQueue().build();
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   @Rule public final MockitoJUnitRule mocks = MockitoJUnitRule.create();
 
   @Rule
@@ -69,14 +72,14 @@ public abstract class CommandTestCase<C extends Command> {
   @Before
   public final void beforeCommandTestCase() {
     // Ensure the UNITTEST environment has been set before constructing a new command instance.
-    RegistryToolEnvironment.UNITTEST.setup();
+    RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
     command = newCommandInstance();
     System.setOut(new PrintStream(stdout));
     System.setErr(new PrintStream(stderr));
   }
 
   void runCommandInEnvironment(RegistryToolEnvironment env, String... args) throws Exception {
-    env.setup();
+    env.setup(systemPropertyRule);
     try {
       JCommander jcommander = new JCommander(command);
       jcommander.addConverterFactory(new ParameterFactory());
@@ -87,7 +90,7 @@ public abstract class CommandTestCase<C extends Command> {
       // This primarily matters for AutoTimestamp fields, which otherwise won't have updated values.
       ofy().clearSessionCache();
       // Reset back to UNITTEST environment.
-      RegistryToolEnvironment.UNITTEST.setup();
+      RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
     }
   }
 

javatests/google/registry/tools/DefaultRequestFactoryModuleTest.java

@@ -21,7 +21,9 @@ import com.google.api.client.http.HttpRequest;
 import com.google.api.client.http.HttpRequestFactory;
 import com.google.api.client.http.HttpRequestInitializer;
 import google.registry.config.RegistryConfig;
+import google.registry.testing.SystemPropertyRule;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -40,11 +42,13 @@ public class DefaultRequestFactoryModuleTest {
         }
       });
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   DefaultRequestFactoryModule module = new DefaultRequestFactoryModule();
 
   @Before
   public void setUp() {
-    RegistryToolEnvironment.UNITTEST.setup();
+    RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
   }
 
   @Test

javatests/google/registry/tools/GtechToolTest.java

@@ -17,7 +17,9 @@ package google.registry.tools;
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.collect.Sets;
+import google.registry.testing.SystemPropertyRule;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -26,9 +28,11 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class GtechToolTest {
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   @Before
   public void init() {
-    RegistryToolEnvironment.UNITTEST.setup();
+    RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
   }
 
   @Test

javatests/google/registry/tools/RegistryToolEnvironmentTest.java

@@ -17,6 +17,8 @@ package google.registry.tools;
 import static com.google.common.truth.Truth.assertThat;
 import static google.registry.testing.JUnitBackports.assertThrows;
 
+import google.registry.testing.SystemPropertyRule;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -25,17 +27,20 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class RegistryToolEnvironmentTest {
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   @Test
   public void testGet_withoutSetup_throws() {
+    RegistryToolEnvironment.reset();
     assertThrows(IllegalStateException.class, RegistryToolEnvironment::get);
   }
 
   @Test
   public void testSetup_changesEnvironmentReturnedByGet() {
-    RegistryToolEnvironment.UNITTEST.setup();
+    RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
     assertThat(RegistryToolEnvironment.get()).isEqualTo(RegistryToolEnvironment.UNITTEST);
 
-    RegistryToolEnvironment.ALPHA.setup();
+    RegistryToolEnvironment.ALPHA.setup(systemPropertyRule);
     assertThat(RegistryToolEnvironment.get()).isEqualTo(RegistryToolEnvironment.ALPHA);
   }
 

javatests/google/registry/tools/RegistryToolTest.java

@@ -25,6 +25,7 @@ import com.google.common.collect.Sets;
 import com.google.common.reflect.ClassPath;
 import com.google.common.reflect.ClassPath.ClassInfo;
 import com.google.common.truth.Expect;
+import google.registry.testing.SystemPropertyRule;
 import java.io.IOException;
 import java.lang.reflect.Modifier;
 import java.util.Map;
@@ -41,9 +42,11 @@ public class RegistryToolTest {
   @Rule
   public final Expect expect = Expect.create();
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   @Before
   public void init() {
-    RegistryToolEnvironment.UNITTEST.setup();
+    RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
   }
 
   @Test

javatests/google/registry/tools/ShellCommandTest.java

@@ -27,6 +27,7 @@ import com.beust.jcommander.Parameters;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import google.registry.testing.FakeClock;
+import google.registry.testing.SystemPropertyRule;
 import google.registry.tools.ShellCommand.JCommanderCompletor;
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
@@ -40,6 +41,7 @@ import org.joda.time.DateTime;
 import org.joda.time.Duration;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -47,6 +49,8 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class ShellCommandTest {
 
+  @Rule public final SystemPropertyRule systemPropertyRule = new SystemPropertyRule();
+
   CommandRunner cli = mock(CommandRunner.class);
   FakeClock clock = new FakeClock(DateTime.parse("2000-01-01TZ"));
 
@@ -107,7 +111,7 @@ public class ShellCommandTest {
 
   @Test
   public void testNoIdleWhenInAlpha() throws Exception {
-    RegistryToolEnvironment.ALPHA.setup();
+    RegistryToolEnvironment.ALPHA.setup(systemPropertyRule);
     MockCli cli = new MockCli();
     ShellCommand shellCommand =
         createShellCommand(cli, Duration.standardDays(1), "test1 foo bar", "test2 foo bar");
@@ -116,7 +120,7 @@ public class ShellCommandTest {
 
   @Test
   public void testNoIdleWhenInSandbox() throws Exception {
-    RegistryToolEnvironment.SANDBOX.setup();
+    RegistryToolEnvironment.SANDBOX.setup(systemPropertyRule);
     MockCli cli = new MockCli();
     ShellCommand shellCommand =
         createShellCommand(cli, Duration.standardDays(1), "test1 foo bar", "test2 foo bar");
@@ -125,7 +129,7 @@ public class ShellCommandTest {
 
   @Test
   public void testIdleWhenOverHourInProduction() throws Exception {
-    RegistryToolEnvironment.PRODUCTION.setup();
+    RegistryToolEnvironment.PRODUCTION.setup(systemPropertyRule);
     MockCli cli = new MockCli();
     ShellCommand shellCommand =
         createShellCommand(cli, Duration.standardMinutes(61), "test1 foo bar", "test2 foo bar");
@@ -135,7 +139,7 @@ public class ShellCommandTest {
 
   @Test
   public void testNoIdleWhenUnderHourInProduction() throws Exception {
-    RegistryToolEnvironment.PRODUCTION.setup();
+    RegistryToolEnvironment.PRODUCTION.setup(systemPropertyRule);
     MockCli cli = new MockCli();
     ShellCommand shellCommand =
         createShellCommand(cli, Duration.standardMinutes(59), "test1 foo bar", "test2 foo bar");
@@ -155,7 +159,7 @@ public class ShellCommandTest {
   public void testMultipleCommandInvocations() throws Exception {
     try (RegistryCli cli =
         new RegistryCli("unittest", ImmutableMap.of("test_command", TestCommand.class))) {
-      RegistryToolEnvironment.UNITTEST.setup();
+      RegistryToolEnvironment.UNITTEST.setup(systemPropertyRule);
       cli.setEnvironment(RegistryToolEnvironment.UNITTEST);
       cli.run(new String[] {"test_command", "-x", "xval", "arg1", "arg2"});
       cli.run(new String[] {"test_command", "-x", "otherxval", "arg3"});
@@ -272,7 +276,7 @@ public class ShellCommandTest {
 
   @Test
   public void testEncapsulatedOutput_command() throws Exception {
-    RegistryToolEnvironment.ALPHA.setup();
+    RegistryToolEnvironment.ALPHA.setup(systemPropertyRule);
     captureOutput();
     ShellCommand shellCommand =
         new ShellCommand(
@@ -296,7 +300,7 @@ public class ShellCommandTest {
 
   @Test
   public void testEncapsulatedOutput_throws() throws Exception {
-    RegistryToolEnvironment.ALPHA.setup();
+    RegistryToolEnvironment.ALPHA.setup(systemPropertyRule);
     captureOutput();
     ShellCommand shellCommand =
         new ShellCommand(