From 68768a561f64754d9472cc16235f472e6a04d1d5 Mon Sep 17 00:00:00 2001 From: guyben Date: Wed, 29 Nov 2017 13:20:55 -0800 Subject: [PATCH] Refactor EppToolVerifier to accept chaining verify commands We're doing this to allow several new tests: - xml files (that exist today) - xml files with substitutions - xml content (maybe? Currently private. Caching the files seems more readable) - no data at all Instead of having only one interface eppToolVerifier.verifySent("file1.xml", "file2.xml"); we're refactoring to allow: eppToolVerifier .verifySent("file1.xml") .verifySentAny() // we don't care about this epps .verifySent("file2.xml", substitutions) .verifyNoMoreSent(); In this case we're checking that "exactly 3 EPPs were sent, where the 1st one has content from file1.xml, and the 3rd one has the content from file2.xml, after the given substitutions were applied" This also updates EppToolCommandTestCase to have only one EppToolVerifier, and always finish by checking verifyNoMoreSent, meaning that in every test - all sent epps must be accounted for (verified or skiped) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=177353887 --- .../tools/AllocateDomainCommandTest.java | 28 ++- .../tools/CreateAnchorTenantCommandTest.java | 12 +- .../tools/CreateContactCommandTest.java | 4 +- .../tools/CreateDomainCommandTest.java | 8 +- .../registry/tools/CreateHostCommandTest.java | 4 +- .../tools/DeleteDomainCommandTest.java | 8 +- .../registry/tools/DeleteHostCommandTest.java | 8 +- .../DomainApplicationInfoCommandTest.java | 4 +- .../tools/DomainCheckClaimsCommandTest.java | 16 +- .../tools/DomainCheckCommandTest.java | 16 +- .../tools/DomainCheckFeeCommandTest.java | 16 +- .../registry/tools/EppToolCommandTest.java | 7 +- .../tools/EppToolCommandTestCase.java | 17 +- .../registry/tools/EppToolVerifier.java | 192 ++++++++++++++---- .../registry/tools/ExecuteEppCommandTest.java | 12 +- .../registry/tools/LockDomainCommandTest.java | 34 ++-- .../UniformRapidSuspensionCommandTest.java | 26 +-- .../tools/UnlockDomainCommandTest.java | 34 ++-- .../tools/UpdateDomainCommandTest.java | 19 +- .../tools/UpdateServerLocksCommandTest.java | 10 +- 20 files changed, 285 insertions(+), 190 deletions(-) diff --git a/javatests/google/registry/tools/AllocateDomainCommandTest.java b/javatests/google/registry/tools/AllocateDomainCommandTest.java index 5b06af429..a8d1bb0b2 100644 --- a/javatests/google/registry/tools/AllocateDomainCommandTest.java +++ b/javatests/google/registry/tools/AllocateDomainCommandTest.java @@ -41,28 +41,31 @@ import google.registry.model.domain.secdns.DelegationSignerData; import google.registry.model.eppcommon.Trid; import google.registry.model.eppinput.EppInput; import google.registry.model.reporting.HistoryEntry; -import google.registry.tools.ServerSideCommand.Connection; import google.registry.tools.server.ToolsTestData; import java.io.IOException; import org.joda.time.DateTime; +import org.junit.After; import org.junit.Before; import org.junit.Test; -import org.mockito.Mock; /** Unit tests for {@link AllocateDomainCommand}. */ public class AllocateDomainCommandTest extends CommandTestCase { - @Mock - Connection connection; + private EppToolVerifier eppVerifier; @Before public void init() throws IOException { - command.setConnection(connection); + eppVerifier = EppToolVerifier.create(command).expectClientId("TheRegistrar").expectSuperuser(); createTld("tld", QUIET_PERIOD); createApplication("example-one.tld", "domain_create_sunrush.xml", "1-TLD"); createApplication("example-two.tld", "domain_create_sunrush2.xml", "2-TLD"); } + @After + public void cleanup() throws Exception { + eppVerifier.verifyNoMoreSent(); + } + private void createApplication(String name, String xmlFile, String repoId) throws IOException { DomainApplication application = persistResource(newDomainApplication(name) @@ -105,30 +108,25 @@ public class AllocateDomainCommandTest extends CommandTestCase { runCommandForced( "--client=NewRegistrar", ToolsTestData.loadUtf8("contact_create.xml")); - eppVerifier().verifySent("contact_create.xml"); + eppVerifier.verifySent("contact_create.xml"); } @Test @@ -63,7 +63,10 @@ public class EppToolCommandTest extends EppToolCommandTestCase { ToolsTestData.loadUtf8("contact_create.xml"), ToolsTestData.loadUtf8("domain_check.xml"), ToolsTestData.loadUtf8("domain_check_fee.xml")); - eppVerifier().verifySent("contact_create.xml", "domain_check.xml", "domain_check_fee.xml"); + eppVerifier + .verifySent("contact_create.xml") + .verifySent("domain_check.xml") + .verifySent("domain_check_fee.xml"); } @Test diff --git a/javatests/google/registry/tools/EppToolCommandTestCase.java b/javatests/google/registry/tools/EppToolCommandTestCase.java index 8c6a33278..6e065f83f 100644 --- a/javatests/google/registry/tools/EppToolCommandTestCase.java +++ b/javatests/google/registry/tools/EppToolCommandTestCase.java @@ -16,29 +16,30 @@ package google.registry.tools; import static google.registry.testing.DatastoreHelper.createTlds; -import google.registry.tools.ServerSideCommand.Connection; +import org.junit.After; import org.junit.Before; -import org.mockito.Mock; /** * Abstract class for commands that construct + send EPP commands. * + * Has an EppToolVerifier member that needs to have all epp messages accounted for before the test + * has ended. + * * @param the command type */ public abstract class EppToolCommandTestCase extends CommandTestCase { - @Mock - Connection connection; + EppToolVerifier eppVerifier; @Before public void init() throws Exception { // Create two TLDs for commands that allow multiple TLDs at once. createTlds("tld", "tld2"); - command.setConnection(connection); + eppVerifier = EppToolVerifier.create(command).expectClientId("NewRegistrar"); } - /** Helper to get a new {@link EppToolVerifier} instance. */ - EppToolVerifier eppVerifier() { - return new EppToolVerifier().withConnection(connection).withClientId("NewRegistrar"); + @After + public void cleanup() throws Exception { + eppVerifier.verifyNoMoreSent(); } } diff --git a/javatests/google/registry/tools/EppToolVerifier.java b/javatests/google/registry/tools/EppToolVerifier.java index 6750fa693..4cd1c6893 100644 --- a/javatests/google/registry/tools/EppToolVerifier.java +++ b/javatests/google/registry/tools/EppToolVerifier.java @@ -14,90 +14,192 @@ package google.registry.tools; -import static com.google.common.collect.ImmutableList.toImmutableList; +import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; import static google.registry.xml.XmlTestUtils.assertXmlEquals; import static java.nio.charset.StandardCharsets.UTF_8; import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.times; +import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.net.MediaType; import google.registry.tools.ServerSideCommand.Connection; import google.registry.tools.server.ToolsTestData; import java.net.URLDecoder; -import java.util.Arrays; -import java.util.List; import java.util.Map; import org.mockito.ArgumentCaptor; -/** Class for verifying EPP commands sent to the server via the tool endpoint. */ +/** + * Class for verifying EPP commands sent to the server via the tool endpoint. + * + *

Provides its own (mock) {@link Connection} that will be monitored for EPP transmission. This + * Connection needs to be registered with the tool endpoint - something like this: + * + * 

   {@code
+ * SomeToolCommand command = ...;
+ * EppToolVerifier eppToolVerifier = EppToolVerifier.create(command);
+ * // run command...
+ * eppToolVerifier.expectClientId("SomeClientId").verifySent("some_epp_file.xml");
+ * }
+ */ public class EppToolVerifier { - private final Connection connection; - private final String clientId; - private final boolean superuser; - private final boolean dryRun; + private final Connection connection = mock(Connection.class); - public EppToolVerifier() { - this(null, null, false, false); + private String clientId; + private boolean superuser; + private boolean dryRun; + private ImmutableList capturedParams; + private int paramIndex; + + private EppToolVerifier() {} + + /** Creates an EppToolVerifier that monitors EPPs sent by the given command. */ + public static EppToolVerifier create(EppToolCommand command) { + EppToolVerifier eppToolVerifier = new EppToolVerifier(); + command.setConnection(eppToolVerifier.getConnection()); + return eppToolVerifier; } - private EppToolVerifier( - Connection connection, String clientId, boolean superuser, boolean dryRun) { - this.connection = connection; + /** + * Sets the expected clientId for any following verifySent command. + * + *

Must be called at least once before any {@link verifySent} calls. + */ + public EppToolVerifier expectClientId(String clientId) { this.clientId = clientId; - this.superuser = superuser; - this.dryRun = dryRun; + return this; } - EppToolVerifier withConnection(Connection connection) { - return new EppToolVerifier(connection, clientId, superuser, dryRun); + /** + * Declares that any following verifySent command expects the "superuser" flag to be set. + * + *

If not called, {@link verifySent} will expect the "superuser" flag to be false. + */ + public EppToolVerifier expectSuperuser() { + this.superuser = true; + return this; } - EppToolVerifier withClientId(String clientId) { - return new EppToolVerifier(connection, clientId, superuser, dryRun); + /** + * Declares that any following verifySent command expects the "dryRun" flag to be set. + * + *

If not called, {@link verifySent} will expect the "dryRun" flag to be false. + */ + public EppToolVerifier expectDryRun() { + this.dryRun = true; + return this; } - EppToolVerifier asSuperuser() { - return new EppToolVerifier(connection, clientId, true, dryRun); + /** + * Tests that the expected EPP was sent. + * + *

The expected EPP must have the correct "clientId", "dryRun" and "superuser" flags as set by + * the various "expect*" calls. + * + *

The expected EPP's content is checked against the given file. + * + *

If multiple EPPs are expected, the verifySent* call order must match the actual EPP order. + * + * @param expectedXmlFile the name of the file holding the expected content of the EPP. The file + * resides in the tools/server/testdata directory. + */ + public EppToolVerifier verifySent(String expectedXmlFile) throws Exception { + return verifySentContents(ToolsTestData.loadUtf8(expectedXmlFile)); } - EppToolVerifier asDryRun() { - return new EppToolVerifier(connection, clientId, superuser, true); + /** + * Tests that the expected EPP was sent, with the given substitutions. + * + *

The expected EPP must have the correct "clientId", "dryRun" and "superuser" flags as set by + * the various "expect*" calls. + * + *

The expected EPP's content is checked against the given file after the given substitutions + * have been applied. + * + *

If multiple EPPs are expected, the verifySent* call order must match the EPP order. + * + * @param expectedXmlFile the name of the file holding the expected content of the EPP. The file + * resides in the tools/server/testdata directory. + * @param substitutions a list of substitutions to apply on the expectedXmlFile + */ + public EppToolVerifier verifySent(String expectedXmlFile, Map substitutions) + throws Exception { + return verifySentContents(ToolsTestData.loadUtf8(expectedXmlFile, substitutions)); } - void verifySent(String... expectedXmlFiles) throws Exception { - verifySentContents( - Arrays.stream(expectedXmlFiles).map(ToolsTestData::loadUtf8).collect(toImmutableList())); + /** + * Tests an EPP was sent, without checking the contents. + * + *

The expected EPP are not check for its content or any of the "contentId" / "superuser" / + * "dryRun" flags - only that it exists. + * + *

If multiple EPPs are expected, the verifySent* call order must match the EPP order. + */ + public EppToolVerifier verifySentAny() throws Exception { + setArgumentsIfNeeded(); + paramIndex++; + assertThat(capturedParams.size()).isAtLeast(paramIndex); + return this; } - void verifySentContents(List expectedXmlContents) throws Exception { + /** + * Test that no more EPPs were sent, after any that were expected in previous "verifySent" calls. + */ + public void verifyNoMoreSent() throws Exception { + setArgumentsIfNeeded(); + assertThat( + capturedParams + .stream() + .skip(paramIndex) + .map(bytes -> new String(bytes, UTF_8)) + .toArray()) + .isEmpty(); + } + + private void setArgumentsIfNeeded() throws Exception { + if (capturedParams != null) { + return; + } ArgumentCaptor params = ArgumentCaptor.forClass(byte[].class); - verify(connection, times(expectedXmlContents.size())).send( + verify(connection, atLeast(0)).send( eq("/_dr/epptool"), eq(ImmutableMap.of()), eq(MediaType.FORM_DATA), params.capture()); - List capturedParams = params.getAllValues(); - assertThat(capturedParams).hasSize(expectedXmlContents.size()); - for (int i = 0; i < expectedXmlContents.size(); i++) { - byte[] capturedParam = capturedParams.get(i); - Map map = - Splitter.on('&').withKeyValueSeparator('=').split(new String(capturedParam, UTF_8)); - assertThat(map).hasSize(4); - assertXmlEquals( - expectedXmlContents.get(i), URLDecoder.decode(map.get("xml"), UTF_8.toString())); - assertThat(map).containsEntry("dryRun", Boolean.toString(dryRun)); - assertThat(map).containsEntry("clientId", clientId); - assertThat(map).containsEntry("superuser", Boolean.toString(superuser)); - } + capturedParams = ImmutableList.copyOf(params.getAllValues()); + paramIndex = 0; } - void verifyNothingSent() { - verifyZeroInteractions(connection); + private String bytesToXml(byte[] bytes) throws Exception { + checkState(clientId != null, "expectClientId must be called before any verifySent command"); + Map map = + Splitter.on('&') + .withKeyValueSeparator('=') + .split(new String(bytes, UTF_8)); + assertThat(map).hasSize(4); + assertThat(map).containsEntry("dryRun", Boolean.toString(dryRun)); + assertThat(map).containsEntry("clientId", clientId); + assertThat(map).containsEntry("superuser", Boolean.toString(superuser)); + return URLDecoder.decode(map.get("xml"), UTF_8.toString()); + } + + private EppToolVerifier verifySentContents(String expectedXmlContent) throws Exception { + setArgumentsIfNeeded(); + assertThat(capturedParams.size()).isGreaterThan(paramIndex); + assertXmlEquals( + expectedXmlContent, + bytesToXml(capturedParams.get(paramIndex))); + paramIndex++; + return this; + } + + /** Returns the (mock) Connection that is being monitored by this verifier. */ + private Connection getConnection() { + return connection; } } diff --git a/javatests/google/registry/tools/ExecuteEppCommandTest.java b/javatests/google/registry/tools/ExecuteEppCommandTest.java index e573b226e..3b5c59020 100644 --- a/javatests/google/registry/tools/ExecuteEppCommandTest.java +++ b/javatests/google/registry/tools/ExecuteEppCommandTest.java @@ -42,19 +42,19 @@ public class ExecuteEppCommandTest extends EppToolCommandTestCase { - /** Gets an overridden eppVerifier that has superuser set to true on it. */ - @Override - EppToolVerifier eppVerifier() { - return new EppToolVerifier() - .withConnection(connection) - .withClientId("NewRegistrar") - .asSuperuser(); + @Before + public void before() { + eppVerifier.expectSuperuser(); } @Test public void testSuccess_sendsCorrectEppXml() throws Exception { persistActiveDomain("example.tld"); runCommandForced("--client=NewRegistrar", "example.tld"); - eppVerifier() - .verifySentContents( - ImmutableList.of( - loadUtf8("domain_lock.xml", ImmutableMap.of("DOMAIN", "example.tld")))); + eppVerifier.verifySent("domain_lock.xml", ImmutableMap.of("DOMAIN", "example.tld")); } @Test @@ -59,24 +52,24 @@ public class LockDomainCommandTest extends EppToolCommandTestCase params = new ArrayList<>(); - List expectedXmls = new ArrayList<>(); - params.add("--client=NewRegistrar"); // Create 26 domains -- one more than the number of entity groups allowed in a transaction (in // case that was going to be the failure point). + List domains = new ArrayList<>(); for (int n = 0; n < 26; n++) { String domain = String.format("domain%d.tld", n); persistActiveDomain(domain); - params.add(domain); - expectedXmls.add(loadUtf8("domain_lock.xml", ImmutableMap.of("DOMAIN", domain))); + domains.add(domain); + } + runCommandForced( + ImmutableList.builder().add("--client=NewRegistrar").addAll(domains).build()); + for (String domain : domains) { + eppVerifier.verifySent("domain_lock.xml", ImmutableMap.of("DOMAIN", domain)); } - runCommandForced(params); - eppVerifier().verifySentContents(expectedXmls); } @Test @@ -96,7 +89,6 @@ public class LockDomainCommandTest extends EppToolCommandTestCase { - /** Gets an overridden eppVerifier that has superuser set to true on it. */ - @Override - EppToolVerifier eppVerifier() { - return new EppToolVerifier() - .withConnection(connection) - .withClientId("NewRegistrar") - .asSuperuser(); + @Before + public void before() { + eppVerifier.expectSuperuser(); } private static void persistLockedDomain(String domainName) { @@ -57,10 +53,7 @@ public class UnlockDomainCommandTest extends EppToolCommandTestCase params = new ArrayList<>(); - List expectedXmls = new ArrayList<>(); - params.add("--client=NewRegistrar"); // Create 26 domains -- one more than the number of entity groups allowed in a transaction (in // case that was going to be the failure point). + List domains = new ArrayList<>(); for (int n = 0; n < 26; n++) { String domain = String.format("domain%d.tld", n); persistLockedDomain(domain); - params.add(domain); - expectedXmls.add(loadUtf8("domain_unlock.xml", ImmutableMap.of("DOMAIN", domain))); + domains.add(domain); + } + runCommandForced( + ImmutableList.builder().add("--client=NewRegistrar").addAll(domains).build()); + for (String domain : domains) { + eppVerifier.verifySent("domain_unlock.xml", ImmutableMap.of("DOMAIN", domain)); } - runCommandForced(params); - eppVerifier().verifySentContents(expectedXmls); } @Test @@ -104,7 +97,6 @@ public class UnlockDomainCommandTest extends EppToolCommandTestCase