Improving Unit Test Coverage (firebase#100)

* Prototyping Firestore integration for Java

* Some code cleanup

* Updating comments

* Code cleanup

* Removed getInstance() methods from FirestoreClient and added getFirestore() methods.

* Minor cleanup

* Adding Firestore OAuth scopes

* Some documentation updates

* Reverted a doc change

* Updating GCS dependency version (firebase#89)

* Netty-based websocket impl

* Refactored the Netty stuff to a separate util class

* More code clean up and removing tubesock package

* Using the SDK ThreadManager for DB websocket threads

* More code cleanup and error checks

* Handling connection drop events

* Handling connection loss events

* Adding documentation

* Removed string list reader

* Removing redundant whitespace

* Revert "Using the SDK ThreadManager for DB websocket threads"

This reverts commit 8e90610.

* src/main/java/com/google/firebase/database/connection/util/StringListReader.java

* Added StringListReader back

* Re-updated websocket client

* reverting more syntactic changes

* Adding new line

* Added documentation; Using a more secure trust manager; Other cleanup

* Removed unused import

* Using the port on URI when available

* Added jacoco plugin and parser

* Coverage reporter impl

* More unit tests and cleaning up dead code

* More tests for RTDB

* More database tests

* Some unit tests for Repo

* More tests for Repo

* Cleaning up mocks

* debug info

* More debug info

* Updated repo test

* Cleaned up test code

* Removed some code changes

* Connection tests

* More unit tests

* Updated test

* More unit tests for PersistentConnectionImpl

* Refactored persistent conn tests

* More connection tests

* Some user management and DB Node tests

* StorageClient unit tests

* cleaned up PersistentConnectionTest

* Fixed typo

* Removing websocket commpression handler

* Unit tests for Connection class

* More tests for the connection package

* Unit tests for GaeExecutorService

* Updated test

* Fixing a lint error

* Added more tests

* More test coverage

* Refactored GaeExecutorService class

* Refactored various connection factory interfaces

* Minor refactoring

* Refactoring iterative test case with a map

Author: hiranya911

src/main/java/com/google/firebase/cloud/StorageClient.java

@@ -22,6 +22,7 @@
 import com.google.cloud.storage.Bucket;
 import com.google.cloud.storage.Storage;
 import com.google.cloud.storage.StorageOptions;
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import com.google.firebase.FirebaseApp;
 import com.google.firebase.ImplFirebaseTrampolines;
@@ -41,12 +42,10 @@ public class StorageClient {
   private final FirebaseApp app;
   private final Storage storage;
 
-  private StorageClient(FirebaseApp app) {
+  @VisibleForTesting
+  StorageClient(FirebaseApp app, Storage storage) {
     this.app = checkNotNull(app, "FirebaseApp must not be null");
-    this.storage = StorageOptions.newBuilder()
-        .setCredentials(ImplFirebaseTrampolines.getCredentials(app))
-        .build()
-        .getService();
+    this.storage = checkNotNull(storage, "Storage must not be null");
   }
 
   public static StorageClient getInstance() {
@@ -57,7 +56,12 @@ public static synchronized StorageClient getInstance(FirebaseApp app) {
     StorageClientService service = ImplFirebaseTrampolines.getService(app, SERVICE_ID,
         StorageClientService.class);
     if (service == null) {
-      service = ImplFirebaseTrampolines.addService(app, new StorageClientService(app));
+      Storage storage = StorageOptions.newBuilder()
+          .setCredentials(ImplFirebaseTrampolines.getCredentials(app))
+          .build()
+          .getService();
+      StorageClient client = new StorageClient(app, storage);
+      service = ImplFirebaseTrampolines.addService(app, new StorageClientService(client));
     }
     return service.getInstance();
   }
@@ -99,8 +103,8 @@ public Bucket bucket(String name) {
 
   private static class StorageClientService extends FirebaseService<StorageClient> {
 
-    StorageClientService(FirebaseApp app) {
-      super(SERVICE_ID, new StorageClient(app));
+    StorageClientService(StorageClient client) {
+      super(SERVICE_ID, client);
     }
 
     @Override

src/main/java/com/google/firebase/database/connection/Connection.java

@@ -16,6 +16,7 @@
 
 package com.google.firebase.database.connection;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.firebase.database.logging.LogWrapper;
 
 import java.util.HashMap;
@@ -39,24 +40,36 @@ class Connection implements WebsocketConnection.Delegate {
   private static final String SERVER_HELLO_HOST = "h";
   private static final String SERVER_HELLO_SESSION_ID = "s";
   private static long connectionIds = 0;
+
   private final LogWrapper logger;
-  private HostInfo hostInfo;
+  private final HostInfo hostInfo;
+  private final Delegate delegate;
+
   private WebsocketConnection conn;
-  private Delegate delegate;
   private State state;
 
-  public Connection(
+  Connection(
       ConnectionContext context,
       HostInfo hostInfo,
       String cachedHost,
       Delegate delegate,
       String optLastSessionId) {
+    this(context, hostInfo, delegate,
+        new DefaultWebsocketConnectionFactory(context, hostInfo, cachedHost, optLastSessionId));
+  }
+
+  @VisibleForTesting
+  Connection(
+      ConnectionContext context,
+      HostInfo hostInfo,
+      Delegate delegate,
+      WebsocketConnectionFactory connFactory) {
     long connId = connectionIds++;
     this.hostInfo = hostInfo;
     this.delegate = delegate;
     this.logger = new LogWrapper(context.getLogger(), Connection.class, "conn_" + connId);
     this.state = State.REALTIME_CONNECTING;
-    this.conn = new WebsocketConnection(context, hostInfo, cachedHost, this, optLastSessionId);
+    this.conn = connFactory.newConnection(this);
   }
 
   public void open() {
@@ -246,11 +259,6 @@ private void sendData(Map<String, Object> data, boolean isSensitive) {
     }
   }
 
-  // For testing
-  public void injectConnectionFailure() {
-    this.close();
-  }
-
   public enum DisconnectReason {
     SERVER_RESET,
     OTHER
@@ -262,6 +270,34 @@ private enum State {
     REALTIME_DISCONNECTED
   }
 
+  interface WebsocketConnectionFactory {
+    WebsocketConnection newConnection(WebsocketConnection.Delegate delegate);
+  }
+
+  private static class DefaultWebsocketConnectionFactory implements WebsocketConnectionFactory {
+
+    final ConnectionContext context;
+    final HostInfo hostInfo;
+    final String cachedHost;
+    final String optLastSessionId;
+
+    DefaultWebsocketConnectionFactory(
+        ConnectionContext context,
+        HostInfo hostInfo,
+        String cachedHost,
+        String optLastSessionId) {
+      this.context = context;
+      this.hostInfo = hostInfo;
+      this.cachedHost = cachedHost;
+      this.optLastSessionId = optLastSessionId;
+    }
+
+    @Override
+    public WebsocketConnection newConnection(WebsocketConnection.Delegate delegate) {
+      return new WebsocketConnection(context, hostInfo, cachedHost, delegate, optLastSessionId);
+    }
+  }
+
   public interface Delegate {
 
     void onCacheHost(String host);

src/main/java/com/google/firebase/database/connection/PersistentConnectionImpl.java

@@ -20,7 +20,6 @@
 
 import com.google.firebase.database.connection.util.RetryHelper;
 import com.google.firebase.database.logging.LogWrapper;
-import com.google.firebase.database.util.AndroidSupport;
 import com.google.firebase.database.util.GAuthToken;
 
 import java.util.ArrayList;
@@ -93,13 +92,16 @@ public class PersistentConnectionImpl implements Connection.Delegate, Persistent
   private static final String IDLE_INTERRUPT_REASON = "connection_idle";
   private static final String TOKEN_REFRESH_INTERRUPT_REASON = "token_refresh";
   private static long connectionIds = 0;
+
   private final Delegate delegate;
   private final HostInfo hostInfo;
   private final ConnectionContext context;
+  private final ConnectionFactory connFactory;
   private final ConnectionAuthTokenProvider authTokenProvider;
   private final ScheduledExecutorService executorService;
   private final LogWrapper logger;
   private final RetryHelper retryHelper;
+
   private String cachedHost;
   private HashSet<String> interruptReasons = new HashSet<>();
   private boolean firstConnection = true;
@@ -123,13 +125,19 @@ public class PersistentConnectionImpl implements Connection.Delegate, Persistent
   private long lastWriteTimestamp;
   private boolean hasOnDisconnects;
 
-  public PersistentConnectionImpl(
-      ConnectionContext context, HostInfo info, final Delegate delegate) {
-    this.delegate = delegate;
+  public PersistentConnectionImpl(ConnectionContext context, HostInfo info, Delegate delegate) {
+    this(context, info, delegate, new DefaultConnectionFactory());
+  }
+
+  PersistentConnectionImpl(
+      ConnectionContext context, HostInfo info, Delegate delegate, ConnectionFactory connFactory) {
+
     this.context = context;
+    this.hostInfo = info;
+    this.delegate = delegate;
+    this.connFactory = connFactory;
     this.executorService = context.getExecutorService();
     this.authTokenProvider = context.getAuthTokenProvider();
-    this.hostInfo = info;
     this.listens = new HashMap<>();
     this.requestCBHash = new HashMap<>();
     this.outstandingPuts = new HashMap<>();
@@ -422,7 +430,7 @@ public boolean isInterrupted(String reason) {
     return interruptReasons.contains(reason);
   }
 
-  boolean shouldReconnect() {
+  private boolean shouldReconnect() {
     return interruptReasons.size() == 0;
   }
 
@@ -524,20 +532,19 @@ public void onError(String error) {
     }
   }
 
-  public void openNetworkConnection(String token) {
+  private void openNetworkConnection(String token) {
     hardAssert(
-        this.connectionState == ConnectionState.GettingToken,
+        connectionState == ConnectionState.GettingToken,
         "Trying to open network connection while in the wrong state: %s",
-        this.connectionState);
+        connectionState);
     // User might have logged out. Positive auth status is handled after authenticating with
     // the server
     if (token == null) {
-      this.delegate.onAuthStatus(false);
+      delegate.onAuthStatus(false);
     }
-    this.authToken = token;
-    this.connectionState = ConnectionState.Connecting;
-    realtime =
-        new Connection(this.context, this.hostInfo, this.cachedHost, this, this.lastSessionId);
+    authToken = token;
+    connectionState = ConnectionState.Connecting;
+    realtime = connFactory.newConnection(this);
     realtime.open();
   }
 
@@ -1054,17 +1061,9 @@ private void warnOnListenerWarnings(List<String> warnings, ListenQuerySpec query
 
   private void sendConnectStats() {
     Map<String, Integer> stats = new HashMap<>();
-    if (AndroidSupport.isAndroid()) {
-      if (this.context.isPersistenceEnabled()) {
-        stats.put("persistence.android.enabled", 1);
-      }
-      stats.put("sdk.android." + context.getClientSdkVersion().replace('.', '-'), 1);
-      // TODO: Also send stats for connection version
-    } else {
-      assert !this.context.isPersistenceEnabled()
-          : "Stats for persistence on JVM missing (persistence not yet supported)";
-      stats.put("sdk.admin_java." + context.getClientSdkVersion().replace('.', '-'), 1);
-    }
+    assert !this.context.isPersistenceEnabled()
+        : "Stats for persistence on JVM missing (persistence not yet supported)";
+    stats.put("sdk.admin_java." + context.getClientSdkVersion().replace('.', '-'), 1);
     if (logger.logsDebug()) {
       logger.debug("Sending first connection stats");
     }
@@ -1137,13 +1136,6 @@ private boolean idleHasTimedOut() {
     return isIdle() && now > (this.lastWriteTimestamp + IDLE_TIMEOUT);
   }
 
-  // For testing
-  public void injectConnectionFailure() {
-    if (this.realtime != null) {
-      this.realtime.injectConnectionFailure();
-    }
-  }
-
   private enum ConnectionState {
     Disconnected,
     GettingToken,
@@ -1215,15 +1207,15 @@ private OutstandingListen(
       this.tag = tag;
     }
 
-    public ListenQuerySpec getQuery() {
+    ListenQuerySpec getQuery() {
       return query;
     }
 
-    public Long getTag() {
+    Long getTag() {
       return this.tag;
     }
 
-    public ListenHashProvider getHashFunction() {
+    ListenHashProvider getHashFunction() {
       return this.hashFunction;
     }
 
@@ -1247,23 +1239,23 @@ private OutstandingPut(
       this.onComplete = onComplete;
     }
 
-    public String getAction() {
+    String getAction() {
       return action;
     }
 
-    public Map<String, Object> getRequest() {
+    Map<String, Object> getRequest() {
       return request;
     }
 
-    public RequestResultCallback getOnComplete() {
+    RequestResultCallback getOnComplete() {
       return onComplete;
     }
 
-    public void markSent() {
+    void markSent() {
       this.sent = true;
     }
 
-    public boolean wasSent() {
+    boolean wasSent() {
       return this.sent;
     }
   }
@@ -1299,4 +1291,18 @@ public RequestResultCallback getOnComplete() {
       return onComplete;
     }
   }
+
+  interface ConnectionFactory {
+    Connection newConnection(PersistentConnectionImpl delegate);
+  }
+
+  private static class DefaultConnectionFactory implements ConnectionFactory {
+    @Override
+    public Connection newConnection(PersistentConnectionImpl delegate) {
+      return new Connection(delegate.context, delegate.hostInfo, delegate.cachedHost,
+          delegate, delegate.lastSessionId);
+    }
+  }
+
+
 }