From 85911e0b95e9c498e861617827cdc8604d360f63 Mon Sep 17 00:00:00 2001
From: mcilwain <mcilwain@google.com>
Date: Tue, 22 Mar 2016 11:12:06 -0700
Subject: [PATCH] Rely less on local variables in UpdateRegistrarCommandTest

Reloading the entities under test every time is more resilient to
coding errors than trying to ensure that a local variable is kept up
to date.  It also reduces total LoC in the test file.
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=117842135
---
 .../tools/UpdateRegistrarCommandTest.java     | 229 +++++++-----------
 1 file changed, 94 insertions(+), 135 deletions(-)

diff --git a/javatests/com/google/domain/registry/tools/UpdateRegistrarCommandTest.java b/javatests/com/google/domain/registry/tools/UpdateRegistrarCommandTest.java
index 20aeb8166..319bc30d8 100644
--- a/javatests/com/google/domain/registry/tools/UpdateRegistrarCommandTest.java
+++ b/javatests/com/google/domain/registry/tools/UpdateRegistrarCommandTest.java
@@ -44,94 +44,88 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
 
   @Test
   public void testSuccess_password() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.testPassword("some_password")).isFalse();
-
+    assertThat(loadByClientId("NewRegistrar").testPassword("some_password")).isFalse();
     runCommand("--password=some_password", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.testPassword("some_password")).isTrue();
+    assertThat(loadByClientId("NewRegistrar").testPassword("some_password")).isTrue();
   }
 
   @Test
   public void testSuccess_registrarType() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    registrar = persistResource(
-        registrar.asBuilder().setType(Registrar.Type.OTE).setIanaIdentifier(null).build());
-    assertThat(registrar.getType()).isEqualTo(Type.OTE);
-
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setType(Registrar.Type.OTE)
+            .setIanaIdentifier(null)
+            .build());
+    assertThat(loadByClientId("NewRegistrar").getType()).isEqualTo(Type.OTE);
     runCommand("--registrar_type=TEST", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getType()).isEqualTo(Type.TEST);
+    assertThat(loadByClientId("NewRegistrar").getType()).isEqualTo(Type.TEST);
   }
 
   @Test
   public void testFailure_noPasscodeOnChangeToReal() throws Exception {
     thrown.expect(IllegalArgumentException.class, "--passcode is required for REAL registrars.");
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    registrar = persistResource(registrar.asBuilder()
-        .setType(Registrar.Type.OTE)
-        .setIanaIdentifier(null)
-        .setPhonePasscode(null)
-        .build());
-
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setType(Registrar.Type.OTE)
+            .setIanaIdentifier(null)
+            .setPhonePasscode(null)
+            .build());
     runCommand("--registrar_type=REAL", "--iana_id=1000", "--force", "NewRegistrar");
   }
 
   @Test
   public void testSuccess_registrarState() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getState()).isEqualTo(State.ACTIVE);
-
+    assertThat(loadByClientId("NewRegistrar").getState()).isEqualTo(State.ACTIVE);
     runCommand("--registrar_state=SUSPENDED", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getState()).isEqualTo(State.SUSPENDED);
+    assertThat(loadByClientId("NewRegistrar").getState()).isEqualTo(State.SUSPENDED);
   }
 
   @Test
   public void testSuccess_allowedTlds() throws Exception {
     createTlds("xn--q9jyb4c", "foobar");
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    registrar = persistResource(
-        registrar.asBuilder().setAllowedTlds(ImmutableSet.of("xn--q9jyb4c")).build());
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setAllowedTlds(ImmutableSet.of("xn--q9jyb4c"))
+            .build());
     runCommand("--allowed_tlds=xn--q9jyb4c,foobar", "--force", "NewRegistrar");
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getAllowedTlds()).containsExactly("xn--q9jyb4c", "foobar");
+    assertThat(loadByClientId("NewRegistrar").getAllowedTlds())
+        .containsExactly("xn--q9jyb4c", "foobar");
   }
 
   @Test
   public void testSuccess_addAllowedTlds() throws Exception {
     createTlds("xn--q9jyb4c", "foo", "bar");
-    Registrar registrar = Registrar.loadByClientId("NewRegistrar");
-    registrar = persistResource(
-        registrar.asBuilder().setAllowedTlds(ImmutableSet.of("xn--q9jyb4c")).build());
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setAllowedTlds(ImmutableSet.of("xn--q9jyb4c"))
+            .build());
     runCommand("--add_allowed_tlds=foo,bar", "--force", "NewRegistrar");
-    registrar = Registrar.loadByClientId("NewRegistrar");
-    assertThat(registrar.getAllowedTlds()).containsExactly("xn--q9jyb4c", "foo", "bar");
+    assertThat(loadByClientId("NewRegistrar").getAllowedTlds())
+        .containsExactly("xn--q9jyb4c", "foo", "bar");
   }
 
   @Test
   public void testSuccess_addAllowedTldsWithDupes() throws Exception {
     createTlds("xn--q9jyb4c", "foo", "bar");
-    Registrar registrar = Registrar.loadByClientId("NewRegistrar");
-    registrar = persistResource(
-        registrar.asBuilder().setAllowedTlds(ImmutableSet.of("xn--q9jyb4c")).build());
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setAllowedTlds(ImmutableSet.of("xn--q9jyb4c"))
+            .build());
     runCommand("--add_allowed_tlds=xn--q9jyb4c,foo,bar", "--force", "NewRegistrar");
-    registrar = Registrar.loadByClientId("NewRegistrar");
-    assertThat(registrar.getAllowedTlds()).isEqualTo(ImmutableSet.of("xn--q9jyb4c", "foo", "bar"));
+    assertThat(loadByClientId("NewRegistrar").getAllowedTlds())
+        .isEqualTo(ImmutableSet.of("xn--q9jyb4c", "foo", "bar"));
   }
 
   @Test
   public void testSuccess_ipWhitelist() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIpAddressWhitelist()).isEmpty();
-
+    assertThat(loadByClientId("NewRegistrar").getIpAddressWhitelist()).isEmpty();
     runCommand("--ip_whitelist=192.168.1.1,192.168.0.2/16", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIpAddressWhitelist())
+    assertThat(loadByClientId("NewRegistrar").getIpAddressWhitelist())
         .containsExactly(
             CidrAddressBlock.create("192.168.1.1"), CidrAddressBlock.create("192.168.0.2/16"))
         .inOrder();
@@ -139,29 +133,26 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
 
   @Test
   public void testSuccess_clearIpWhitelist() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    persistResource(registrar.asBuilder()
-        .setIpAddressWhitelist(ImmutableList.of(
-            CidrAddressBlock.create("192.168.1.1"), CidrAddressBlock.create("192.168.0.2/16")))
-        .build());
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIpAddressWhitelist()).isNotEmpty();
-
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setIpAddressWhitelist(
+                ImmutableList.of(
+                    CidrAddressBlock.create("192.168.1.1"),
+                    CidrAddressBlock.create("192.168.0.2/16")))
+            .build());
+    assertThat(loadByClientId("NewRegistrar").getIpAddressWhitelist()).isNotEmpty();
     runCommand("--ip_whitelist=null", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIpAddressWhitelist()).isEmpty();
+    assertThat(loadByClientId("NewRegistrar").getIpAddressWhitelist()).isEmpty();
   }
 
   @Test
   public void testSuccess_certFile() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    Registrar registrar = checkNotNull(loadByClientId("NewRegistrar"));
     assertThat(registrar.getClientCertificate()).isNull();
     assertThat(registrar.getClientCertificateHash()).isNull();
-
     runCommand("--cert_file=" + getCertFilename(), "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    registrar = checkNotNull(loadByClientId("NewRegistrar"));
     // NB: Hash was computed manually using 'openssl x509 -fingerprint -sha256 -in ...' and then
     // converting the result from a hex string to non-padded base64 encoded string.
     assertThat(registrar.getClientCertificate()).isEqualTo(SAMPLE_CERT);
@@ -170,80 +161,63 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
 
   @Test
   public void testSuccess_certHash() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getClientCertificateHash()).isNull();
-
+    assertThat(loadByClientId("NewRegistrar").getClientCertificateHash()).isNull();
     runCommand("--cert_hash=" + SAMPLE_CERT_HASH, "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getClientCertificateHash()).isEqualTo(SAMPLE_CERT_HASH);
+    assertThat(loadByClientId("NewRegistrar").getClientCertificateHash())
+        .isEqualTo(SAMPLE_CERT_HASH);
   }
 
   @Test
   public void testSuccess_clearCert() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
     persistResource(
-        registrar.asBuilder().setClientCertificate(SAMPLE_CERT, DateTime.now(UTC)).build());
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(isNullOrEmpty(registrar.getClientCertificate())).isFalse();
-
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setClientCertificate(SAMPLE_CERT, DateTime.now(UTC))
+            .build());
+    assertThat(isNullOrEmpty(loadByClientId("NewRegistrar").getClientCertificate())).isFalse();
     runCommand("--cert_file=/dev/null", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getClientCertificate()).isNull();
+    assertThat(loadByClientId("NewRegistrar").getClientCertificate()).isNull();
   }
 
   @Test
   public void testSuccess_clearCertHash() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    persistResource(registrar.asBuilder().setClientCertificateHash(SAMPLE_CERT_HASH).build());
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(isNullOrEmpty(registrar.getClientCertificateHash())).isFalse();
-
+    persistResource(
+        loadByClientId("NewRegistrar")
+            .asBuilder()
+            .setClientCertificateHash(SAMPLE_CERT_HASH)
+            .build());
+    assertThat(isNullOrEmpty(loadByClientId("NewRegistrar").getClientCertificateHash())).isFalse();
     runCommand("--cert_hash=null", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getClientCertificateHash()).isNull();
+    assertThat(loadByClientId("NewRegistrar").getClientCertificateHash()).isNull();
   }
 
   @Test
   public void testSuccess_ianaId() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIanaIdentifier()).isEqualTo(8);
-
+    assertThat(loadByClientId("NewRegistrar").getIanaIdentifier()).isEqualTo(8);
     runCommand("--iana_id=12345", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getIanaIdentifier()).isEqualTo(12345);
+    assertThat(loadByClientId("NewRegistrar").getIanaIdentifier()).isEqualTo(12345);
   }
 
   @Test
   public void testSuccess_billingId() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBillingIdentifier()).isNull();
-
+    assertThat(loadByClientId("NewRegistrar").getBillingIdentifier()).isNull();
     runCommand("--billing_id=12345", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBillingIdentifier()).isEqualTo(12345);
+    assertThat(loadByClientId("NewRegistrar").getBillingIdentifier()).isEqualTo(12345);
   }
 
   @Test
   public void testSuccess_changeBillingMethodToBraintreeWhenBalanceIsZero() throws Exception {
     createTlds("xn--q9jyb4c");
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBillingMethod()).isEqualTo(BillingMethod.EXTERNAL);
-
+    assertThat(loadByClientId("NewRegistrar").getBillingMethod()).isEqualTo(BillingMethod.EXTERNAL);
     runCommand("--billing_method=braintree", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBillingMethod()).isEqualTo(BillingMethod.BRAINTREE);
+    assertThat(loadByClientId("NewRegistrar").getBillingMethod())
+        .isEqualTo(BillingMethod.BRAINTREE);
   }
 
   @Test
   public void testFailure_changeBillingMethodWhenBalanceIsNonZero() throws Exception {
     createTlds("xn--q9jyb4c");
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    Registrar registrar = checkNotNull(loadByClientId("NewRegistrar"));
     persistResource(
         new RegistrarBillingEntry.Builder()
             .setPrevious(null)
@@ -264,7 +238,7 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
     runCommand("--street=\"1234 Main St\"", "--street \"4th Floor\"", "--street \"Suite 1\"",
         "--city Brooklyn", "--state NY", "--zip 11223", "--cc US", "--force", "NewRegistrar");
 
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    Registrar registrar = checkNotNull(loadByClientId("NewRegistrar"));
     assertThat(registrar.getLocalizedAddress() != null).isTrue();
     assertThat(registrar.getLocalizedAddress().getStreet()).hasSize(3);
     assertThat(registrar.getLocalizedAddress().getStreet().get(0)).isEqualTo("1234 Main St");
@@ -278,53 +252,39 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
 
   @Test
   public void testSuccess_blockPremiumNames() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBlockPremiumNames()).isFalse();
-
+    assertThat(loadByClientId("NewRegistrar").getBlockPremiumNames()).isFalse();
     runCommand("--block_premium=true", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBlockPremiumNames()).isTrue();
+    assertThat(loadByClientId("NewRegistrar").getBlockPremiumNames()).isTrue();
   }
 
   @Test
   public void testSuccess_resetBlockPremiumNames() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    persistResource(registrar.asBuilder().setBlockPremiumNames(true).build());
-
+    persistResource(loadByClientId("NewRegistrar").asBuilder().setBlockPremiumNames(true).build());
     runCommand("--block_premium=false", "--force", "NewRegistrar");
-
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBlockPremiumNames()).isFalse();
+    assertThat(loadByClientId("NewRegistrar").getBlockPremiumNames()).isFalse();
   }
 
   @Test
   public void testSuccess_blockPremiumNamesUnspecified() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    persistResource(registrar.asBuilder().setBlockPremiumNames(true).build());
-
+    persistResource(loadByClientId("NewRegistrar").asBuilder().setBlockPremiumNames(true).build());
     // Make some unrelated change where we don't specify "--block_premium".
     runCommand("--billing_id=12345", "--force", "NewRegistrar");
-
     // Make sure the field didn't get reset back to false.
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
-    assertThat(registrar.getBlockPremiumNames()).isTrue();
+    assertThat(loadByClientId("NewRegistrar").getBlockPremiumNames()).isTrue();
   }
 
   @Test
   public void testSuccess_updateMultiple() throws Exception {
-    assertThat(Registrar.loadByClientId("TheRegistrar").getState()).isEqualTo(State.ACTIVE);
-    assertThat(Registrar.loadByClientId("NewRegistrar").getState()).isEqualTo(State.ACTIVE);
-
+    assertThat(loadByClientId("TheRegistrar").getState()).isEqualTo(State.ACTIVE);
+    assertThat(loadByClientId("NewRegistrar").getState()).isEqualTo(State.ACTIVE);
     runCommand("--registrar_state=SUSPENDED", "--force", "TheRegistrar", "NewRegistrar");
-
-    assertThat(Registrar.loadByClientId("TheRegistrar").getState()).isEqualTo(State.SUSPENDED);
-    assertThat(Registrar.loadByClientId("NewRegistrar").getState()).isEqualTo(State.SUSPENDED);
+    assertThat(loadByClientId("TheRegistrar").getState()).isEqualTo(State.SUSPENDED);
+    assertThat(loadByClientId("NewRegistrar").getState()).isEqualTo(State.SUSPENDED);
   }
 
   @Test
   public void testSuccess_resetOptionalParamsNullString() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    Registrar registrar = checkNotNull(loadByClientId("NewRegistrar"));
     registrar = persistResource(registrar.asBuilder()
         .setType(Type.PDT) // for non-null IANA ID
         .setIanaIdentifier(9995L)
@@ -356,7 +316,7 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
         "--force",
         "NewRegistrar");
 
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    registrar = checkNotNull(loadByClientId("NewRegistrar"));
     assertThat(registrar.getIanaIdentifier()).isNull();
     assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
@@ -368,7 +328,7 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
 
   @Test
   public void testSuccess_resetOptionalParamsEmptyString() throws Exception {
-    Registrar registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    Registrar registrar = checkNotNull(loadByClientId("NewRegistrar"));
     registrar = persistResource(registrar.asBuilder()
         .setType(Type.PDT) // for non-null IANA ID
         .setIanaIdentifier(9995L)
@@ -400,7 +360,7 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
         "--force",
         "NewRegistrar");
 
-    registrar = checkNotNull(Registrar.loadByClientId("NewRegistrar"));
+    registrar = checkNotNull(loadByClientId("NewRegistrar"));
     assertThat(registrar.getIanaIdentifier()).isNull();
     assertThat(registrar.getBillingIdentifier()).isNull();
     assertThat(registrar.getPhoneNumber()).isNull();
@@ -413,8 +373,7 @@ public class UpdateRegistrarCommandTest extends CommandTestCase<UpdateRegistrarC
   @Test
   public void testSuccess_setWhoisServer_works() throws Exception {
     runCommand("--whois=whois.goth.black", "--force", "NewRegistrar");
-    assertThat(Registrar.loadByClientId("NewRegistrar").getWhoisServer())
-        .isEqualTo("whois.goth.black");
+    assertThat(loadByClientId("NewRegistrar").getWhoisServer()).isEqualTo("whois.goth.black");
   }
 
   @Test