From bbe7c3fc88c64e782bb06769346b61bb7fc18109 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 26 Sep 2019 11:21:09 +0900 Subject: [PATCH 1/2] MultipleQuotaPluginsIT: Clean up and fix tests The refillsOnException test is intended to verify that when there are multiple plugins, and an exception is thrown by one of them causing the operation to fail, quota previously deducted by the ones that succeeded should be refilled. The test was instead throwing the exception from the first plugin, which means no plugins were actually deducting. It was expecting the refill on the second one, but not verifying that the call was made. Adding the call to verify() causes the test to fail, because no refill gets called. Rework the test so that the successfull deduction is done first, and the second one fails with an exception. Verify that the refill is called on the first one that was previously successfull. Also add missing calls to verify on other tests, thus confirming that the expected calls are actually occurring. Change-Id: I2cb478d0f64ad1cf66b81a051b126e1f87bcc7c7 --- .../server/quota/MultipleQuotaPluginsIT.java | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java index 0ad2010564..46d11c4d57 100644 --- a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java @@ -87,15 +87,18 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { .isEqualTo( QuotaResponse.Aggregated.create( ImmutableList.of(QuotaResponse.ok(), QuotaResponse.error("fail")))); + + verify(quotaEnforcerA); + verify(quotaEnforcerB); } @Test public void refillsOnException() { NullPointerException exception = new NullPointerException(); QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andThrow(exception); - expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok()); - quotaEnforcerB.refill("testGroup", ctx, 1); + expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok()); + expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andThrow(exception); + quotaEnforcerA.refill("testGroup", ctx, 1); expectLastCall(); replay(quotaEnforcerA); @@ -108,6 +111,7 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { assertThat(thrown).isEqualTo(exception); verify(quotaEnforcerA); + verify(quotaEnforcerB); } @Test @@ -124,6 +128,9 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { .isEqualTo( QuotaResponse.Aggregated.create( ImmutableList.of(QuotaResponse.error("fail"), QuotaResponse.noOp()))); + + verify(quotaEnforcerA); + verify(quotaEnforcerB); } @Test @@ -139,6 +146,9 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { quotaBackend.user(identifiedAdmin).availableTokens("testGroup").availableTokens(); assertThat(tokens).isPresent(); assertThat(tokens.getAsLong()).isEqualTo(10L); + + verify(quotaEnforcerA); + verify(quotaEnforcerB); } @Test @@ -154,5 +164,8 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { quotaBackend.user(identifiedAdmin).availableTokens("testGroup").availableTokens(); assertThat(tokens).isPresent(); assertThat(tokens.getAsLong()).isEqualTo(20L); + + verify(quotaEnforcerA); + verify(quotaEnforcerB); } } From 4f40272758a30646f27f5697103f188cf2c3c35d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 26 Sep 2019 11:42:41 +0900 Subject: [PATCH 2/2] MultipleQuotaPluginsIT: Migrate from easymock to mockito Bug: Issue 5057 Change-Id: I70fc70147229f947dacc364bb52c1a41b6781144 --- .../server/quota/MultipleQuotaPluginsIT.java | 83 +++++++------------ 1 file changed, 30 insertions(+), 53 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java index 46d11c4d57..c6b09ccc5a 100644 --- a/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java +++ b/javatests/com/google/gerrit/acceptance/server/quota/MultipleQuotaPluginsIT.java @@ -17,11 +17,10 @@ package com.google.gerrit.acceptance.server.quota; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth8.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.expectLastCall; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.resetToStrict; -import static org.easymock.EasyMock.verify; +import static org.mockito.Mockito.clearInvocations; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -35,15 +34,12 @@ import com.google.gerrit.server.quota.QuotaResponse; import com.google.inject.Inject; import com.google.inject.Module; import java.util.OptionalLong; -import org.easymock.EasyMock; import org.junit.Before; import org.junit.Test; public class MultipleQuotaPluginsIT extends AbstractDaemonTest { - private static final QuotaEnforcer quotaEnforcerA = - EasyMock.createStrictMock(QuotaEnforcer.class); - private static final QuotaEnforcer quotaEnforcerB = - EasyMock.createStrictMock(QuotaEnforcer.class); + private static final QuotaEnforcer quotaEnforcerA = mock(QuotaEnforcer.class); + private static final QuotaEnforcer quotaEnforcerB = mock(QuotaEnforcer.class); private IdentifiedUser identifiedAdmin; @Inject private QuotaBackend quotaBackend; @@ -67,42 +63,32 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { @Before public void setUp() { identifiedAdmin = identifiedUserFactory.create(admin.id()); - resetToStrict(quotaEnforcerA); - resetToStrict(quotaEnforcerB); + clearInvocations(quotaEnforcerA); + clearInvocations(quotaEnforcerB); } @Test public void refillsOnError() { QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok()); - expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)) - .andReturn(QuotaResponse.error("fail")); - quotaEnforcerA.refill("testGroup", ctx, 1); - expectLastCall(); - - replay(quotaEnforcerA); - replay(quotaEnforcerB); + when(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).thenReturn(QuotaResponse.ok()); + when(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).thenReturn(QuotaResponse.error("fail")); assertThat(quotaBackend.user(identifiedAdmin).requestToken("testGroup")) .isEqualTo( QuotaResponse.Aggregated.create( ImmutableList.of(QuotaResponse.ok(), QuotaResponse.error("fail")))); - verify(quotaEnforcerA); - verify(quotaEnforcerB); + verify(quotaEnforcerA).requestTokens("testGroup", ctx, 1); + verify(quotaEnforcerB).requestTokens("testGroup", ctx, 1); + verify(quotaEnforcerA).refill("testGroup", ctx, 1); } @Test public void refillsOnException() { NullPointerException exception = new NullPointerException(); QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.ok()); - expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andThrow(exception); - quotaEnforcerA.refill("testGroup", ctx, 1); - expectLastCall(); - - replay(quotaEnforcerA); - replay(quotaEnforcerB); + when(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).thenReturn(QuotaResponse.ok()); + when(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).thenThrow(exception); NullPointerException thrown = assertThrows( @@ -110,62 +96,53 @@ public class MultipleQuotaPluginsIT extends AbstractDaemonTest { () -> quotaBackend.user(identifiedAdmin).requestToken("testGroup")); assertThat(thrown).isEqualTo(exception); - verify(quotaEnforcerA); - verify(quotaEnforcerB); + verify(quotaEnforcerA).requestTokens("testGroup", ctx, 1); + verify(quotaEnforcerB).requestTokens("testGroup", ctx, 1); + verify(quotaEnforcerA).refill("testGroup", ctx, 1); } @Test public void doesNotRefillNoOp() { QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.requestTokens("testGroup", ctx, 1)) - .andReturn(QuotaResponse.error("fail")); - expect(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).andReturn(QuotaResponse.noOp()); - - replay(quotaEnforcerA); - replay(quotaEnforcerB); + when(quotaEnforcerA.requestTokens("testGroup", ctx, 1)).thenReturn(QuotaResponse.error("fail")); + when(quotaEnforcerB.requestTokens("testGroup", ctx, 1)).thenReturn(QuotaResponse.noOp()); assertThat(quotaBackend.user(identifiedAdmin).requestToken("testGroup")) .isEqualTo( QuotaResponse.Aggregated.create( ImmutableList.of(QuotaResponse.error("fail"), QuotaResponse.noOp()))); - verify(quotaEnforcerA); - verify(quotaEnforcerB); + verify(quotaEnforcerA).requestTokens("testGroup", ctx, 1); + verify(quotaEnforcerB).requestTokens("testGroup", ctx, 1); } @Test public void minimumAvailableTokens() { QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.availableTokens("testGroup", ctx)).andReturn(QuotaResponse.ok(20L)); - expect(quotaEnforcerB.availableTokens("testGroup", ctx)).andReturn(QuotaResponse.ok(10L)); - - replay(quotaEnforcerA); - replay(quotaEnforcerB); + when(quotaEnforcerA.availableTokens("testGroup", ctx)).thenReturn(QuotaResponse.ok(20L)); + when(quotaEnforcerB.availableTokens("testGroup", ctx)).thenReturn(QuotaResponse.ok(10L)); OptionalLong tokens = quotaBackend.user(identifiedAdmin).availableTokens("testGroup").availableTokens(); assertThat(tokens).isPresent(); assertThat(tokens.getAsLong()).isEqualTo(10L); - verify(quotaEnforcerA); - verify(quotaEnforcerB); + verify(quotaEnforcerA).availableTokens("testGroup", ctx); + verify(quotaEnforcerB).availableTokens("testGroup", ctx); } @Test public void ignoreNoOpForAvailableTokens() { QuotaRequestContext ctx = QuotaRequestContext.builder().user(identifiedAdmin).build(); - expect(quotaEnforcerA.availableTokens("testGroup", ctx)).andReturn(QuotaResponse.noOp()); - expect(quotaEnforcerB.availableTokens("testGroup", ctx)).andReturn(QuotaResponse.ok(20L)); - - replay(quotaEnforcerA); - replay(quotaEnforcerB); + when(quotaEnforcerA.availableTokens("testGroup", ctx)).thenReturn(QuotaResponse.noOp()); + when(quotaEnforcerB.availableTokens("testGroup", ctx)).thenReturn(QuotaResponse.ok(20L)); OptionalLong tokens = quotaBackend.user(identifiedAdmin).availableTokens("testGroup").availableTokens(); assertThat(tokens).isPresent(); assertThat(tokens.getAsLong()).isEqualTo(20L); - verify(quotaEnforcerA); - verify(quotaEnforcerB); + verify(quotaEnforcerA).availableTokens("testGroup", ctx); + verify(quotaEnforcerB).availableTokens("testGroup", ctx); } }