all: Prefer mock+delegatesTo() over Mockito.spy()

Spies are really magical and easily produce unexpected results. Using them in
tests can easily yield tests that don't do what you think they do. Delegation
is much safer when possible.

Delegation doesn't work when methods `return true`, final methods, and with
restricted visibility, though. So CensusModulesTest and
MaxConnectionIdleManagerTest are left as-is.

8 files changed, 52 insertions, 43 deletions

diff --git a/core/src/test/java/io/grpc/ClientInterceptorsTest.java b/core/src/test/java/io/grpc/ClientInterceptorsTest.java
index e0b91c7e9..81a7055e3 100644
--- a/core/src/test/java/io/grpc/ClientInterceptorsTest.java
+++ b/core/src/test/java/io/grpc/ClientInterceptorsTest.java
@@ -23,11 +23,11 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.AdditionalAnswers.delegatesTo;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.isA;
 import static org.mockito.Matchers.same;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -92,7 +92,8 @@ public class ClientInterceptorsTest {
 
   @Test
   public void channelAndInterceptorCalled() {
-    ClientInterceptor interceptor = spy(new NoopInterceptor());
+    ClientInterceptor interceptor =
+        mock(ClientInterceptor.class, delegatesTo(new NoopInterceptor()));
     Channel intercepted = ClientInterceptors.intercept(channel, interceptor);
     CallOptions callOptions = CallOptions.DEFAULT;
     // First call
@@ -216,15 +217,16 @@ public class ClientInterceptorsTest {
     final CallOptions initialCallOptions = CallOptions.DEFAULT.withDeadlineAfter(100, NANOSECONDS);
     final CallOptions newCallOptions = initialCallOptions.withDeadlineAfter(300, NANOSECONDS);
     assertNotSame(initialCallOptions, newCallOptions);
-    ClientInterceptor interceptor = spy(new ClientInterceptor() {
-      @Override
-      public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
-          MethodDescriptor<ReqT, RespT> method,
-          CallOptions callOptions,
-          Channel next) {
-        return next.newCall(method, newCallOptions);
-      }
-    });
+    ClientInterceptor interceptor =
+        mock(ClientInterceptor.class, delegatesTo(new ClientInterceptor() {
+          @Override
+          public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
+              MethodDescriptor<ReqT, RespT> method,
+              CallOptions callOptions,
+              Channel next) {
+            return next.newCall(method, newCallOptions);
+          }
+        }));
     Channel intercepted = ClientInterceptors.intercept(channel, interceptor);
     intercepted.newCall(method, initialCallOptions);
     verify(interceptor).interceptCall(
@@ -396,7 +398,8 @@ public class ClientInterceptorsTest {
     CallOptions.Key<String> customOption = CallOptions.Key.of("custom", null);
     CallOptions callOptions = CallOptions.DEFAULT.withOption(customOption, "value");
     ArgumentCaptor<CallOptions> passedOptions = ArgumentCaptor.forClass(CallOptions.class);
-    ClientInterceptor interceptor = spy(new NoopInterceptor());
+    ClientInterceptor interceptor =
+        mock(ClientInterceptor.class, delegatesTo(new NoopInterceptor()));
 
     Channel intercepted = ClientInterceptors.intercept(channel, interceptor);
 
diff --git a/core/src/test/java/io/grpc/ServerInterceptorsTest.java b/core/src/test/java/io/grpc/ServerInterceptorsTest.java
index 0b1ecce87..95b3e5b94 100644
--- a/core/src/test/java/io/grpc/ServerInterceptorsTest.java
+++ b/core/src/test/java/io/grpc/ServerInterceptorsTest.java
@@ -20,6 +20,7 @@ import static com.google.common.collect.Iterables.getOnlyElement;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.AdditionalAnswers.delegatesTo;
 import static org.mockito.Matchers.same;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -119,7 +120,8 @@ public class ServerInterceptorsTest {
 
   @Test
   public void multipleInvocationsOfHandler() {
-    ServerInterceptor interceptor = Mockito.spy(new NoopInterceptor());
+    ServerInterceptor interceptor =
+        mock(ServerInterceptor.class, delegatesTo(new NoopInterceptor()));
     ServerServiceDefinition intercepted
         = ServerInterceptors.intercept(serviceDefinition, Arrays.asList(interceptor));
     assertSame(listener,
diff --git a/examples/src/test/java/io/grpc/examples/header/HeaderClientInterceptorTest.java b/examples/src/test/java/io/grpc/examples/header/HeaderClientInterceptorTest.java
index b5e286955..0123ce961 100644
--- a/examples/src/test/java/io/grpc/examples/header/HeaderClientInterceptorTest.java
+++ b/examples/src/test/java/io/grpc/examples/header/HeaderClientInterceptorTest.java
@@ -18,7 +18,8 @@ package io.grpc.examples.header;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
-import static org.mockito.Mockito.spy;
+import static org.mockito.AdditionalAnswers.delegatesTo;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
 import io.grpc.ClientInterceptors;
@@ -59,14 +60,14 @@ public class HeaderClientInterceptorTest {
   @Rule
   public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
 
-  private final ServerInterceptor mockServerInterceptor = spy(
+  private final ServerInterceptor mockServerInterceptor = mock(ServerInterceptor.class, delegatesTo(
       new ServerInterceptor() {
         @Override
         public <ReqT, RespT> Listener<ReqT> interceptCall(
             ServerCall<ReqT, RespT> call, Metadata headers, ServerCallHandler<ReqT, RespT> next) {
           return next.startCall(call, headers);
         }
-      });
+      }));
 
   @Test
   public void clientHeaderDeliveredToServer() {
diff --git a/examples/src/test/java/io/grpc/examples/header/HeaderServerInterceptorTest.java b/examples/src/test/java/io/grpc/examples/header/HeaderServerInterceptorTest.java
index bbf222a10..64617de36 100644
--- a/examples/src/test/java/io/grpc/examples/header/HeaderServerInterceptorTest.java
+++ b/examples/src/test/java/io/grpc/examples/header/HeaderServerInterceptorTest.java
@@ -18,7 +18,8 @@ package io.grpc.examples.header;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
-import static org.mockito.Mockito.spy;
+import static org.mockito.AdditionalAnswers.delegatesTo;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
 import io.grpc.CallOptions;
@@ -87,7 +88,7 @@ public class HeaderServerInterceptorTest {
           @Override
           public void start(Listener<RespT> responseListener, Metadata headers) {
             spyListener = responseListener =
-                spy(new SimpleForwardingClientCallListener(responseListener) {});
+                mock(ClientCall.Listener.class, delegatesTo(responseListener));
             super.start(responseListener, headers);
           }
         };
diff --git a/examples/src/test/java/io/grpc/examples/helloworld/HelloWorldClientTest.java b/examples/src/test/java/io/grpc/examples/helloworld/HelloWorldClientTest.java
index b44b6f0e1..7d1237de5 100644
--- a/examples/src/test/java/io/grpc/examples/helloworld/HelloWorldClientTest.java
+++ b/examples/src/test/java/io/grpc/examples/helloworld/HelloWorldClientTest.java
@@ -17,7 +17,8 @@
 package io.grpc.examples.helloworld;
 
 import static org.junit.Assert.assertEquals;
-import static org.mockito.Mockito.spy;
+import static org.mockito.AdditionalAnswers.delegatesTo;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
 import io.grpc.stub.StreamObserver;
@@ -47,7 +48,8 @@ public class HelloWorldClientTest {
   @Rule
   public final GrpcServerRule grpcServerRule = new GrpcServerRule().directExecutor();
 
-  private final GreeterGrpc.GreeterImplBase serviceImpl = spy(new GreeterGrpc.GreeterImplBase() {});
+  private final GreeterGrpc.GreeterImplBase serviceImpl =
+      mock(GreeterGrpc.GreeterImplBase.class, delegatesTo(new GreeterGrpc.GreeterImplBase() {}));
   private HelloWorldClient client;
 
   @Before
diff --git a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
index 41c081a1c..9d8bc4f15 100644
--- a/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
+++ b/grpclb/src/test/java/io/grpc/grpclb/GrpclbLoadBalancerTest.java
@@ -29,6 +29,7 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.AdditionalAnswers.delegatesTo;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Matchers.same;
@@ -37,7 +38,6 @@ import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -165,24 +165,25 @@ public class GrpclbLoadBalancerTest {
         .thenReturn(pickFirstBalancer);
     when(roundRobinBalancerFactory.newLoadBalancer(any(Helper.class)))
         .thenReturn(roundRobinBalancer);
-    mockLbService = spy(new LoadBalancerGrpc.LoadBalancerImplBase() {
-        @Override
-        public StreamObserver<LoadBalanceRequest> balanceLoad(
-            final StreamObserver<LoadBalanceResponse> responseObserver) {
-          StreamObserver<LoadBalanceRequest> requestObserver =
-              mock(StreamObserver.class);
-          Answer<Void> closeRpc = new Answer<Void>() {
-              @Override
-              public Void answer(InvocationOnMock invocation) {
-                responseObserver.onCompleted();
-                return null;
-              }
-            };
-          doAnswer(closeRpc).when(requestObserver).onCompleted();
-          lbRequestObservers.add(requestObserver);
-          return requestObserver;
-        }
-      });
+    mockLbService = mock(LoadBalancerGrpc.LoadBalancerImplBase.class, delegatesTo(
+        new LoadBalancerGrpc.LoadBalancerImplBase() {
+          @Override
+          public StreamObserver<LoadBalanceRequest> balanceLoad(
+              final StreamObserver<LoadBalanceResponse> responseObserver) {
+            StreamObserver<LoadBalanceRequest> requestObserver =
+                mock(StreamObserver.class);
+            Answer<Void> closeRpc = new Answer<Void>() {
+                @Override
+                public Void answer(InvocationOnMock invocation) {
+                  responseObserver.onCompleted();
+                  return null;
+                }
+              };
+            doAnswer(closeRpc).when(requestObserver).onCompleted();
+            lbRequestObservers.add(requestObserver);
+            return requestObserver;
+          }
+        }));
     fakeLbServer = InProcessServerBuilder.forName("fakeLb")
         .directExecutor().addService(mockLbService).build().start();
     doAnswer(new Answer<ManagedChannel>() {
diff --git a/netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java b/netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java
index 70206b6ba..9c938773a 100644
--- a/netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java
+++ b/netty/src/test/java/io/grpc/netty/NettyHandlerTestBase.java
@@ -25,7 +25,6 @@ import static org.mockito.Matchers.anyLong;
 import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -114,7 +113,7 @@ public abstract class NettyHandlerTestBase<T extends Http2ConnectionHandler> {
    */
   protected final void initChannel(Http2HeadersDecoder headersDecoder) throws Exception {
     content = Unpooled.copiedBuffer("hello world", UTF_8);
-    frameWriter = spy(new DefaultHttp2FrameWriter());
+    frameWriter = mock(Http2FrameWriter.class, delegatesTo(new DefaultHttp2FrameWriter()));
     frameReader = new DefaultHttp2FrameReader(headersDecoder);
 
     handler = newHandler();
diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
index 5303c837f..9a2f84b38 100644
--- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
+++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
@@ -45,7 +45,6 @@ import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
@@ -121,7 +120,8 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
   @Mock
   private ServerStreamTracer.Factory streamTracerFactory;
 
-  private final ServerTransportListener transportListener = spy(new ServerTransportListenerImpl());
+  private final ServerTransportListener transportListener =
+      mock(ServerTransportListener.class, delegatesTo(new ServerTransportListenerImpl()));
   private final TestServerStreamTracer streamTracer = new TestServerStreamTracer();
 
   private NettyServerStream stream;