Set firebase-database to -Werror #1030

inlined · 2019-12-03T21:03:24Z

As a bar-raising experiment, I added -Werror to our compilation options. This broke the build; fixes are also included in this change. With this update, compiler warnings (most importantly, errorprone warnings) will not be allowed into future pull requests. Hopefully this will catch a bug or two before our customers do.

inlined

Annotations for the first case of every type of warning/error I fixed

inlined · 2019-12-03T21:04:48Z

firebase-database/firebase-database.gradle

    androidTestImplementation 'net.java:quickcheck:0.6'
-    androidTestAnnotationProcessor 'net.java:quickcheck-src-generator:0.6'


These annotations were not used and caused conflicts because their maximum version is 1.6

inlined · 2019-12-03T21:05:29Z

firebase-database/src/androidTest/java/com/google/firebase/database/DataTest.java

@@ -710,7 +712,7 @@ public boolean isComplete(List<EventRecord> events) {
                  try {
                    ref.child("two").removeValue();
                  } catch (DatabaseException e) {
-                    fail("Should not fail");
+                    throw new AssertionError("Should not fail", e);


errorprone calls catch-then-fail an anti-pattern. AssertionError is an unchecked exception that can wrap any other throwable to preserve cause and stack traces.

inlined · 2019-12-03T21:05:48Z

firebase-database/src/androidTest/java/com/google/firebase/database/DataTest.java

@@ -2346,9 +2348,8 @@ public void onComplete(DatabaseError error, DatabaseReference ref) {
    assertEquals(snap.getPriority().getClass(), Double.class);
    assertEquals(snap.getPriority(), snap.child("b").getPriority());
    assertEquals(snap.child("a").getValue(), snap.child("b").getValue());
-    assert (Math.abs(


assert is a production method, not a test method

inlined · 2019-12-03T21:06:52Z

firebase-database/src/androidTest/java/com/google/firebase/database/EventTest.java

@@ -623,15 +626,15 @@ public void onDataChange(DataSnapshot snapshot) {
              try {
                IntegrationTestHelpers.waitFor(blockSem);
              } catch (InterruptedException e) {
-                e.printStackTrace();
+                LOGGER.log(WARNING, "unexpected error", e);


I'll admit I don't fully understand why printStackTrace() is considered an anti-pattern. I'm assuming it's because we should be printing to logcat rather than stdout.

inlined · 2019-12-03T21:07:12Z

firebase-database/src/androidTest/java/com/google/firebase/database/FirebaseDatabaseTest.java

@@ -20,8 +20,8 @@
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.fail;

-import androidx.test.InstrumentationRegistry;
-import androidx.test.runner.AndroidJUnit4;
+import androidx.test.ext.junit.runners.AndroidJUnit4;


These libraries moved

inlined · 2019-12-03T21:14:47Z

firebase-database/src/test/java/com/google/firebase/database/MapperTest.java

@@ -1243,8 +1245,7 @@ public void beanMapsMustHaveStringKeys() {

  @Test
  public void serializeStringBean() {
-    StringBean bean = new StringBean();
-    bean.value = "foo";


Direct member access was disallowed. I had the choice of adding a setValue or just a constructor that accepted a value.

inlined · 2019-12-03T21:15:50Z

firebase-database/src/test/java/com/google/firebase/database/MapperTest.java

@@ -1380,54 +1380,28 @@ public void serializingListsWorks() {
  @Test
  public void serializingMapsWorks() {
    MapBean bean = new MapBean();
-    bean.values =


Parameter list initialization is disallowed because it leads to memory issues. We can use the ImmutableFoo.of methods in unit tests or the ImmutableFoo builders in code with a lower target JRE version.

inlined · 2019-12-03T21:16:54Z

firebase-database/src/test/java/com/google/firebase/database/util/ComparatorTester.java

@@ -151,8 +150,8 @@ private final void doTestEquivalanceGroupOrdering() {
        testClassCast(reference);
        for (int otherIndex = 0; otherIndex < equalityGroups.size(); otherIndex++) {
          for (Object other : equalityGroups.get(otherIndex)) {
-            assertThat(Integer.signum(compare(reference, other)))
-                .named("compare(%s, %s)", reference, other)


The .named method is deprecated. We're instead supposed to assertWithMessage.

inlined · 2019-12-03T21:17:48Z

firebase-database/src/test/java/com/google/firebase/database/util/ComparatorTester.java

-            assertWithMessage(
-                    "Testing equals() for compatibility with compare()/compareTo(), "
-                        + "add a call to doNotRequireEqualsCompatibility() if this is not required")
+            // Testing equals() for compatibility with compare()/compareTo(),


Per above, I had to use only assertWithMessage, not .named I kept the printf text formerly in .named and moved the other message to a comment to avoid losing information.

inlined · 2019-12-03T21:19:11Z

firebase-database/src/testUtil/java/com/google/firebase/database/DeepEquals.java

@@ -99,7 +100,7 @@ private static boolean weakTypeMatch(Object a, Object b) {
  }

  private static boolean deepEquals(Object a, Object b, Set visited) {
-    LinkedList<DualKey> stack = new LinkedList<DualKey>();
+    Deque<DualKey> stack = new ArrayDeque<>();


LinkedList is generally prohibited because it almost never outperforms an ArrayList (there's a great C++ talk from Meyers on this; std::linked_list performs worse than std::vector for random operations and the gap actually increases as the list size increases. Memory locality is more important than O-notation)

schmidt-sebastian

Thank you so much!

schmidt-sebastian · 2019-12-03T21:19:53Z

firebase-database/src/test/java/com/google/firebase/database/util/ComparatorTester.java

-                        + "add a call to doNotRequireEqualsCompatibility() if this is not required")
+            // Testing equals() for compatibility with compare()/compareTo(),
+            // add a call to doNotRequireEqualsCompatibility() if this is not required
+            assertWithMessage("%s.equals(%s)", reference, other)


Nit: Should the message have quotes around %s?

LOL. you just couldn't let me have a clean review, eh?

schmidt-sebastian · 2019-12-03T21:29:30Z

Annotations for the first case of every type of warning/error I fixed

Awesome :) Thank you!

* Set compile options to -Werror * Fix all build breaks

PromanSEW · 2019-12-04T19:29:58Z

firebase-database/src/androidTest/java/com/google/firebase/database/DataTest.java

-    assert (Math.abs(
-            System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString()))
-        < 2000);
+    Long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());


Suggested change

Long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());

long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());

Good cleanup

PromanSEW · 2019-12-04T19:35:57Z

firebase-database/src/main/java/com/google/firebase/database/core/utilities/ParsedUrl.java

+    int result = repoInfo.hashCode();
+    result = 31 * result + path.hashCode();
+    return result;


Suggested change

int result = repoInfo.hashCode();

result = 31 * result + path.hashCode();

return result;

return 31 * repoInfo.hashCode() + path.hashCode();

It's been too many years since I've been in school to remember the name of the notation but writing
AX^N + BX^(N-1) + CX^(N-2)...YX^1 + Z as
((((A)X + B)X + C)X + D)...
is a much very common factorization for hashes and checksums because it forms a pattern that works incrementally as the input increase. Keeping the pattern.

PromanSEW · 2019-12-04T19:36:53Z

firebase-database/src/test/java/com/google/firebase/database/MapperTest.java

@@ -37,11 +37,17 @@
 @org.junit.runner.RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class MapperTest {
-  private static final double EPISLON = 0.00001f;
+  private static final double EPISLON = 0.00025f;


Suggested change

private static final double EPISLON = 0.00025f;

private static final double EPSILON = 0.00025f;

Good typo catch.

PromanSEW · 2019-12-04T19:37:50Z

firebase-database/src/test/java/com/google/firebase/database/DatabaseRegistrarTest.java

@@ -41,7 +41,7 @@ public void getComponents_publishesLibVersionComponent() {

  private static FirebaseApp appForDatabaseUrl(String url, String name) {
    return FirebaseApp.initializeApp(
-        RuntimeEnvironment.application.getApplicationContext(),
+        getApplicationContext(),


Suggested change

getApplicationContext(),

ApplicationProvider.getApplicationContext(),

The public Google style guide only states that static methods should be referenced from a class and not a variable. The Google employee style guide is stricter and enforces this by requiring static methods to be statically imported.

PromanSEW · 2019-12-04T19:38:48Z

firebase-database/src/test/java/com/google/firebase/database/UnitTestHelpers.java

@@ -289,7 +289,7 @@ public static void ensureAppInitialized() {
    if (!appInitialized) {
      appInitialized = true;
      FirebaseApp.initializeApp(
-          RuntimeEnvironment.application.getApplicationContext(),
+          getApplicationContext(),


Suggested change

getApplicationContext(),

ApplicationProvider.getApplicationContext(),

PromanSEW · 2019-12-04T19:39:03Z

...ase-database/src/test/java/com/google/firebase/database/apitest/FirebaseDatabaseApiTest.java

@@ -59,7 +59,7 @@ public void getDifferentInstanceForAppWithUrl() {

  private static FirebaseApp appForDatabaseUrl(String url, String name) {
    return FirebaseApp.initializeApp(
-        RuntimeEnvironment.application.getApplicationContext(),
+        getApplicationContext(),


Suggested change

getApplicationContext(),

ApplicationProvider.getApplicationContext(),

inlined

Thanks for the drive-by sanity check @PromanSEW

inlined · 2019-12-09T18:55:28Z

firebase-database/src/androidTest/java/com/google/firebase/database/DataTest.java

-    assert (Math.abs(
-            System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString()))
-        < 2000);
+    Long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());


Good cleanup

inlined · 2019-12-09T19:05:46Z

firebase-database/src/main/java/com/google/firebase/database/core/utilities/ParsedUrl.java

+    int result = repoInfo.hashCode();
+    result = 31 * result + path.hashCode();
+    return result;


It's been too many years since I've been in school to remember the name of the notation but writing
AX^N + BX^(N-1) + CX^(N-2)...YX^1 + Z as
((((A)X + B)X + C)X + D)...
is a much very common factorization for hashes and checksums because it forms a pattern that works incrementally as the input increase. Keeping the pattern.

inlined · 2019-12-09T19:06:21Z

firebase-database/src/test/java/com/google/firebase/database/DatabaseRegistrarTest.java

@@ -41,7 +41,7 @@ public void getComponents_publishesLibVersionComponent() {

  private static FirebaseApp appForDatabaseUrl(String url, String name) {
    return FirebaseApp.initializeApp(
-        RuntimeEnvironment.application.getApplicationContext(),
+        getApplicationContext(),


The public Google style guide only states that static methods should be referenced from a class and not a variable. The Google employee style guide is stricter and enforces this by requiring static methods to be statically imported.

inlined · 2019-12-09T19:08:08Z

firebase-database/src/test/java/com/google/firebase/database/MapperTest.java

@@ -37,11 +37,17 @@
 @org.junit.runner.RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class MapperTest {
-  private static final double EPISLON = 0.00001f;
+  private static final double EPISLON = 0.00025f;


Good typo catch.

google-oss-bot · 2019-12-09T19:28:05Z

@inlined: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
api-information	`17e511c`	link	`/test api-information`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

inlined · 2019-12-09T22:41:06Z

Ignoring the api-information test failure. It seems like all pull requests have failed :firebase-common:ktx:apiInformation since #1041.

googlebot added the cla: yes Override cla label Dec 3, 2019

inlined requested a review from schmidt-sebastian December 3, 2019 21:03

inlined assigned schmidt-sebastian Dec 3, 2019

google-oss-bot added the size/L label Dec 3, 2019

inlined commented Dec 3, 2019

View reviewed changes

schmidt-sebastian approved these changes Dec 3, 2019

View reviewed changes

schmidt-sebastian removed their assignment Dec 3, 2019

Fix all warnings

8f8c66a

* Set compile options to -Werror * Fix all build breaks

inlined force-pushed the inlined.lint branch from 68697b4 to 8f8c66a Compare December 4, 2019 18:45

PromanSEW reviewed Dec 4, 2019

View reviewed changes

inlined commented Dec 9, 2019

View reviewed changes

PR feedback

17e511c

inlined merged commit 00956bc into master Dec 9, 2019

firebase locked and limited conversation to collaborators Jan 9, 2020

kaibolay deleted the inlined.lint branch September 14, 2022 17:53

		androidTestImplementation 'net.java:quickcheck:0.6'
		androidTestAnnotationProcessor 'net.java:quickcheck-src-generator:0.6'

	Long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());
	long drift = System.currentTimeMillis() - Long.parseLong(snap.child("a").getValue().toString());

	private static final double EPISLON = 0.00025f;
	private static final double EPSILON = 0.00025f;

	getApplicationContext(),
	ApplicationProvider.getApplicationContext(),

Set firebase-database to -Werror #1030

Set firebase-database to -Werror #1030

Uh oh!

Conversation

inlined commented Dec 3, 2019

Uh oh!

inlined 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!

schmidt-sebastian 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!

schmidt-sebastian commented Dec 3, 2019

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!

inlined 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!

google-oss-bot commented Dec 9, 2019

Uh oh!

inlined commented Dec 9, 2019

Uh oh!

Uh oh!