Porting the legacy integration test suite #2

hiranya911 · 2017-04-13T06:53:54Z

This PR ports the legacy integration test suite (most of it) to run against Firebase production. This requires a Firebase project. Create a project in Firebase console, and download the service account key file. Then run the test suite as:

mvn verify -Dfirebase.it.certificate=[path/to/serviceAccountKey.json] -Dfirebase.it.url=[DB URL]

The IntegrationTestUtils class handles reading the service account key, and making it available to all integration tests.

This PR also cleans up the test sources. In particular:

A dependency on jackson-asl has been removed.
A dependency on the legacy Firebase token generator has been removed.
TestConstants class removed.
Most of the methods in TestUtils, which were used by the old test suite has been modified, and moved to IntegrationTestUtils.
Removed the ListBuilder class.
The DeepEquals class has been removed and replaced with the DeepEquals class from cedarsoftware-utils library.
Removed the PerformanceBenchmarks and SynchronousConnection classes. We can fix and add them later if necessary.

There's a lot more that can be done to fix and improve the integration test suite, but I feel this is a great starting point since we didn't have a working test suite for a while (and definitely not one that runs against Firebase prod). I will send in more improvements in future PRs.

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

…ethod

…ndency

mikelehen

This is too big to actually review in detail, but I tried to spot-check it (especially the utils and stuff) and overall it looks good and clearly gets us into a much better state. Thanks for tackling! I left a few questions / comments.

mikelehen · 2017-04-13T19:49:11Z

src/test/java/com/google/firebase/database/TestHelpers.java

-        fail("Found error on run loop");
-      }
-    }
-  }


Can you explain why this code is going away? Have you verified that an exception on the test event target still causes tests to fail and the error to be reported nicely?

I honestly don't remember the full context, but I assume if you e.g. added a snapshot listener and the listener implementation threw an exception (e.g. because a test assertion failed), it wasn't being reliably reported in a friendly way.

This method works with a set of made up contexts and a TestEventTarget. The new integration tests uses the actual database context and the actual ThreadPoolEventTarget, and therefore this method will not work for us anymore. Also we were calling this method from tearDown events, which is kind of broken.

However, I do understand the point you're making about assertions checked in async event handlers. I've fixed a bunch of them so that the assertions are checked outside the event handlers in a blocking manner. However, I'm pretty sure there are plenty left to fix, which I will get around to eventually. The good news is however, whenever an async event handler encounters a failed assertion it throws an exception, which Maven logs to the console. I don't see any of those in the test output.

Hrm... Are you saying that right now, asynchronous test failures log an exception instead of fail the test? And the plan going forward is to rewrite them to not do asynchronous assertions? I'm not sure I'm a fan of that... It seems like a lot of work and easy to forget / miss cases. What was the motivation for undoing the existing approach?

We shouldn't be running assertions in tearDown methods. This can lead to test environment not getting cleaned up properly. Also this requires using a non-default RunLoop implementation that keeps track of errors. I'm not sure if that's a good idea, since we will not be testing the actual run loop that a typical users get.

Let me also try to figure out how often we do async assertions.

Ok, we do this kind of thing often enough, so we should implement some way to check for async errors. But this requires messing with both RunLoop and EventTarget. Specifically we need to modify them during tests to catch errors. This is going to require some work, and so I'd rather do them later.

Okay. Just keep in mind that means many test failures may not fail travis / whatever we're using for CI and thus may not be caught until later (if/when somebody happens to notice the log messages I guess). Given we're not making a lot of changes, this is probably okay. But if we start doing nontrivial development or start receiving pull requests from the community, this debt could easily bite us.

Noted. I've been experimenting with some changes, and I think I have a way to fix this properly. I will send a PR with the fix soon, once the open sourcing stuff is sufficiently under control. Thanks Michael for pointing this problem out.

mikelehen · 2017-04-13T19:50:06Z

src/test/java/com/google/firebase/database/TestHelpers.java

+      FirebaseApp.initializeApp(
+          new FirebaseOptions.Builder()
+              .setCredential(FirebaseCredentials.fromCertificate(ServiceAccount.EDITOR.asStream()))
+              .setDatabaseUrl("http://tests.fblocal.com:9000")


Don't we want to point this elsewhere?

This is now only used in unit tests (DataSnapshotTest, DefaultPersistenceManagetTest etc.). So it doesn't matter where it points to.

To be consistent, I've changed this to admin-java-sdk.firebaseio.com

mikelehen · 2017-04-13T19:51:02Z

src/test/java/com/google/firebase/database/TestHelpers.java

    TestRunLoop runLoop = new TestRunLoop();
    DatabaseConfig config = new DatabaseConfig();
-    config.setLogLevel(Logger.Level.DEBUG);
+    config.setLogLevel(Logger.Level.WARN);


I'm curious why you're changing this. It's probably fine, but the logs are often useful for debugging test failures.

The debug logs clutter up the test output. Ideally we should use a proper logging framework like JUL or SLF4J, so that the user/developer can control the log level (for both released code, and tests) externally. I'm planning to do this in the future. But for now I'd prefer to have a simple and less verbose test output as the default.

mikelehen · 2017-04-13T19:53:44Z

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

 import java.net.URI;
 import org.junit.Test;

 public class RepoInfoTest {

  @Test
  public void getConnectionURLTestOverloadWorks() {
+    final String repo = "tests";
+    final String server = "fblocal.com:9000";


I guess maybe this is okay because we're not actually starting a connection, but I think we should remove as many references to fblocal.com as possible. This is a legacy hack we used for testing against a local Firebase instance, and more problematically, fblocal.com is a real domain name and we do not own it. :-/

We could probably switch to fb.local or something instead if we have references we want to keep.

Changed to admin-java-sdk.firebaseio.com

mikelehen · 2017-04-13T19:53:53Z

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

    info.internalHost = info.host;
    info.secure = false;
-    info.namespace = TestConstants.TEST_REPO;
+    info.namespace = repo;
    URI url = info.getConnectionURL(null);
    assertEquals("ws://tests.fblocal.com:9000/.ws?ns=tests&v=5", url.toString());


Changed to firebaseio.com URL

mikelehen · 2017-04-13T20:03:28Z

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

@@ -47,15 +46,13 @@ public void startListening(
          ListenHashProvider hash,
          SyncTree.CompletionListener onListenComplete) {
        Path path = query.getPath();
-        logger.debug("Listening at " + path + " for Tag " + tag + ")");


why remove all this logging?

This was also to prevent the default test output from getting cluttered up. But I've added it back with log level set to WARN, so it won't be as verbose. We can fix it better once the logging stuff is implemented properly.

mikelehen · 2017-04-13T20:06:05Z

src/test/java/com/google/firebase/database/core/persistence/KeepSyncedTestIT.java

+import org.junit.BeforeClass;
+import org.junit.Test;
+
+public class KeepSyncedTestIT {


What is IT?

Stands for Integration Test. Maven surefire and failsafe plugins use this naming convention to differentiate between unit tests and integration tests. I will document this in my next pull request.

hiranya911 · 2017-04-13T22:48:12Z

I've added some of the logging code back, and dropped all references to fblocal domain.

jwngr · 2017-04-14T00:29:05Z

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

 import java.net.URI;
 import org.junit.Test;

 public class RepoInfoTest {

  @Test
  public void getConnectionURLTestOverloadWorks() {
+    final String repo = "tests";
+    final String server = "admin-java-sdk.firebaseio.com:9000";


Get rid of the port in all three places in this file.

…reporting.

mikelehen · 2017-04-14T15:06:39Z

Thanks for the changes. LGTM.

jwngr

LGTM. Thanks Hiranya!

Hiranya Jayathilaka and others added 30 commits April 5, 2017 19:12

Initial code migration:

428eedb

* Reformatted all source files * Separated unit and integration tests * Added maven checkstyle plugin * Got the basic build and unit tests working * Scrubbed the codebase for private keys, certificates etc.

Reformatting code according to check-style plugin conventions

cfbbd6a

Code reformatting: Fixing javadoc indentation

59842b8

More reformatting: Added google check style config

fef4ecf

Code formatting continued

a606833

Code formatting continued

1d2ed82

Reformatting

8972b1b

Fixed all checkstyle violations; Build configured to fail on lint errors

e5509c3

Updated checkstyle config

0587d52

Working on re-enabling the integration tests

7f2f470

Temporarily removing IT test cases

8ee5cfc

Extending checkstyle coverage to test sourcesw

dbc6ee6

Cleaned up verbose test output

c66f35b

Adding 2 integration tests to verify the maven integration test support

f7cbdc2

Moving test only trampolines to the test/ subtree; Adding a removed m…

7715b90

…ethod

Fixing integration tests

c3591ed

Added event test

bc54329

Added the real time test

75df20f

Adding data test back

471d0a9

Fixed the object serialization test

f692bb6

Adding a factory method to MapBuilder

c368248

Added transaction integration test

a30ae95

Added transaction test

7714024

Added rules integration test

2da7955

Updated rules test

094d756

Merge branch 'hkj-initial-code-migration' into hkj-db-integration-tests

f446c79

Cleaning up the test code; Removed unnecessary utils and constants

952c6b0

Added more factory methods for MapBuilder; Removed unused legacy depe…

594bfbb

…ndency

Dropped the jackson test dependency

5dc413f

Fixed test race condition

2da5b03

Hiranya Jayathilaka and others added 2 commits April 12, 2017 17:35

Fixing some race conditions in ITs

7d39734

Cleaning up EventHelper instances after use

4b0361c

hiranya911 requested review from mikelehen and jwngr April 13, 2017 06:56

hiranya911 assigned jwngr Apr 13, 2017

mikelehen reviewed Apr 13, 2017

View reviewed changes

Hiranya Jayathilaka added 2 commits April 13, 2017 15:38

Adding back some logging code; Dropping fblocal URLs

3d3be22

Dropping all references to fblocal

eefc4ce

jwngr reviewed Apr 14, 2017

View reviewed changes

Hiranya Jayathilaka and others added 2 commits April 13, 2017 17:47

Removing port numbers from test URLs

e911387

Separated conjunctive assertions into separate ones for better error …

3c7f98f

…reporting.

jwngr approved these changes Apr 14, 2017

View reviewed changes

hiranya911 merged commit 76dd3d2 into master Apr 14, 2017

hiranya911 deleted the hkj-db-integration-tests branch April 14, 2017 16:52

jwngr removed their assignment Apr 15, 2017

mikelehen mentioned this pull request May 16, 2017

Fixing a Thread Leak in Database Client Code (Related to #29) #30

Merged

Porting the legacy integration test suite #2

Porting the legacy integration test suite #2

Uh oh!

Conversation

hiranya911 commented Apr 13, 2017

Uh oh!

mikelehen left a comment

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!

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!

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 Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen commented Apr 14, 2017

Uh oh!

jwngr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!