Skip to content

Commit

Permalink
Upgrade mockito to 3.12.4 (#79669)
Browse files Browse the repository at this point in the history
Securemock is a wrapper around Mockito that monkey patches internals of
Mockito to work with the SecurityManager. However, the library has not
been updated in several years due to the complicated nature of this
monkey patching. This has left us with an ancient version of Mockito,
missing out on updates to the library in the last half decade.

While Securemock currently works with Mockito 1.x, in 2.x an official
means of plugging into mockito was added, MockMaker. This commit removes
securemock as a dependnecy of the test framework, replacing it with a
modern version of Mockito, and implementing a MockMaker that integrates
with SecurityManager.

Note that while there is a newer version of Mockito available, 4.0, it
has several deprecations removed that are used throughout Elasticsearch.
Those can be addressed in followups, and then a subsequent upgrade to
4.0 should be possible.

relates #79567
closes #40334
  • Loading branch information
rjernst committed Oct 24, 2021
1 parent eee4317 commit d9299e0
Show file tree
Hide file tree
Showing 87 changed files with 493 additions and 294 deletions.
1 change: 0 additions & 1 deletion build-tools-internal/version.properties
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ httpasyncclient = 4.1.4
commonslogging = 1.1.3
commonscodec = 1.14
hamcrest = 2.1
securemock = 1.2
mocksocket = 1.2

# benchmark dependencies
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ public void initClients() throws IOException {

doAnswer(inv -> mockPerformRequest((Request) inv.getArguments()[0]))
.when(restClient)
.performRequest(argThat(new RequestMatcher("GET", ENDPOINT)));
.performRequest(argThat(new RequestMatcher("GET", ENDPOINT)::matches));

doAnswer(inv -> mockPerformRequestAsync(
((Request) inv.getArguments()[0]),
(ResponseListener) inv.getArguments()[1]))
.when(restClient)
.performRequestAsync(argThat(new RequestMatcher("GET", ENDPOINT)), any(ResponseListener.class));
.performRequestAsync(argThat(new RequestMatcher("GET", ENDPOINT)::matches), any(ResponseListener.class));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public static void mockGetRoot(RestClient restClient, byte[] responseBody, boole
}

when(restClient
.performRequestAsync(argThat(new RequestMatcher("GET", "/")), any()))
.performRequestAsync(argThat(new RequestMatcher("GET", "/")::matches), any()))
.thenAnswer(i -> {
((ResponseListener)i.getArguments()[1]).onSuccess(response);
return Cancellable.NO_OP;
Expand Down Expand Up @@ -1035,7 +1035,8 @@ private static void doTestProductCompatibilityCheck(

Build build = new Build(Build.Flavor.DEFAULT, Build.Type.UNKNOWN, "hash", "date", false, version);
mockGetRoot(restClient, build, setProductHeader);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")))).thenReturn(apiResponse);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches)))
.thenReturn(apiResponse);

RestHighLevelClient highLevelClient = new RestHighLevelClient(restClient, RestClient::close, Collections.emptyList());

Expand Down Expand Up @@ -1082,7 +1083,8 @@ public void testProductCompatibilityTagline() throws Exception {
when(apiStatus.getStatusCode()).thenReturn(200);
Response apiResponse = mock(Response.class);
when(apiResponse.getStatusLine()).thenReturn(apiStatus);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")))).thenReturn(apiResponse);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches)))
.thenReturn(apiResponse);

RestHighLevelClient highLevelClient = new RestHighLevelClient(restClient, RestClient::close, Collections.emptyList());

Expand Down Expand Up @@ -1120,7 +1122,8 @@ public void testProductCompatibilityFlavor() throws Exception {
when(apiStatus.getStatusCode()).thenReturn(200);
Response apiResponse = mock(Response.class);
when(apiResponse.getStatusLine()).thenReturn(apiStatus);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")))).thenReturn(apiResponse);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches)))
.thenReturn(apiResponse);

RestHighLevelClient highLevelClient = new RestHighLevelClient(restClient, RestClient::close, Collections.emptyList());

Expand Down Expand Up @@ -1162,10 +1165,11 @@ public void testProductCompatibilityRequestFailure() throws Exception {
when(apiStatus.getStatusCode()).thenReturn(200);
Response apiResponse = mock(Response.class);
when(apiResponse.getStatusLine()).thenReturn(apiStatus);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")))).thenReturn(apiResponse);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches)))
.thenReturn(apiResponse);

// Have the verification request fail
when(restClient.performRequestAsync(argThat(new RequestMatcher("GET", "/")), any()))
when(restClient.performRequestAsync(argThat(new RequestMatcher("GET", "/")::matches), any()))
.thenAnswer(i -> {
((ResponseListener)i.getArguments()[1]).onFailure(new IOException("Something bad happened"));
return Cancellable.NO_OP;
Expand Down Expand Up @@ -1194,10 +1198,11 @@ public void testProductCompatibilityWithForbiddenInfoEndpoint() throws Exception
when(apiStatus.getStatusCode()).thenReturn(200);
Response apiResponse = mock(Response.class);
when(apiResponse.getStatusLine()).thenReturn(apiStatus);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")))).thenReturn(apiResponse);
when(restClient.performRequest(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches)))
.thenReturn(apiResponse);

// Have the info endpoint used for verification return a 403 (forbidden)
when(restClient.performRequestAsync(argThat(new RequestMatcher("GET", "/")), any()))
when(restClient.performRequestAsync(argThat(new RequestMatcher("GET", "/")::matches), any()))
.thenAnswer(i -> {
StatusLine infoStatus = mock(StatusLine.class);
when(apiStatus.getStatusCode()).thenReturn(HttpStatus.SC_FORBIDDEN);
Expand All @@ -1220,7 +1225,8 @@ public void testCancellationForwarding() throws Exception {

mockGetRoot(restClient);
Cancellable cancellable = mock(Cancellable.class);
when(restClient.performRequestAsync(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")), any())).thenReturn(cancellable);
when(restClient.performRequestAsync(argThat(new RequestMatcher("HEAD", "/foo/_source/bar")::matches), any()))
.thenReturn(cancellable);

Cancellable result = restHighLevelClient.existsSourceAsync(
new GetSourceRequest("foo", "bar"),
Expand Down
1 change: 0 additions & 1 deletion client/rest/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ dependencies {
testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
testImplementation "junit:junit:${versions.junit}"
testImplementation "org.hamcrest:hamcrest:${versions.hamcrest}"
testImplementation "org.elasticsearch:securemock:${versions.securemock}"
testImplementation "org.elasticsearch:mocksocket:${versions.mocksocket}"
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
import org.mockito.ArgumentCaptor;
import org.mockito.stubbing.Answer;

import javax.net.ssl.SSLHandshakeException;
import java.io.IOException;
import java.io.PrintWriter;
import java.io.StringWriter;
Expand All @@ -72,6 +71,7 @@
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.atomic.AtomicReference;
import javax.net.ssl.SSLHandshakeException;

import static java.util.Collections.singletonList;
import static org.elasticsearch.client.RestClientTestUtil.getAllErrorStatusCodes;
Expand All @@ -87,6 +87,7 @@
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -124,7 +125,7 @@ public void createRestClient() {
static CloseableHttpAsyncClient mockHttpClient(final ExecutorService exec) {
CloseableHttpAsyncClient httpClient = mock(CloseableHttpAsyncClient.class);
when(httpClient.<HttpResponse>execute(any(HttpAsyncRequestProducer.class), any(HttpAsyncResponseConsumer.class),
any(HttpClientContext.class), any(FutureCallback.class))).thenAnswer((Answer<Future<HttpResponse>>) invocationOnMock -> {
any(HttpClientContext.class), nullable(FutureCallback.class))).thenAnswer((Answer<Future<HttpResponse>>) invocationOnMock -> {
final HttpAsyncRequestProducer requestProducer = (HttpAsyncRequestProducer) invocationOnMock.getArguments()[0];
final FutureCallback<HttpResponse> futureCallback =
(FutureCallback<HttpResponse>) invocationOnMock.getArguments()[3];
Expand Down Expand Up @@ -202,7 +203,7 @@ public void testInternalHttpRequest() throws Exception {
for (String httpMethod : getHttpMethods()) {
HttpUriRequest expectedRequest = performRandomRequest(httpMethod);
verify(httpClient, times(++times)).<HttpResponse>execute(requestArgumentCaptor.capture(),
any(HttpAsyncResponseConsumer.class), any(HttpClientContext.class), any(FutureCallback.class));
any(HttpAsyncResponseConsumer.class), any(HttpClientContext.class), nullable(FutureCallback.class));
HttpUriRequest actualRequest = (HttpUriRequest)requestArgumentCaptor.getValue().generateRequest();
assertEquals(expectedRequest.getURI(), actualRequest.getURI());
assertEquals(expectedRequest.getClass(), actualRequest.getClass());
Expand Down
1 change: 0 additions & 1 deletion client/sniffer/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ dependencies {
testImplementation project(":client:test")
testImplementation "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
testImplementation "junit:junit:${versions.junit}"
testImplementation "org.elasticsearch:securemock:${versions.securemock}"
testImplementation "org.elasticsearch:mocksocket:${versions.mocksocket}"
}

Expand Down
8 changes: 8 additions & 0 deletions client/test/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ dependencies {
api "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
api "junit:junit:${versions.junit}"
api "org.hamcrest:hamcrest:${versions.hamcrest}"

// mockito
api 'org.mockito:mockito-core:3.12.4'
api 'net.bytebuddy:byte-buddy:1.11.13'
api 'org.objenesis:objenesis:3.2'
}

tasks.named('forbiddenApisMain').configure {
Expand All @@ -38,6 +43,9 @@ tasks.named('forbiddenApisTest').configure {
replaceSignatureFiles 'jdk-signatures'
}

// since this client implementation is going away, third party audit is pointless
tasks.named("thirdPartyAudit").configure { enabled = false }

// JarHell is part of es server, which we don't want to pull in
// TODO: Not anymore. Now in :libs:elasticsearch-core
tasks.named("jarHell").configure { enabled = false }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@

package org.elasticsearch.nio;

import org.elasticsearch.common.util.concurrent.AbstractRunnable;
import org.elasticsearch.core.CheckedRunnable;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
import org.elasticsearch.test.ESTestCase;
import org.junit.Before;
import org.mockito.ArgumentCaptor;
Expand All @@ -27,8 +27,8 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.BiConsumer;

import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyInt;
import static org.mockito.Matchers.isNull;
import static org.mockito.Matchers.same;
import static org.mockito.Mockito.doAnswer;
Expand Down Expand Up @@ -168,7 +168,7 @@ public void testSelectorTimeoutWillBeReducedIfTaskSooner() throws Exception {

public void testSelectorClosedExceptionIsNotCaughtWhileRunning() throws IOException {
boolean closedSelectorExceptionCaught = false;
when(rawSelector.select(anyInt())).thenThrow(new ClosedSelectorException());
when(rawSelector.select(anyLong())).thenThrow(new ClosedSelectorException());
try {
this.selector.singleLoop();
} catch (ClosedSelectorException e) {
Expand All @@ -181,7 +181,7 @@ public void testSelectorClosedExceptionIsNotCaughtWhileRunning() throws IOExcept
public void testIOExceptionWhileSelect() throws IOException {
IOException ioException = new IOException();

when(rawSelector.select(anyInt())).thenThrow(ioException);
when(rawSelector.select(anyLong())).thenThrow(ioException);

this.selector.singleLoop();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import org.elasticsearch.index.mapper.TestDocumentParserContext;
import org.elasticsearch.index.query.SearchExecutionContext;
import org.elasticsearch.index.query.TermQueryBuilder;
import org.elasticsearch.mock.orig.Mockito;
import org.mockito.Mockito;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
import org.elasticsearch.test.ESTestCase;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//// These are mock objects and test management that we allow test framework libs
//// to provide on our behalf. But tests themselves cannot do this stuff!

grant codeBase "${codebase.securemock}" {
grant codeBase "${codebase.mockito-core}" {
// needed to access ReflectionFactory (see below)
permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
// needed for reflection in ibm jdk
Expand All @@ -20,6 +20,27 @@ grant codeBase "${codebase.securemock}" {
// needed for spy interception, etc
permission java.lang.RuntimePermission "accessDeclaredMembers";
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
permission java.lang.RuntimePermission "getClassLoader";
};

grant codeBase "${codebase.byte-buddy}" {
permission java.lang.RuntimePermission "getClassLoader";
permission java.lang.RuntimePermission "createClassLoader";
permission java.lang.RuntimePermission "accessDeclaredMembers";
permission java.lang.RuntimePermission "net.bytebuddy.createJavaDispatcher";
permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
permission java.lang.reflect.ReflectPermission "newProxyInPackage.net.bytebuddy.utility";
permission java.lang.reflect.ReflectPermission "newProxyInPackage.net.bytebuddy.dynamic.loading";
permission java.lang.reflect.ReflectPermission "newProxyInPackage.net.bytebuddy.description.type";
permission java.lang.reflect.ReflectPermission "newProxyInPackage.net.bytebuddy.description.method";
};

grant codeBase "${codebase.objenesis}" {
permission java.lang.RuntimePermission "reflectionFactoryAccess";
permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
permission java.lang.RuntimePermission "accessDeclaredMembers";
permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
};

grant codeBase "${codebase.lucene-test-framework}" {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public void setupAction() {
// initialize captors, which must be members to use @Capture because of generics
threadPool = mock(ThreadPool.class);
when(threadPool.executor(anyString())).thenReturn(EsExecutors.DIRECT_EXECUTOR_SERVICE);
MockitoAnnotations.initMocks(this);
MockitoAnnotations.openMocks(this);
// setup services that will be called by action
transportService = mock(TransportService.class);
clusterService = mock(ClusterService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import static org.elasticsearch.common.UUIDs.randomBase64UUID;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -122,13 +123,15 @@ public TaskManager getTaskManager() {
when(index2ShardIterator.shardId()).thenReturn(new ShardId(index2, randomInt()));

final OperationRouting operationRouting = mock(OperationRouting.class);
when(operationRouting.getShards(eq(clusterState), eq(index1.getName()), anyString(), anyString(), anyString()))
when(operationRouting.getShards(eq(clusterState), eq(index1.getName()),
anyString(), nullable(String.class), nullable(String.class)))
.thenReturn(index1ShardIterator);
when(operationRouting.shardId(eq(clusterState), eq(index1.getName()), anyString(), anyString()))
when(operationRouting.shardId(eq(clusterState), eq(index1.getName()), nullable(String.class), nullable(String.class)))
.thenReturn(new ShardId(index1, randomInt()));
when(operationRouting.getShards(eq(clusterState), eq(index2.getName()), anyString(), anyString(), anyString()))
when(operationRouting.getShards(eq(clusterState), eq(index2.getName()),
anyString(), nullable(String.class), nullable(String.class)))
.thenReturn(index2ShardIterator);
when(operationRouting.shardId(eq(clusterState), eq(index2.getName()), anyString(), anyString()))
when(operationRouting.shardId(eq(clusterState), eq(index2.getName()), nullable(String.class), nullable(String.class)))
.thenReturn(new ShardId(index2, randomInt()));

clusterService = mock(ClusterService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import static org.elasticsearch.common.UUIDs.randomBase64UUID;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -123,13 +124,15 @@ public TaskManager getTaskManager() {
when(index2ShardIterator.shardId()).thenReturn(new ShardId(index2, randomInt()));

final OperationRouting operationRouting = mock(OperationRouting.class);
when(operationRouting.getShards(eq(clusterState), eq(index1.getName()), anyString(), anyString(), anyString()))
when(operationRouting.getShards(eq(clusterState), eq(index1.getName()),
anyString(), nullable(String.class), nullable(String.class)))
.thenReturn(index1ShardIterator);
when(operationRouting.shardId(eq(clusterState), eq(index1.getName()), anyString(), anyString()))
when(operationRouting.shardId(eq(clusterState), eq(index1.getName()), nullable(String.class), nullable(String.class)))
.thenReturn(new ShardId(index1, randomInt()));
when(operationRouting.getShards(eq(clusterState), eq(index2.getName()), anyString(), anyString(), anyString()))
when(operationRouting.getShards(eq(clusterState), eq(index2.getName()),
anyString(), nullable(String.class), nullable(String.class)))
.thenReturn(index2ShardIterator);
when(operationRouting.shardId(eq(clusterState), eq(index2.getName()), anyString(), anyString()))
when(operationRouting.shardId(eq(clusterState), eq(index2.getName()), nullable(String.class), nullable(String.class)))
.thenReturn(new ShardId(index2, randomInt()));

clusterService = mock(ClusterService.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateUpdateTask;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.mock.orig.Mockito;
import org.mockito.Mockito;
import org.elasticsearch.test.ESTestCase;
import org.junit.Before;
import org.mockito.stubbing.Answer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@

import java.util.Collections;

import static org.elasticsearch.mock.orig.Mockito.never;
import static org.elasticsearch.mock.orig.Mockito.when;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.when;
import static org.elasticsearch.test.ClusterServiceUtils.createClusterService;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.elasticsearch.mock.orig.Mockito.when;
import static org.mockito.Mockito.when;
import static org.elasticsearch.test.ClusterServiceUtils.createClusterService;
import static org.hamcrest.Matchers.sameInstance;
import static org.mockito.Mockito.mock;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import java.util.Collections;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.elasticsearch.mock.orig.Mockito.when;
import static org.mockito.Mockito.when;
import static org.elasticsearch.test.ClusterServiceUtils.createClusterService;
import static org.hamcrest.Matchers.sameInstance;
import static org.mockito.Mockito.mock;
Expand Down

0 comments on commit d9299e0

Please sign in to comment.