Improving Unit Test Coverage #100

hiranya911 · 2017-11-22T19:40:21Z

This PR adds more unit tests to cover classes like Connection, PersistentConnectionImpl and WebsocketConnection. To enable this, I also made some minor modifications to the classes in question, so they are more amenable to unit testing. Overall this brings our unit test coverage to 80%.

…tore() methods.

…ase-admin-java into hkj-firestore-java

This reverts commit 8e90610.

…Reader.java

schmidt-sebastian · 2017-12-01T18:24:58Z

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

+      HostInfo hostInfo,
+      String cachedHost,
+      Delegate delegate,
+      String optLastSessionId,


Nit: I know it's not actually optional, but should this be the last argument?

Refactored away

schmidt-sebastian · 2017-12-01T18:29:04Z

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

+        String optLastSessionId);
+  }
+
+  private static class DefaultConnectionFactory implements WebsocketConnectionFactory {


Nit: Can you come up with a more specific name for this? If WebsocketConnectionFactory wasn't already taken, I would suggest that as a name.

Renamed to DefaultWebsocketConnectionFactory

schmidt-sebastian · 2017-12-01T18:31:53Z

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

@@ -262,6 +269,28 @@ public void injectConnectionFailure() {
    REALTIME_DISCONNECTED
  }

+  interface WebsocketConnectionFactory {
+    WebsocketConnection newConnection(


Can we move some of this state to the factory instance? A lot of this shouldn't change after the initial creation.

schmidt-sebastian · 2017-12-01T18:32:58Z

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

-    realtime =
-        new Connection(this.context, this.hostInfo, this.cachedHost, this, this.lastSessionId);
-    realtime.open();
+    this.realtime = connFactory.newConnection(


I'd be more in favor of removing 'this' from the other places rather than adding it everywhere.

Removed this dereferences from the method body

schmidt-sebastian · 2017-12-01T18:35:23Z

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

    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(context, hostInfo, cachedHost, this, optLastSessionId);


Can we pass in the instance directly? It looks like at least this connection never gets recreated.

We need to inject a reference to this into the created connection. Therefore, it must be done post construction.

schmidt-sebastian · 2017-12-01T18:53:24Z

src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java

+    FirebaseUserManager userManager = new FirebaseUserManager(gson, transport, credentials);
+    try {
+      userManager.getUserByEmail("[email protected]");
+      fail("No error thrown for invalid response");


Does the junit version you are using offer assertThrows()?

We are still on junit4. Lets upgrade to junit5 in a separate PR.

schmidt-sebastian · 2017-12-01T18:54:42Z

src/test/java/com/google/firebase/auth/FirebaseUserManagerTest.java

@@ -153,6 +185,74 @@ public void testDeleteUser() throws Exception {

  @Test
  public void testGetUserHttpError() throws Exception {
+    UserManagerOp[] operations = new UserManagerOp[]{


Can this be a map from Error Code to Callable?

There are multiple operations that return the same error code.

Is it possible to flip this map around? You could then use Map builders and remove about 50 lines in this file (No, I didn't count).

Done. (removed 33 lines :) )

schmidt-sebastian · 2017-12-01T18:55:08Z

src/test/java/com/google/firebase/cloud/StorageClientTest.java

+        .build());
+    Storage mockStorage = Mockito.mock(Storage.class);
+    StorageClient client = new StorageClient(app, mockStorage);
+    try {


Here and everywhere: Suggesting to use assertThrows.

assertThrows is a junit5 method. We are still on junit4. Lets plan to do a upgrade in a future PR.

schmidt-sebastian · 2017-12-01T19:08:38Z

src/test/java/com/google/firebase/database/core/RepoTest.java

+  public void testTransactionError() throws InterruptedException {
+    final Repo repo = newRepo();
+    final List<DataSnapshot> events = new ArrayList<>();
+    final Path path = new Path("/txn_error");


I'm gonna use my knowledge that I just learned from Testing on the Toilet:

I think these tests would be easier to follow if you set up the mocks inline.

schmidt-sebastian · 2017-12-01T19:12:35Z

src/test/java/com/google/firebase/internal/GaeExecutorServiceTest.java

+    assertTrue(executorService.isTerminated());
+  }
+
+  private static class CountingThreadFactory implements ThreadFactory {


I was hoping this was part of the JDK :/ This seems incredible useful!

It seems to be supported by BasicThreadFactory of commons-lang. But we are currently not depending on it.

hiranya911 · 2017-12-04T20:07:25Z

I've made all the changes except for the use of generics for different factory types. I've left a follow up question regarding that.

hiranya911 added 30 commits August 31, 2017 16:35

Prototyping Firestore integration for Java

705abcd

Some code cleanup

084453e

Updating comments

39dec4c

Code cleanup

cd8030c

Merge branch 'master' into hkj-firestore-java

9be05c9

Removed getInstance() methods from FirestoreClient and added getFires…

dfc6bec

…tore() methods.

Merge branch 'hkj-firestore-java' of github.com:FirebasePrivate/fireb…

ef3881f

…ase-admin-java into hkj-firestore-java

Merged with master

0cf020b

Minor cleanup

ab3469a

Adding Firestore OAuth scopes

d64b4f6

Merge branch 'master' into hkj-firestore-java

d87f364

Some documentation updates

8ed4e5f

Reverted a doc change

48104da

Updating GCS dependency version (#89)

1771a01

Netty-based websocket impl

4201ff1

Refactored the Netty stuff to a separate util class

7e7f694

More code clean up and removing tubesock package

7a74047

Using the SDK ThreadManager for DB websocket threads

8e90610

More code cleanup and error checks

b910380

Handling connection drop events

a227cae

Handling connection loss events

6d8d90f

Adding documentation

e276f9a

Removed string list reader

c10753d

Merged with latest master

da73c79

Merge branch 'master' into hkj-firestore-java

193fcde

Merge branch 'hkj-firestore-java' into hkj-netty-rtdb

a6613ce

Removing redundant whitespace

59d1f5a

Revert "Using the SDK ThreadManager for DB websocket threads"

e03a896

This reverts commit 8e90610.

src/main/java/com/google/firebase/database/connection/util/StringList…

b87f536

…Reader.java

Added StringListReader back

4d7c74a

hiranya911 added 12 commits November 8, 2017 15:11

Fixed typo

fd5c79e

Merged with master

3adf4a7

Removing websocket commpression handler

884bd18

Merged with hkj-netty-rtdb

931ce8a

Unit tests for Connection class

6bc58f5

More tests for the connection package

ec33e35

Unit tests for GaeExecutorService

de41e44

Updated test

c28da33

Fixing a lint error

bd3fafc

Added more tests

768ed57

More test coverage

45e4a49

Merged with master

03cbd15

hiranya911 requested a review from schmidt-sebastian November 27, 2017 22:02

hiranya911 assigned schmidt-sebastian Nov 27, 2017

schmidt-sebastian suggested changes Dec 1, 2017

View reviewed changes

hiranya911 added 2 commits December 1, 2017 13:45

Refactored GaeExecutorService class

48db4d8

Refactored various connection factory interfaces

67e0d74

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Dec 1, 2017

Minor refactoring

c3aad51

hiranya911 assigned schmidt-sebastian and unassigned hiranya911 Dec 4, 2017

schmidt-sebastian approved these changes Dec 5, 2017

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Dec 6, 2017

hiranya911 added 2 commits December 6, 2017 11:45

Merged with master

fa9addf

Refactoring iterative test case with a map

5fc783d

hiranya911 merged commit cc47045 into master Dec 6, 2017

hiranya911 deleted the hkj-conn-test branch December 6, 2017 20:06

Improving Unit Test Coverage #100

Improving Unit Test Coverage #100

Uh oh!

Conversation

hiranya911 commented Nov 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Dec 4, 2017

Uh oh!

Uh oh!

hiranya911 Dec 1, 2017 •

edited

Loading

hiranya911 Dec 1, 2017 •

edited

Loading