core: Simplify ChannelImpl constructor; testable Builder

Add some example tests for easier fields in AbstractManagedChannelImplBuilder.

Many fields are no longer Nullable, in order to move logic from construction to
mutation, which eases testing and simplifies cross-class interactions.

The nameResolverFactory comment starting "Avoid loading the provider unless
necessary" was outdated and has not been a concern since #2071 which swapped to
a hard-coded list on Android.

core/src/main/java/io/grpc/internal/AbstractManagedChannelImplBuilder.java

@@ -16,7 +16,6 @@
 
 package io.grpc.internal;
 
-import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -75,34 +74,45 @@ public abstract class AbstractManagedChannelImplBuilder
   @VisibleForTesting
   static final long IDLE_MODE_MIN_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(1);
 
-  @Nullable
-  private Executor executor;
+  private static final ObjectPool<? extends Executor> DEFAULT_EXECUTOR_POOL =
+      SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR);
+
+  private static final NameResolver.Factory DEFAULT_NAME_RESOLVER_FACTORY =
+      NameResolverProvider.asFactory();
+
+  private static final LoadBalancer.Factory DEFAULT_LOAD_BALANCER_FACTORY =
+      PickFirstBalancerFactory.getInstance();
+
+  private static final DecompressorRegistry DEFAULT_DECOMPRESSOR_REGISTRY =
+      DecompressorRegistry.getDefaultInstance();
+
+  private static final CompressorRegistry DEFAULT_COMPRESSOR_REGISTRY =
+      CompressorRegistry.getDefaultInstance();
+
+  ObjectPool<? extends Executor> executorPool = DEFAULT_EXECUTOR_POOL;
 
   private final List<ClientInterceptor> interceptors = new ArrayList<ClientInterceptor>();
 
-  private final String target;
+  final String target;
 
   @Nullable
   private final SocketAddress directServerAddress;
 
   @Nullable
-  private String userAgent;
+  String userAgent;
 
   @Nullable
-  private String authorityOverride;
+  String authorityOverride;
 
-  @Nullable
-  private NameResolver.Factory nameResolverFactory;
+  NameResolver.Factory nameResolverFactory = DEFAULT_NAME_RESOLVER_FACTORY;
 
-  private LoadBalancer.Factory loadBalancerFactory;
+  LoadBalancer.Factory loadBalancerFactory = DEFAULT_LOAD_BALANCER_FACTORY;
 
-  @Nullable
-  private DecompressorRegistry decompressorRegistry;
+  DecompressorRegistry decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY;
 
-  @Nullable
-  private CompressorRegistry compressorRegistry;
+  CompressorRegistry compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY;
 
-  private long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS;
+  long idleTimeoutMillis = IDLE_MODE_DEFAULT_TIMEOUT_MILLIS;
 
   private int maxInboundMessageSize = GrpcUtil.DEFAULT_MAX_MESSAGE_SIZE;
 
@@ -162,7 +172,11 @@ public abstract class AbstractManagedChannelImplBuilder
 
   @Override
   public final T executor(Executor executor) {
-    this.executor = executor;
+    if (executor != null) {
+      this.executorPool = new FixedObjectPool<Executor>(executor);
+    } else {
+      this.executorPool = DEFAULT_EXECUTOR_POOL;
+    }
     return thisT();
   }
 
@@ -182,7 +196,11 @@ public abstract class AbstractManagedChannelImplBuilder
     Preconditions.checkState(directServerAddress == null,
         "directServerAddress is set (%s), which forbids the use of NameResolverFactory",
         directServerAddress);
+    if (resolverFactory != null) {
       this.nameResolverFactory = resolverFactory;
+    } else {
+      this.nameResolverFactory = DEFAULT_NAME_RESOLVER_FACTORY;
+    }
     return thisT();
   }
 
@@ -191,19 +209,31 @@ public abstract class AbstractManagedChannelImplBuilder
     Preconditions.checkState(directServerAddress == null,
         "directServerAddress is set (%s), which forbids the use of LoadBalancer.Factory",
         directServerAddress);
+    if (loadBalancerFactory != null) {
       this.loadBalancerFactory = loadBalancerFactory;
+    } else {
+      this.loadBalancerFactory = DEFAULT_LOAD_BALANCER_FACTORY;
+    }
     return thisT();
   }
 
   @Override
   public final T decompressorRegistry(DecompressorRegistry registry) {
+    if (registry != null) {
       this.decompressorRegistry = registry;
+    } else {
+      this.decompressorRegistry = DEFAULT_DECOMPRESSOR_REGISTRY;
+    } 
     return thisT();
   }
 
   @Override
   public final T compressorRegistry(CompressorRegistry registry) {
+    if (registry != null) {
       this.compressorRegistry = registry;
+    } else {
+      this.compressorRegistry = DEFAULT_COMPRESSOR_REGISTRY;
+    }
     return thisT();
   }
 
@@ -287,19 +317,18 @@ public abstract class AbstractManagedChannelImplBuilder
 
   @Override
   public ManagedChannel build() {
-    ClientTransportFactory transportFactory = buildTransportFactory();
-    NameResolver.Factory nameResolverFactory = this.nameResolverFactory;
-    if (nameResolverFactory == null) {
-      // Avoid loading the provider unless necessary, as a way to workaround a possibly-costly
-      // and poorly optimized getResource() call on Android. If any other piece of code calls
-      // getResource(), then this shouldn't be a problem unless called on the UI thread.
-      nameResolverFactory = NameResolverProvider.asFactory();
-    }
-    if (authorityOverride != null) {
-      nameResolverFactory =
-          new OverrideAuthorityNameResolverFactory(nameResolverFactory, authorityOverride);
+    return new ManagedChannelImpl(
+        this,
+        buildTransportFactory(),
+        // TODO(carl-mastrangelo): Allow clients to pass this in
+        new ExponentialBackoffPolicy.Provider(),
+        SharedResourcePool.forResource(GrpcUtil.TIMER_SERVICE),
+        SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR),
+        GrpcUtil.STOPWATCH_SUPPLIER,
+        getEffectiveInterceptors());
   }
 
+  private List<ClientInterceptor> getEffectiveInterceptors() {
     List<ClientInterceptor> effectiveInterceptors =
         new ArrayList<ClientInterceptor>(this.interceptors);
     if (recordsStats()) {
@@ -319,24 +348,7 @@ public abstract class AbstractManagedChannelImplBuilder
           new CensusTracingModule(Tracing.getTracer(), Tracing.getBinaryPropagationHandler());
       effectiveInterceptors.add(0, censusTracing.getClientInterceptor());
     }
-
-    return new ManagedChannelImpl(
-        target,
-        // TODO(carl-mastrangelo): Allow clients to pass this in
-        new ExponentialBackoffPolicy.Provider(),
-        nameResolverFactory,
-        getNameResolverParams(),
-        firstNonNull(loadBalancerFactory, PickFirstBalancerFactory.getInstance()),
-        transportFactory,
-        firstNonNull(decompressorRegistry, DecompressorRegistry.getDefaultInstance()),
-        firstNonNull(compressorRegistry, CompressorRegistry.getDefaultInstance()),
-        SharedResourcePool.forResource(GrpcUtil.TIMER_SERVICE),
-        getExecutorPool(executor),
-        SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR),
-        GrpcUtil.STOPWATCH_SUPPLIER,
-        idleTimeoutMillis,
-        userAgent,
-        effectiveInterceptors);
+    return effectiveInterceptors;
   }
 
   /**
@@ -355,24 +367,6 @@ public abstract class AbstractManagedChannelImplBuilder
     return Attributes.EMPTY;
   }
 
-  private static ObjectPool<? extends Executor> getExecutorPool(final @Nullable Executor executor) {
-    if (executor != null) {
-      return new ObjectPool<Executor>() {
-        @Override
-        public Executor getObject() {
-          return executor;
-        }
-
-        @Override
-        public Executor returnObject(Object returned) {
-          return null;
-        }
-      };
-    } else {
-      return SharedResourcePool.forResource(GrpcUtil.SHARED_CHANNEL_EXECUTOR);
-    }
-  }
-
   private static class DirectAddressNameResolverFactory extends NameResolver.Factory {
     final SocketAddress address;
     final String authority;
@@ -408,60 +402,6 @@ public abstract class AbstractManagedChannelImplBuilder
     }
   }
 
-  /**
-   * A wrapper class that overrides the authority of a NameResolver, while preserving all other
-   * functionality.
-   */
-  @VisibleForTesting
-  static class OverrideAuthorityNameResolverFactory extends NameResolver.Factory {
-    private final NameResolver.Factory delegate;
-    private final String authorityOverride;
-
-    /**
-     * Constructor for the {@link NameResolver.Factory}
-     *
-     * @param delegate The actual underlying factory that will produce the a {@link NameResolver}
-     * @param authorityOverride The authority that will be returned by {@link
-     *   NameResolver#getServiceAuthority()}
-     */
-    OverrideAuthorityNameResolverFactory(NameResolver.Factory delegate,
-        String authorityOverride) {
-      this.delegate = delegate;
-      this.authorityOverride = authorityOverride;
-    }
-
-    @Nullable
-    @Override
-    public NameResolver newNameResolver(URI targetUri, Attributes params) {
-      final NameResolver resolver = delegate.newNameResolver(targetUri, params);
-      // Do not wrap null values. We do not want to impede error signaling.
-      if (resolver == null) {
-        return null;
-      }
-      return new NameResolver() {
-        @Override
-        public String getServiceAuthority() {
-          return authorityOverride;
-        }
-
-        @Override
-        public void start(Listener listener) {
-          resolver.start(listener);
-        }
-
-        @Override
-        public void shutdown() {
-          resolver.shutdown();
-        }
-      };
-    }
-
-    @Override
-    public String getDefaultScheme() {
-      return delegate.getDefaultScheme();
-    }
-  }
-
   /**
    * Returns the correctly typed version of the builder.
    */

core/src/main/java/io/grpc/internal/FixedObjectPool.java

@@ -0,0 +1,40 @@
+/*
+ * Copyright 2017, gRPC Authors All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.internal;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * An object pool that always returns the same instance and does nothing when returning the object.
+ */
+final class FixedObjectPool<T> implements ObjectPool<T> {
+  private final T object;
+
+  public FixedObjectPool(T object) {
+    this.object = Preconditions.checkNotNull(object, "object");
+  }
+
+  @Override
+  public T getObject() {
+    return object;
+  }
+
+  @Override
+  public T returnObject(Object returned) {
+    return null;
+  }
+}

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -364,43 +364,49 @@ public final class ManagedChannelImpl extends ManagedChannel implements WithLogI
     }
   };
 
-  ManagedChannelImpl(String target, BackoffPolicy.Provider backoffPolicyProvider,
-      NameResolver.Factory nameResolverFactory, Attributes nameResolverParams,
-      LoadBalancer.Factory loadBalancerFactory, ClientTransportFactory transportFactory,
-      DecompressorRegistry decompressorRegistry, CompressorRegistry compressorRegistry,
+  ManagedChannelImpl(
+      AbstractManagedChannelImplBuilder<?> builder,
+      ClientTransportFactory clientTransportFactory,
+      BackoffPolicy.Provider backoffPolicyProvider,
       ObjectPool<ScheduledExecutorService> timerServicePool,
-      ObjectPool<? extends Executor> executorPool, ObjectPool<? extends Executor> oobExecutorPool,
-      Supplier<Stopwatch> stopwatchSupplier, long idleTimeoutMillis,
-      @Nullable String userAgent,
+      ObjectPool<? extends Executor> oobExecutorPool,
+      Supplier<Stopwatch> stopwatchSupplier,
       List<ClientInterceptor> interceptors) {
-    this.target = checkNotNull(target, "target");
-    this.nameResolverFactory = checkNotNull(nameResolverFactory, "nameResolverFactory");
-    this.nameResolverParams = checkNotNull(nameResolverParams, "nameResolverParams");
+    this.target = checkNotNull(builder.target, "target");
+    NameResolver.Factory tmpNameResolverFactory = builder.nameResolverFactory;
+    if (builder.authorityOverride != null) {
+      tmpNameResolverFactory = new OverrideAuthorityNameResolverFactory(
+          tmpNameResolverFactory, builder.authorityOverride);
+    }
+    this.nameResolverFactory = tmpNameResolverFactory;
+    this.nameResolverParams = checkNotNull(builder.getNameResolverParams(), "nameResolverParams");
     this.nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams);
-    this.loadBalancerFactory = checkNotNull(loadBalancerFactory, "loadBalancerFactory");
-    this.executorPool = checkNotNull(executorPool, "executorPool");
+    this.loadBalancerFactory =
+        checkNotNull(builder.loadBalancerFactory, "loadBalancerFactory");
+    this.executorPool = checkNotNull(builder.executorPool, "executorPool");
     this.oobExecutorPool = checkNotNull(oobExecutorPool, "oobExecutorPool");
     this.executor = checkNotNull(executorPool.getObject(), "executor");
     this.delayedTransport = new DelayedClientTransport(this.executor, this.channelExecutor);
     this.delayedTransport.start(delayedTransportListener);
     this.backoffPolicyProvider = backoffPolicyProvider;
     this.transportFactory =
-        new CallCredentialsApplyingTransportFactory(transportFactory, this.executor);
+        new CallCredentialsApplyingTransportFactory(clientTransportFactory, this.executor);
     this.interceptorChannel = ClientInterceptors.intercept(new RealChannel(), interceptors);
     this.timerServicePool = checkNotNull(timerServicePool, "timerServicePool");
     this.scheduledExecutor = checkNotNull(timerServicePool.getObject(), "timerService");
     this.stopwatchSupplier = checkNotNull(stopwatchSupplier, "stopwatchSupplier");
-    if (idleTimeoutMillis == IDLE_TIMEOUT_MILLIS_DISABLE) {
-      this.idleTimeoutMillis = idleTimeoutMillis;
+    if (builder.idleTimeoutMillis == IDLE_TIMEOUT_MILLIS_DISABLE) {
+      this.idleTimeoutMillis = builder.idleTimeoutMillis;
     } else {
       checkArgument(
-          idleTimeoutMillis >= AbstractManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS,
-          "invalid idleTimeoutMillis %s", idleTimeoutMillis);
-      this.idleTimeoutMillis = idleTimeoutMillis;
+          builder.idleTimeoutMillis
+              >= AbstractManagedChannelImplBuilder.IDLE_MODE_MIN_TIMEOUT_MILLIS,
+          "invalid idleTimeoutMillis %s", builder.idleTimeoutMillis);
+      this.idleTimeoutMillis = builder.idleTimeoutMillis;
     }
-    this.decompressorRegistry = checkNotNull(decompressorRegistry, "decompressorRegistry");
-    this.compressorRegistry = checkNotNull(compressorRegistry, "compressorRegistry");
-    this.userAgent = userAgent;
+    this.decompressorRegistry = checkNotNull(builder.decompressorRegistry, "decompressorRegistry");
+    this.compressorRegistry = checkNotNull(builder.compressorRegistry, "compressorRegistry");
+    this.userAgent = builder.userAgent;
 
     log.log(Level.FINE, "[{0}] Created with target {1}", new Object[] {getLogId(), target});
   }

core/src/main/java/io/grpc/internal/OverrideAuthorityNameResolverFactory.java

@@ -0,0 +1,74 @@
+/*
+ * Copyright 2017, gRPC Authors All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.internal;
+
+import io.grpc.Attributes;
+import io.grpc.NameResolver;
+import java.net.URI;
+import javax.annotation.Nullable;
+
+/**
+ * A wrapper class that overrides the authority of a NameResolver, while preserving all other
+ * functionality.
+ */
+final class OverrideAuthorityNameResolverFactory extends NameResolver.Factory {
+  private final NameResolver.Factory delegate;
+  private final String authorityOverride;
+
+  /**
+   * Constructor for the {@link NameResolver.Factory}
+   *
+   * @param delegate The actual underlying factory that will produce the a {@link NameResolver}
+   * @param authorityOverride The authority that will be returned by {@link
+   *   NameResolver#getServiceAuthority()}
+   */
+  OverrideAuthorityNameResolverFactory(NameResolver.Factory delegate, String authorityOverride) {
+    this.delegate = delegate;
+    this.authorityOverride = authorityOverride;
+  }
+
+  @Nullable
+  @Override
+  public NameResolver newNameResolver(URI targetUri, Attributes params) {
+    final NameResolver resolver = delegate.newNameResolver(targetUri, params);
+    // Do not wrap null values. We do not want to impede error signaling.
+    if (resolver == null) {
+      return null;
+    }
+    return new NameResolver() {
+      @Override
+      public String getServiceAuthority() {
+        return authorityOverride;
+      }
+
+      @Override
+      public void start(Listener listener) {
+        resolver.start(listener);
+      }
+
+      @Override
+      public void shutdown() {
+        resolver.shutdown();
+      }
+    };
+  }
+
+  @Override
+  public String getDefaultScheme() {
+    return delegate.getDefaultScheme();
+  }
+}

core/src/test/java/io/grpc/internal/AbstractManagedChannelImplBuilderTest.java

@@ -17,16 +17,24 @@
 package io.grpc.internal;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import com.google.common.util.concurrent.MoreExecutors;
 import io.grpc.Attributes;
+import io.grpc.CompressorRegistry;
+import io.grpc.DecompressorRegistry;
+import io.grpc.LoadBalancer;
 import io.grpc.NameResolver;
 import java.net.InetSocketAddress;
+import java.net.SocketAddress;
 import java.net.URI;
+import java.util.concurrent.Executor;
 import java.util.concurrent.TimeUnit;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -35,6 +43,173 @@ import org.junit.runners.JUnit4;
 /** Unit tests for {@link AbstractManagedChannelImplBuilder}. */
 @RunWith(JUnit4.class)
 public class AbstractManagedChannelImplBuilderTest {
+  private Builder builder = new Builder("fake");
+  private Builder directAddressBuilder = new Builder(new SocketAddress(){}, "fake");
+
+  @Test
+  public void executor_default() {
+    assertNotNull(builder.executorPool);
+  }
+
+  @Test
+  public void executor_normal() {
+    Executor executor = mock(Executor.class);
+    assertEquals(builder, builder.executor(executor));
+    assertEquals(executor, builder.executorPool.getObject());
+  }
+
+  @Test
+  public void executor_null() {
+    ObjectPool<? extends Executor> defaultValue = builder.executorPool;
+    builder.executor(mock(Executor.class));
+    assertEquals(builder, builder.executor(null));
+    assertEquals(defaultValue, builder.executorPool);
+  }
+
+  @Test
+  public void directExecutor() {
+    assertEquals(builder, builder.directExecutor());
+    assertEquals(MoreExecutors.directExecutor(), builder.executorPool.getObject());
+  }
+
+  @Test
+  public void nameResolverFactory_default() {
+    assertNotNull(builder.nameResolverFactory);
+  }
+
+  @Test
+  public void nameResolverFactory_normal() {
+    NameResolver.Factory nameResolverFactory = mock(NameResolver.Factory.class);
+    assertEquals(builder, builder.nameResolverFactory(nameResolverFactory));
+    assertEquals(nameResolverFactory, builder.nameResolverFactory);
+  }
+
+  @Test
+  public void nameResolverFactory_null() {
+    NameResolver.Factory defaultValue = builder.nameResolverFactory;
+    builder.nameResolverFactory(mock(NameResolver.Factory.class));
+    assertEquals(builder, builder.nameResolverFactory(null));
+    assertEquals(defaultValue, builder.nameResolverFactory);
+  }
+
+  @Test(expected = IllegalStateException.class)
+  public void nameResolverFactory_notAllowedWithDirectAddress() {
+    directAddressBuilder.nameResolverFactory(mock(NameResolver.Factory.class));
+  }
+
+  @Test
+  public void loadBalancerFactory_default() {
+    assertNotNull(builder.loadBalancerFactory);
+  }
+
+  @Test
+  public void loadBalancerFactory_normal() {
+    LoadBalancer.Factory loadBalancerFactory = mock(LoadBalancer.Factory.class);
+    assertEquals(builder, builder.loadBalancerFactory(loadBalancerFactory));
+    assertEquals(loadBalancerFactory, builder.loadBalancerFactory);
+  }
+
+  @Test
+  public void loadBalancerFactory_null() {
+    LoadBalancer.Factory defaultValue = builder.loadBalancerFactory;
+    builder.loadBalancerFactory(mock(LoadBalancer.Factory.class));
+    assertEquals(builder, builder.loadBalancerFactory(null));
+    assertEquals(defaultValue, builder.loadBalancerFactory);
+  }
+
+  @Test(expected = IllegalStateException.class)
+  public void loadBalancerFactory_notAllowedWithDirectAddress() {
+    directAddressBuilder.loadBalancerFactory(mock(LoadBalancer.Factory.class));
+  }
+
+  @Test
+  public void decompressorRegistry_default() {
+    assertNotNull(builder.decompressorRegistry);
+  }
+
+  @Test
+  public void decompressorRegistry_normal() {
+    DecompressorRegistry decompressorRegistry = DecompressorRegistry.emptyInstance();
+    assertNotEquals(decompressorRegistry, builder.decompressorRegistry);
+    assertEquals(builder, builder.decompressorRegistry(decompressorRegistry));
+    assertEquals(decompressorRegistry, builder.decompressorRegistry);
+  }
+
+  @Test
+  public void decompressorRegistry_null() {
+    DecompressorRegistry defaultValue = builder.decompressorRegistry;
+    assertEquals(builder, builder.decompressorRegistry(DecompressorRegistry.emptyInstance()));
+    assertNotEquals(defaultValue, builder.decompressorRegistry);
+    builder.decompressorRegistry(null);
+    assertEquals(defaultValue, builder.decompressorRegistry);
+  }
+
+  @Test
+  public void compressorRegistry_default() {
+    assertNotNull(builder.compressorRegistry);
+  }
+
+  @Test
+  public void compressorRegistry_normal() {
+    CompressorRegistry compressorRegistry = CompressorRegistry.newEmptyInstance();
+    assertNotEquals(compressorRegistry, builder.compressorRegistry);
+    assertEquals(builder, builder.compressorRegistry(compressorRegistry));
+    assertEquals(compressorRegistry, builder.compressorRegistry);
+  }
+
+  @Test
+  public void compressorRegistry_null() {
+    CompressorRegistry defaultValue = builder.compressorRegistry;
+    builder.compressorRegistry(CompressorRegistry.newEmptyInstance());
+    assertNotEquals(defaultValue, builder.compressorRegistry);
+    assertEquals(builder, builder.compressorRegistry(null));
+    assertEquals(defaultValue, builder.compressorRegistry);
+  }
+
+  @Test
+  public void userAgent_default() {
+    assertNull(builder.userAgent);
+  }
+
+  @Test
+  public void userAgent_normal() {
+    String userAgent = "user-agent/1";
+    assertEquals(builder, builder.userAgent(userAgent));
+    assertEquals(userAgent, builder.userAgent);
+  }
+
+  @Test
+  public void userAgent_null() {
+    assertEquals(builder, builder.userAgent(null));
+    assertNull(builder.userAgent);
+
+    builder.userAgent("user-agent/1");
+    builder.userAgent(null);
+    assertNull(builder.userAgent);
+  }
+
+  @Test
+  public void overrideAuthority_default() {
+    assertNull(builder.authorityOverride);
+  }
+
+  @Test
+  public void overrideAuthority_normal() {
+    String overrideAuthority = "best-authority";
+    assertEquals(builder, builder.overrideAuthority(overrideAuthority));
+    assertEquals(overrideAuthority, builder.authorityOverride);
+  }
+
+  @Test(expected = NullPointerException.class)
+  public void overrideAuthority_null() {
+    builder.overrideAuthority(null);
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void overrideAuthority_invalid() {
+    builder.overrideAuthority("not_allowed");
+  }
+
   @Test
   public void makeTargetStringForDirectAddress_scopedIpv6() throws Exception {
     InetSocketAddress address = new InetSocketAddress("0:0:0:0:0:0:0:0%0", 10005);
@@ -47,23 +222,7 @@ public class AbstractManagedChannelImplBuilderTest {
 
   @Test
   public void idleTimeout() {
-    class Builder extends AbstractManagedChannelImplBuilder<Builder> {
-      Builder() {
-        super("target");
-      }
-
-      @Override
-      protected ClientTransportFactory buildTransportFactory() {
-        throw new UnsupportedOperationException();
-      }
-
-      @Override
-      public Builder usePlaintext(boolean value) {
-        return this;
-      }
-    }
-
-    Builder builder = new Builder();
+    Builder builder = new Builder("target");
 
     assertEquals(AbstractManagedChannelImplBuilder.IDLE_MODE_DEFAULT_TIMEOUT_MILLIS,
         builder.getIdleTimeoutMillis());
@@ -98,8 +257,7 @@ public class AbstractManagedChannelImplBuilderTest {
       .thenReturn(nameResolverMock);
     String override = "override:5678";
     NameResolver.Factory factory =
-        new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory,
-          override);
+        new OverrideAuthorityNameResolverFactory(wrappedFactory, override);
     NameResolver nameResolver = factory.newNameResolver(URI.create("dns:///localhost:443"),
         Attributes.EMPTY);
     assertNotNull(nameResolver);
@@ -111,9 +269,28 @@ public class AbstractManagedChannelImplBuilderTest {
     NameResolver.Factory wrappedFactory = mock(NameResolver.Factory.class);
     when(wrappedFactory.newNameResolver(any(URI.class), any(Attributes.class))).thenReturn(null);
     NameResolver.Factory factory =
-        new AbstractManagedChannelImplBuilder.OverrideAuthorityNameResolverFactory(wrappedFactory,
-            "override:5678");
+        new OverrideAuthorityNameResolverFactory(wrappedFactory, "override:5678");
     assertEquals(null,
         factory.newNameResolver(URI.create("dns:///localhost:443"), Attributes.EMPTY));
   }
+
+  static class Builder extends AbstractManagedChannelImplBuilder<Builder> {
+    Builder(String target) {
+      super(target);
+    }
+
+    Builder(SocketAddress directServerAddress, String authority) {
+      super(directServerAddress, authority);
+    }
+
+    @Override
+    protected ClientTransportFactory buildTransportFactory() {
+      throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public Builder usePlaintext(boolean value) {
+      throw new UnsupportedOperationException();
+    }
+  }
 }

core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java

@@ -34,8 +34,6 @@ import io.grpc.Attributes;
 import io.grpc.CallOptions;
 import io.grpc.ClientCall;
 import io.grpc.ClientInterceptor;
-import io.grpc.CompressorRegistry;
-import io.grpc.DecompressorRegistry;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.IntegerMarshaller;
 import io.grpc.LoadBalancer;
@@ -93,10 +91,13 @@ public class ManagedChannelImplIdlenessTest {
           .build();
 
   private final List<EquivalentAddressGroup> servers = Lists.newArrayList();
+  private final ObjectPool<ScheduledExecutorService> timerServicePool =
+      new FixedObjectPool<ScheduledExecutorService>(timer.getScheduledExecutorService());
+  private final ObjectPool<Executor> executorPool =
+      new FixedObjectPool<Executor>(executor.getScheduledExecutorService());
+  private final ObjectPool<Executor> oobExecutorPool =
+      new FixedObjectPool<Executor>(oobExecutor.getScheduledExecutorService());
 
-  @Mock private ObjectPool<ScheduledExecutorService> timerServicePool;
-  @Mock private ObjectPool<Executor> executorPool;
-  @Mock private ObjectPool<Executor> oobExecutorPool;
   @Mock private ClientTransportFactory mockTransportFactory;
   @Mock private LoadBalancer mockLoadBalancer;
   @Mock private LoadBalancer.Factory mockLoadBalancerFactory;
@@ -110,20 +111,35 @@ public class ManagedChannelImplIdlenessTest {
   @Before
   public void setUp() {
     MockitoAnnotations.initMocks(this);
-    when(timerServicePool.getObject()).thenReturn(timer.getScheduledExecutorService());
-    when(executorPool.getObject()).thenReturn(executor.getScheduledExecutorService());
-    when(oobExecutorPool.getObject()).thenReturn(oobExecutor.getScheduledExecutorService());
     when(mockLoadBalancerFactory.newLoadBalancer(any(Helper.class))).thenReturn(mockLoadBalancer);
     when(mockNameResolver.getServiceAuthority()).thenReturn(AUTHORITY);
     when(mockNameResolverFactory
         .newNameResolver(any(URI.class), any(Attributes.class)))
         .thenReturn(mockNameResolver);
 
-    channel = new ManagedChannelImpl("fake://target", new FakeBackoffPolicyProvider(),
-        mockNameResolverFactory, Attributes.EMPTY, mockLoadBalancerFactory,
-        mockTransportFactory, DecompressorRegistry.getDefaultInstance(),
-        CompressorRegistry.getDefaultInstance(), timerServicePool, executorPool, oobExecutorPool,
-        timer.getStopwatchSupplier(), TimeUnit.SECONDS.toMillis(IDLE_TIMEOUT_SECONDS), USER_AGENT,
+    class Builder extends AbstractManagedChannelImplBuilder<Builder> {
+      Builder(String target) {
+        super(target);
+      }
+
+      @Override protected ClientTransportFactory buildTransportFactory() {
+        throw new UnsupportedOperationException();
+      }
+
+      @Override public Builder usePlaintext(boolean b) {
+        throw new UnsupportedOperationException();
+      }
+    }
+
+    Builder builder = new Builder("fake://target")
+        .nameResolverFactory(mockNameResolverFactory)
+        .loadBalancerFactory(mockLoadBalancerFactory)
+        .idleTimeout(IDLE_TIMEOUT_SECONDS, TimeUnit.SECONDS)
+        .userAgent(USER_AGENT);
+    builder.executorPool = executorPool;
+    channel = new ManagedChannelImpl(
+        builder, mockTransportFactory, new FakeBackoffPolicyProvider(),
+        timerServicePool, oobExecutorPool, timer.getStopwatchSupplier(),
         Collections.<ClientInterceptor>emptyList());
     newTransports = TestUtils.captureTransports(mockTransportFactory);
 

core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java

@@ -52,10 +52,8 @@ import io.grpc.Channel;
 import io.grpc.ClientCall;
 import io.grpc.ClientInterceptor;
 import io.grpc.ClientStreamTracer;
-import io.grpc.CompressorRegistry;
 import io.grpc.ConnectivityStateInfo;
 import io.grpc.Context;
-import io.grpc.DecompressorRegistry;
 import io.grpc.EquivalentAddressGroup;
 import io.grpc.IntegerMarshaller;
 import io.grpc.LoadBalancer;
@@ -171,12 +169,33 @@ public class ManagedChannelImplTest {
 
   private void createChannel(
       NameResolver.Factory nameResolverFactory, List<ClientInterceptor> interceptors) {
-    channel = new ManagedChannelImpl(target, new FakeBackoffPolicyProvider(),
-        nameResolverFactory, NAME_RESOLVER_PARAMS, mockLoadBalancerFactory,
-        mockTransportFactory, DecompressorRegistry.getDefaultInstance(),
-        CompressorRegistry.getDefaultInstance(), timerServicePool, executorPool, oobExecutorPool,
-        timer.getStopwatchSupplier(),  ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, userAgent,
-        interceptors);
+    class Builder extends AbstractManagedChannelImplBuilder<Builder> {
+      Builder(String target) {
+        super(target);
+      }
+
+      @Override protected ClientTransportFactory buildTransportFactory() {
+        throw new UnsupportedOperationException();
+      }
+
+      @Override protected Attributes getNameResolverParams() {
+        return NAME_RESOLVER_PARAMS;
+      }
+
+      @Override public Builder usePlaintext(boolean b) {
+        throw new UnsupportedOperationException();
+      }
+    }
+
+    Builder builder = new Builder(target)
+        .nameResolverFactory(nameResolverFactory)
+        .loadBalancerFactory(mockLoadBalancerFactory)
+        .userAgent(userAgent);
+    builder.executorPool = executorPool;
+    builder.idleTimeoutMillis = ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE;
+    channel = new ManagedChannelImpl(
+        builder, mockTransportFactory, new FakeBackoffPolicyProvider(),
+        timerServicePool, oobExecutorPool, timer.getStopwatchSupplier(), interceptors);
     // Force-exit the initial idle-mode
     channel.exitIdleMode();
     assertEquals(0, timer.numPendingTasks());