Avoid mocking in tests for EppMetric

Followup to []  Mocking shouldn't be used when you can use the real
implementation just as easily (and more robustly) - in particular, you almost
never want to mock a value type.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=132586826
This commit is contained in:
nickfelt 2016-09-08 12:33:00 -07:00 committed by Ben McIlwain
parent e478fd09fb
commit 36c6d59fee
4 changed files with 26 additions and 29 deletions

View file

@ -14,6 +14,7 @@
package google.registry.flows; package google.registry.flows;
import static com.google.common.truth.Truth.assertThat;
import static google.registry.flows.EppXmlTransformer.marshal; import static google.registry.flows.EppXmlTransformer.marshal;
import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions;
import static java.nio.charset.StandardCharsets.UTF_8; import static java.nio.charset.StandardCharsets.UTF_8;
@ -37,6 +38,7 @@ import org.junit.Before;
import org.junit.Rule; import org.junit.Rule;
import org.junit.Test; import org.junit.Test;
import org.junit.runner.RunWith; import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Matchers; import org.mockito.Matchers;
import org.mockito.Mock; import org.mockito.Mock;
import org.mockito.runners.MockitoJUnitRunner; import org.mockito.runners.MockitoJUnitRunner;
@ -50,8 +52,6 @@ public class EppControllerTest extends ShardableTestCase {
@Mock SessionMetadata sessionMetadata; @Mock SessionMetadata sessionMetadata;
@Mock TransportCredentials transportCredentials; @Mock TransportCredentials transportCredentials;
@Mock EppMetric.Builder eppMetricBuilder;
@Mock EppMetric eppMetric;
@Mock BigQueryMetricsEnqueuer metricsEnqueuer; @Mock BigQueryMetricsEnqueuer metricsEnqueuer;
@Mock FlowComponent.Builder flowComponentBuilder; @Mock FlowComponent.Builder flowComponentBuilder;
@Mock FlowComponent flowComponent; @Mock FlowComponent flowComponent;
@ -64,7 +64,7 @@ public class EppControllerTest extends ShardableTestCase {
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
when(sessionMetadata.getClientId()).thenReturn("foo"); when(sessionMetadata.getClientId()).thenReturn("some-client");
when(flowComponentBuilder.flowModule(Matchers.<FlowModule>any())) when(flowComponentBuilder.flowModule(Matchers.<FlowModule>any()))
.thenReturn(flowComponentBuilder); .thenReturn(flowComponentBuilder);
when(flowComponentBuilder.build()).thenReturn(flowComponent); when(flowComponentBuilder.build()).thenReturn(flowComponent);
@ -74,10 +74,9 @@ public class EppControllerTest extends ShardableTestCase {
when(eppOutput.getResponse()).thenReturn(eppResponse); when(eppOutput.getResponse()).thenReturn(eppResponse);
when(eppResponse.getResult()).thenReturn(result); when(eppResponse.getResult()).thenReturn(result);
when(result.getCode()).thenReturn(Code.SuccessWithNoMessages); when(result.getCode()).thenReturn(Code.SuccessWithNoMessages);
when(eppMetricBuilder.build()).thenReturn(eppMetric);
eppController = new EppController(); eppController = new EppController();
eppController.metric = eppMetricBuilder; eppController.metric = new EppMetric.Builder();
eppController.bigQueryMetricsEnqueuer = metricsEnqueuer; eppController.bigQueryMetricsEnqueuer = metricsEnqueuer;
eppController.clock = new FakeClock(); eppController.clock = new FakeClock();
eppController.flowComponentBuilder = flowComponentBuilder; eppController.flowComponentBuilder = flowComponentBuilder;
@ -93,6 +92,7 @@ public class EppControllerTest extends ShardableTestCase {
@Test @Test
public void testHandleEppCommand_unmarshallableData_exportsMetric() { public void testHandleEppCommand_unmarshallableData_exportsMetric() {
ArgumentCaptor<EppMetric> metricCaptor = ArgumentCaptor.forClass(EppMetric.class);
eppController.handleEppCommand( eppController.handleEppCommand(
sessionMetadata, sessionMetadata,
transportCredentials, transportCredentials,
@ -101,15 +101,16 @@ public class EppControllerTest extends ShardableTestCase {
false, false,
new byte[0]); new byte[0]);
verify(eppMetricBuilder).setClientId("foo"); verify(metricsEnqueuer).export(metricCaptor.capture());
verify(eppMetricBuilder).setPrivilegeLevel("NORMAL"); EppMetric metric = metricCaptor.getValue();
verify(eppMetricBuilder).setStatus(Code.SyntaxError); assertThat(metric.getClientId()).hasValue("some-client");
verify(eppMetricBuilder).build(); assertThat(metric.getPrivilegeLevel()).hasValue("NORMAL");
verify(metricsEnqueuer).export(eppMetric); assertThat(metric.getStatus()).hasValue(Code.SyntaxError);
} }
@Test @Test
public void testHandleEppCommand_regularEppCommand_exportsMetric() { public void testHandleEppCommand_regularEppCommand_exportsMetric() {
ArgumentCaptor<EppMetric> metricCaptor = ArgumentCaptor.forClass(EppMetric.class);
String domainCreateXml = String domainCreateXml =
loadFileWithSubstitutions( loadFileWithSubstitutions(
getClass(), "domain_create_prettyprinted.xml", ImmutableMap.<String, String>of()); getClass(), "domain_create_prettyprinted.xml", ImmutableMap.<String, String>of());
@ -121,12 +122,12 @@ public class EppControllerTest extends ShardableTestCase {
true, true,
domainCreateXml.getBytes(UTF_8)); domainCreateXml.getBytes(UTF_8));
verify(eppMetricBuilder).setClientId("foo"); verify(metricsEnqueuer).export(metricCaptor.capture());
verify(eppMetricBuilder).setPrivilegeLevel("SUPERUSER"); EppMetric metric = metricCaptor.getValue();
verify(eppMetricBuilder).setStatus(Code.SuccessWithNoMessages); assertThat(metric.getClientId()).hasValue("some-client");
verify(eppMetricBuilder).setCommandName("Create"); assertThat(metric.getPrivilegeLevel()).hasValue("SUPERUSER");
verify(eppMetricBuilder).setEppTarget("example.tld"); assertThat(metric.getStatus()).hasValue(Code.SuccessWithNoMessages);
verify(eppMetricBuilder).build(); assertThat(metric.getCommandName()).hasValue("Create");
verify(metricsEnqueuer).export(eppMetric); assertThat(metric.getEppTarget()).hasValue("example.tld");
} }
} }

View file

@ -48,7 +48,7 @@ interface EppTestComponent {
FakesAndMocksModule(FakeClock clock) { FakesAndMocksModule(FakeClock clock) {
this.clock = clock; this.clock = clock;
this.metrics = mock(EppMetric.Builder.class); this.metrics = new EppMetric.Builder();
this.modulesService = mock(ModulesService.class); this.modulesService = mock(ModulesService.class);
this.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class); this.metricsEnqueuer = mock(BigQueryMetricsEnqueuer.class);
} }

View file

@ -19,7 +19,6 @@ import static com.google.common.truth.Truth.assertThat;
import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions; import static google.registry.testing.TestDataHelper.loadFileWithSubstitutions;
import static java.nio.charset.StandardCharsets.UTF_8; import static java.nio.charset.StandardCharsets.UTF_8;
import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when; import static org.mockito.Mockito.when;
import com.google.appengine.api.users.User; import com.google.appengine.api.users.User;
@ -91,7 +90,7 @@ public class FlowRunnerTest extends ShardableTestCase {
flowRunner.isDryRun = false; flowRunner.isDryRun = false;
flowRunner.isSuperuser = false; flowRunner.isSuperuser = false;
flowRunner.isTransactional = false; flowRunner.isTransactional = false;
flowRunner.metric = mock(EppMetric.Builder.class); flowRunner.metric = new EppMetric.Builder();
flowRunner.sessionMetadata = flowRunner.sessionMetadata =
new StatelessRequestSessionMetadata("TheRegistrar", ImmutableSet.<String>of()); new StatelessRequestSessionMetadata("TheRegistrar", ImmutableSet.<String>of());
flowRunner.trid = Trid.create("client-123", "server-456"); flowRunner.trid = Trid.create("client-123", "server-456");
@ -110,18 +109,16 @@ public class FlowRunnerTest extends ShardableTestCase {
} }
@Test @Test
public void testRun_notIsTransactional_callsMetricIncrementAttempts() throws Exception { public void testRun_notIsTransactional_incrementsMetricAttempts() throws Exception {
flowRunner.run(); flowRunner.run();
assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1);
verify(flowRunner.metric).incrementAttempts();
} }
@Test @Test
public void testRun_isTransactional_callsMetricIncrementAttempts() throws Exception { public void testRun_isTransactional_incrementsMetricAttempts() throws Exception {
flowRunner.isTransactional = true; flowRunner.isTransactional = true;
flowRunner.run(); flowRunner.run();
assertThat(flowRunner.metric.build().getAttempts()).isEqualTo(1);
verify(flowRunner.metric).incrementAttempts();
} }
@Test @Test

View file

@ -87,13 +87,12 @@ public class LoginFlowViaConsoleTest extends LoginFlowTestCase {
} }
Environment login(final String name, final String authDomain, final String gaeUserId) { Environment login(final String name, final String authDomain, final String gaeUserId) {
oldEnv = ApiProxy.getCurrentEnvironment();
// This envAttr thing is the only way to set userId. // This envAttr thing is the only way to set userId.
// see https://code.google.com/p/googleappengine/issues/detail?id=3579 // see https://code.google.com/p/googleappengine/issues/detail?id=3579
final HashMap<String, Object> envAttr = new HashMap<>(); final HashMap<String, Object> envAttr = new HashMap<>(oldEnv.getAttributes());
envAttr.put("com.google.appengine.api.users.UserService.user_id_key", gaeUserId); envAttr.put("com.google.appengine.api.users.UserService.user_id_key", gaeUserId);
// And then.. this.
oldEnv = ApiProxy.getCurrentEnvironment();
final Environment e = oldEnv; final Environment e = oldEnv;
ApiProxy.setEnvironmentForCurrentThread(new Environment() { ApiProxy.setEnvironmentForCurrentThread(new Environment() {
@Override @Override