Remove Base64 and Other Redundant Utils. Implement Async Error Checks for Integration Tests. #4
Conversation
* 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.
…min-java into hkj-release-process
…tps://github.com/google/guava/wiki/Release21); Excluding the transitive dependency on Guava 17 from Google API client to prevent having 2 versions of Guava in the classpath
… hkj-remove-utils
| // TODO: should we set an uncaught exception handler here? Probably want | ||
| // to let | ||
| // exceptions happen... | ||
| threadInitializer.setUncaughtExceptionHandler(thread, ThreadPoolEventTarget.this); |
There was a problem hiding this comment.
Thanks for tackling this! For future reference, it'd be nice to keep unrelated changes like this in a separate PR for easier reviewing... especially since the rest of this PR is pretty mechanical... but now that I've seen a non-mechanical change, I'm paranoid there may be others I should watch out for (in other words, it's usually better to keep "pure refactoring" PRs separate from "behavior change" PRs)
| FirebaseOptions options2 = | ||
| new FirebaseOptions.Builder() | ||
| .setCredential(credential) | ||
| .setDatabaseUrl("http://test.firebaseio.com") |
There was a problem hiding this comment.
Generally speaking, firebase URLs should always use https://. Doesn't really matter in this context, but I'd prefer avoid setting (or extending?) a bad precedent and we seem to have a bunch of http:// URLs in this PR.
| return thread; | ||
| } | ||
| }); | ||
| public static void wrapForErrorHandling(FirebaseApp app) { |
| } | ||
| | ||
| private static class TestRunLoop extends DefaultRunLoop { | ||
| private static class ErrorHandlingRunLoop extends DefaultRunLoop { |
There was a problem hiding this comment.
It seems weird that you're both wrapping and extending DefaultRunLoop. If this is really intentional, maybe add a comment explaining what's going on?
There was a problem hiding this comment.
It's the decorator pattern in use. I added a comment to explain.
2 participants
Remove Redundant Utils
Following classes from the internal package have been removed.
Base64andBase64UtilsPreconditionsObjectsInvocations of these utils have been replaced by calls to the Guava library, which implements more or less the same APIs.
Async Error Checks for Integration Tests
Two new methods have been introduced to
TestHelpers:wrapForErrorHandling(app)assertAndUnwrapErrorHandlers(app)First method registers some error handlers with the
RunLoopandEventTargetofFirebaseDatabase. Second method checks whether the error handlers were fired, and if so throws the exception wrapped in aRuntimeException. By calling the above two methods in appropriate test fixtures (@Beforeand@Afterrespectively), we can detect any async errors that may occur during integration tests, and force the corresponding test case to fail.