Adapt the test code to not mock cache instances
Recent Bazel upgrades Error Prone major version to 2.4.0, that is much
stricter now. Particularly DoNotMock pattern doesn't allow to Mock cache
instances any more:
RateLimitUploadListenerTest.java:44: error: [DoNotMock] Do not mock
'com.google.common.cache.LoadingCache<java.lang.String,com.googlesource.gerrit.plugins.quota.Module.Holder>'
(which is-a 'com.google.common.cache.Cache'); com.google.common.cache.Cache is annotated as @DoNotMock:
Use CacheBuilder.newBuilder().build().
@Mock private LoadingCache<String, Holder> limitsPerRemoteHost;
^
(see https://errorprone.info/bugpattern/DoNotMock)
Adapt the code to not mock the cache instance.
One side effect of this change, is that testDoFilterCacheMiss test is
removed.
The ExecutionException is thrown when a cache loader throws a (runtime)
exception while loading a value (which wasn't present in the cache). The
thrown ExecutionException wraps the root cause exception. However, the
reason for testing this behavior here is not clear. Further, it is also
not clear why should in the case of ExecutionException all requests be
rejected as if the rate limit was exceeded.
Change-Id: Ieb1327ed71df2f72bcd7243fae9821168dc07238
(cherry picked from commit dbe485d16d3193b5f40fb46e5a89b918b3292630)
diff --git a/src/test/java/com/googlesource/gerrit/plugins/quota/RateLimitUploadListenerTest.java b/src/test/java/com/googlesource/gerrit/plugins/quota/RateLimitUploadListenerTest.java
index fc6a196..fc4dc76 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/quota/RateLimitUploadListenerTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/quota/RateLimitUploadListenerTest.java
@@ -19,14 +19,20 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.google.common.cache.CacheBuilder;
import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.RateLimiter;
import com.google.gerrit.entities.Account;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser.GenericFactory;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Provider;
+import com.googlesource.gerrit.plugins.quota.AccountLimitsConfig.Type;
import com.googlesource.gerrit.plugins.quota.Module.Holder;
import java.util.concurrent.ExecutionException;
+import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -39,9 +45,13 @@
private static final String LIMIT_EXCEEDED_MSG = "test exceeded message: {0,number,##.##}";
private static final String REMOTE_HOST = "host";
private RateLimitUploadListener uploadHook;
+ private LoadingCache<Account.Id, Holder> limitsPerAccount;
+ private LoadingCache<String, Holder> limitsPerRemoteHost;
+ @Mock @GerritServerConfig Config cfg;
+ @Mock GenericFactory userFactory;
+ SystemGroupBackend systemGroupBackend;
+ @Mock AccountLimitsFinder finder;
@Mock private Provider<CurrentUser> user;
- @Mock private LoadingCache<Account.Id, Holder> limitsPerAccount;
- @Mock private LoadingCache<String, Holder> limitsPerRemoteHost;
@Mock(answer = Answers.RETURNS_DEEP_STUBS)
private CurrentUser currentUser;
@@ -52,6 +62,17 @@
@Before
public void setUp() {
+ systemGroupBackend = new SystemGroupBackend(cfg);
+ limitsPerAccount =
+ CacheBuilder.newBuilder()
+ .build(new Module.HolderCacheLoaderByAccountId(Type.UPLOADPACK, userFactory, finder));
+ limitsPerAccount.put(accountId, holder);
+ limitsPerRemoteHost =
+ CacheBuilder.newBuilder()
+ .build(
+ new Module.HolderCacheLoaderByRemoteHost(
+ Type.UPLOADPACK, systemGroupBackend, finder));
+ limitsPerRemoteHost.put(REMOTE_HOST, holder);
uploadHook =
spy(
new RateLimitUploadListener(
@@ -62,19 +83,16 @@
private void setUpRegisteredUser() throws ExecutionException {
when(currentUser.isIdentifiedUser()).thenReturn(true);
when(currentUser.asIdentifiedUser().getAccountId()).thenReturn(accountId);
- when(limitsPerAccount.get(accountId)).thenReturn(holder);
when(holder.get()).thenReturn(limiter);
}
private void setUpRegisteredUserExecutionException() throws ExecutionException {
when(currentUser.isIdentifiedUser()).thenReturn(true);
when(currentUser.asIdentifiedUser().getAccountId()).thenReturn(accountId);
- when(limitsPerAccount.get(accountId)).thenThrow(new ExecutionException(null));
}
private void setUpAnonymous() throws ExecutionException {
when(currentUser.isIdentifiedUser()).thenReturn(false);
- when(limitsPerRemoteHost.get(REMOTE_HOST)).thenReturn(holder);
when(holder.get()).thenReturn(limiter);
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/quota/RestApiRateLimiterTest.java b/src/test/java/com/googlesource/gerrit/plugins/quota/RestApiRateLimiterTest.java
index 4795212..7dafad3 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/quota/RestApiRateLimiterTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/quota/RestApiRateLimiterTest.java
@@ -24,11 +24,16 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.google.common.cache.CacheBuilder;
import com.google.common.cache.LoadingCache;
import com.google.common.util.concurrent.RateLimiter;
import com.google.gerrit.entities.Account;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser.GenericFactory;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Provider;
+import com.googlesource.gerrit.plugins.quota.AccountLimitsConfig.Type;
import com.googlesource.gerrit.plugins.quota.Module.Holder;
import java.io.IOException;
import java.util.concurrent.ExecutionException;
@@ -36,6 +41,7 @@
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -47,12 +53,11 @@
public class RestApiRateLimiterTest {
private static final String LIMIT_EXCEEDED_MSG =
"test exceeded message: {0,number,##.##}, {1,number,###}";
+ private static final String REMOTE_HOST = "host";
@Mock private HttpServletRequest req;
@Mock private HttpServletResponse res;
@Mock private FilterChain chain;
@Mock private Provider<CurrentUser> user;
- @Mock private LoadingCache<Account.Id, Holder> limitsPerAccount;
- @Mock private LoadingCache<String, Holder> limitsPerRemoteHost;
@Mock(answer = Answers.RETURNS_DEEP_STUBS)
private CurrentUser currentUser;
@@ -63,10 +68,32 @@
private Holder holder;
@Mock private RateLimiter rateLimiter;
+
+ @Mock @GerritServerConfig Config cfg;
+ @Mock GenericFactory userFactory;
+ @Mock AccountLimitsFinder finder;
+
private RestApiRateLimiter restReqFilter;
+ private SystemGroupBackend systemGroupBackend;
+ private LoadingCache<Account.Id, Holder> limitsPerAccount;
+ private LoadingCache<String, Holder> limitsPerRemoteHost;
@Before
public void setUp() throws IOException, ServletException {
+ systemGroupBackend = new SystemGroupBackend(cfg);
+
+ limitsPerAccount =
+ CacheBuilder.newBuilder()
+ .build(new Module.HolderCacheLoaderByAccountId(Type.UPLOADPACK, userFactory, finder));
+ limitsPerAccount.put(accountId, holder);
+
+ limitsPerRemoteHost =
+ CacheBuilder.newBuilder()
+ .build(
+ new Module.HolderCacheLoaderByRemoteHost(
+ Type.UPLOADPACK, systemGroupBackend, finder));
+ limitsPerRemoteHost.put(REMOTE_HOST, holder);
+
restReqFilter =
spy(
new RestApiRateLimiter(
@@ -79,19 +106,11 @@
private void setUpRegisteredUser() throws ExecutionException {
when(currentUser.isIdentifiedUser()).thenReturn(true);
when(currentUser.asIdentifiedUser().getAccountId()).thenReturn(accountId);
- when(limitsPerAccount.get(accountId)).thenReturn(holder);
- }
-
- private void setUpRegisteredUserExecutionException() throws ExecutionException {
- when(currentUser.isIdentifiedUser()).thenReturn(true);
- when(currentUser.asIdentifiedUser().getAccountId()).thenReturn(accountId);
- when(limitsPerAccount.get(accountId)).thenThrow(new ExecutionException(null));
}
private void setUpAnonymous() throws ExecutionException {
when(currentUser.isIdentifiedUser()).thenReturn(false);
- when(req.getRemoteHost()).thenReturn("host");
- when(limitsPerRemoteHost.get("host")).thenReturn(holder);
+ when(req.getRemoteHost()).thenReturn(REMOTE_HOST);
}
private void setUpNoQuotaViolation1() {
@@ -132,13 +151,6 @@
}
@Test
- public void testDoFilterCacheMiss() throws IOException, ServletException, ExecutionException {
- setUpRegisteredUserExecutionException();
- restReqFilter.doFilter(req, res, chain);
- verify(res, times(0)).sendError(eq(SC_TOO_MANY_REQUESTS), anyString());
- }
-
- @Test
public void testDoFilterAnonymQuotaViolation()
throws IOException, ServletException, ExecutionException {
setUpAnonymous();
