Check that calls to Terminate() and ClearPersistence() actually succeed.

I had incorrectly assumed that `Await()` would fail the test if the `Future` it was specified failed. This incorrect assumption hid the fact that `ClearPersistence()` failed on Android, resulting in unexplained downstream failures, resulting in a rollback: cl/326267389.

With this change, the failure of the `Future` objects returned from `ClearPersistence()` and `Terminate()` will be noted, highlighting the source of the test failures.

PiperOrigin-RevId: 330777066

Author: Googler

firestore/src/tests/firestore_integration_test.cc

@@ -41,6 +41,8 @@ void LocateEmulator(Firestore* db) {
   }
 }
 
+}  // namespace
+
 std::string ToFirestoreErrorCodeName(int error_code) {
   switch (error_code) {
     case kErrorOk:
@@ -82,7 +84,19 @@ std::string ToFirestoreErrorCodeName(int error_code) {
   }
 }
 
-}  // anonymous namespace
+int WaitFor(const FutureBase& future) {
+  // Instead of getting a clock, we count the cycles instead.
+  int cycles = kTimeOutMillis / kCheckIntervalMillis;
+  while (future.status() == FutureStatus::kFutureStatusPending && cycles > 0) {
+    if (ProcessEvents(kCheckIntervalMillis)) {
+      std::cout << "WARNING: app receives an event requesting exit."
+                << std::endl;
+      break;
+    }
+    --cycles;
+  }
+  return cycles;
+}
 
 FirestoreIntegrationTest::FirestoreIntegrationTest() {
   // Allocate the default Firestore eagerly.

firestore/src/tests/firestore_integration_test.h

@@ -29,6 +29,17 @@ App* GetApp();
 App* GetApp(const char* name);
 bool ProcessEvents(int msec);
 
+// Converts a Firestore error code to a human-friendly name. The `error_code`
+// argument is expected to be an element from the firebase::firestore::Error
+// enum, but this function will gracefully handle the case where it is not.
+std::string ToFirestoreErrorCodeName(int error_code);
+
+// Waits for a Future to complete. If a timeout is reached then this method
+// returns as if successful; therefore, the caller should verify the status of
+// the given Future after this function returns. Returns the number of cycles
+// that were left before a timeout would have occurred.
+int WaitFor(const FutureBase& future);
+
 template <typename T>
 class EventAccumulator;
 
@@ -222,17 +233,7 @@ class FirestoreIntegrationTest : public testing::Test {
   // A helper function to block until the future completes.
   template <typename T>
   static const T* Await(const Future<T>& future) {
-    // Instead of getting a clock, we count the cycles instead.
-    int cycles = kTimeOutMillis / kCheckIntervalMillis;
-    while (future.status() == FutureStatus::kFutureStatusPending &&
-           cycles > 0) {
-      if (ProcessEvents(kCheckIntervalMillis)) {
-        std::cout << "WARNING: app receives an event requesting exit."
-                  << std::endl;
-        return nullptr;
-      }
-      --cycles;
-    }
+    int cycles = WaitFor(future);
     EXPECT_GT(cycles, 0) << "Waiting future timed out.";
     if (future.status() == FutureStatus::kFutureStatusComplete) {
       if (future.result() == nullptr) {

firestore/src/tests/firestore_test.cc

@@ -8,6 +8,7 @@
 #include "firestore/src/include/firebase/firestore.h"
 #include "firestore/src/tests/firestore_integration_test.h"
 #include "firestore/src/tests/util/event_accumulator.h"
+#include "firestore/src/tests/util/future_test_util.h"
 #if defined(__ANDROID__)
 #include "firestore/src/android/util_android.h"
 #endif  // defined(__ANDROID__)
@@ -1179,7 +1180,7 @@ TEST_F(FirestoreIntegrationTest, TestToString) {
 // exceptions.
 #if defined(__ANDROID__)
 TEST_F(FirestoreIntegrationTest, ClientCallsAfterTerminateFails) {
-  Await(firestore()->Terminate());
+  EXPECT_THAT(firestore()->Terminate(), FutureSucceeds());
   EXPECT_THROW(Await(firestore()->DisableNetwork()), FirestoreException);
 }
 
@@ -1188,7 +1189,7 @@ TEST_F(FirestoreIntegrationTest, NewOperationThrowsAfterFirestoreTerminate) {
   DocumentReference reference = firestore()->Document("abc/123");
   Await(reference.Set({{"Field", FieldValue::Integer(100)}}));
 
-  Await(instance->Terminate());
+  EXPECT_THAT(instance->Terminate(), FutureSucceeds());
 
   EXPECT_THROW(Await(reference.Get()), FirestoreException);
   EXPECT_THROW(Await(reference.Update({{"Field", FieldValue::Integer(1)}})),
@@ -1214,12 +1215,12 @@ TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) {
   DocumentReference reference = instance->Document("abc/123");
   Await(reference.Set({{"Field", FieldValue::Integer(100)}}));
 
-  Await(instance->Terminate());
+  EXPECT_THAT(instance->Terminate(), FutureSucceeds());
 
   EXPECT_THROW(Await(reference.Get()), FirestoreException);
 
   // Calling a second time should go through and change nothing.
-  Await(instance->Terminate());
+  EXPECT_THAT(instance->Terminate(), FutureSucceeds());
 
   EXPECT_THROW(Await(reference.Update({{"Field", FieldValue::Integer(1)}})),
                FirestoreException);
@@ -1244,7 +1245,7 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) {
 
   // Shutdown `db` and create a new instance, make sure they are different
   // instances.
-  Await(db->Terminate());
+  EXPECT_THAT(db->Terminate(), FutureSucceeds());
   auto db_2 = CreateFirestore(app->name());
   EXPECT_NE(db_2, db);
 
@@ -1263,7 +1264,7 @@ TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) {
       accumulator.listener()->AttachTo(&reference);
 
   accumulator.Await();
-  Await(instance->Terminate());
+  EXPECT_THAT(instance->Terminate(), FutureSucceeds());
 
   // This should proceed without error.
   registration.Remove();
@@ -1321,8 +1322,8 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) {
 
   // ClearPersistence() requires Firestore to be terminated. Delete the app and
   // the Firestore instance to emulate the way an end user would do this.
-  Await(db->Terminate());
-  Await(db->ClearPersistence());
+  EXPECT_THAT(db->Terminate(), FutureSucceeds());
+  EXPECT_THAT(db->ClearPersistence(), FutureSucceeds());
   Release(db);
 
   // We restart the app with the same name and options to check that the
@@ -1347,7 +1348,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) {
   std::string path = document.path();
   WriteDocument(document, MapFieldValue{{"foo", FieldValue::Integer(42)}});
 
-  Await(db->Terminate());
+  EXPECT_THAT(db->Terminate(), FutureSucceeds());
   delete db;
   delete app;
 
@@ -1356,7 +1357,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) {
   // restart. Calling firestore() here would create a new instance of firestore,
   // which defeats the purpose of this test.
   Firestore* db_2 = CreateFirestore(app_name);
-  Await(db_2->ClearPersistence());
+  EXPECT_THAT(db_2->ClearPersistence(), FutureSucceeds());
   DocumentReference document_2 = db_2->Document(path);
   Future<DocumentSnapshot> await_get = document_2.Get(Source::kCache);
   Await(await_get);

firestore/src/tests/util/future_test_util.cc

@@ -0,0 +1,61 @@
+#include "firestore/src/tests/util/future_test_util.h"
+
+#include <ostream>
+
+#include "firestore/src/tests/firestore_integration_test.h"
+#include "firebase/firestore/firestore_errors.h"
+
+namespace firebase {
+
+namespace {
+
+void PrintTo(std::ostream* os, FutureStatus future_status, int error,
+             const char* error_message = nullptr) {
+  *os << "Future<void>{status=" << ToEnumeratorName(future_status)
+      << " error=" << firestore::ToFirestoreErrorCodeName(error);
+  if (error_message != nullptr) {
+    *os << " error_message=" << error_message;
+  }
+  *os << "}";
+}
+
+class FutureSucceedsImpl
+    : public testing::MatcherInterface<const Future<void>&> {
+ public:
+  void DescribeTo(std::ostream* os) const override {
+    PrintTo(os, FutureStatus::kFutureStatusComplete,
+            firestore::Error::kErrorOk);
+  }
+
+  bool MatchAndExplain(const Future<void>& future,
+                       testing::MatchResultListener*) const override {
+    firestore::WaitFor(future);
+    return future.status() == FutureStatus::kFutureStatusComplete &&
+           future.error() == firestore::Error::kErrorOk;
+  }
+};
+
+}  // namespace
+
+std::string ToEnumeratorName(FutureStatus status) {
+  switch (status) {
+    case kFutureStatusComplete:
+      return "kFutureStatusComplete";
+    case kFutureStatusPending:
+      return "kFutureStatusPending";
+    case kFutureStatusInvalid:
+      return "kFutureStatusInvalid";
+    default:
+      return "[invalid FutureStatus]";
+  }
+}
+
+void PrintTo(const Future<void>& future, std::ostream* os) {
+  PrintTo(os, future.status(), future.error(), future.error_message());
+}
+
+testing::Matcher<const Future<void>&> FutureSucceeds() {
+  return testing::Matcher<const Future<void>&>(new FutureSucceedsImpl());
+}
+
+}  // namespace firebase

firestore/src/tests/util/future_test_util.h

@@ -0,0 +1,35 @@
+#ifndef FIREBASE_FIRESTORE_CLIENT_CPP_SRC_TESTS_UTIL_FUTURE_TEST_UTIL_H_
+#define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_TESTS_UTIL_FUTURE_TEST_UTIL_H_
+
+#include <iosfwd>
+#include <string>
+
+#include "app/src/include/firebase/future.h"
+#include "third_party/googletest/googletest/include/gtest/gtest-matchers.h"
+
+namespace firebase {
+
+// Prints a human-friendly representation of a Future to an ostream.
+// This function is found dynamically by googletest to print a Future object
+// in a test failure message.
+void PrintTo(const Future<void>& future, std::ostream* os);
+
+// Creates and returns a "matcher" for a Future's success. The matcher will wait
+// for the Future to complete with a timeout. If the timeout is reached or the
+// Future completes unsuccessfully then the matcher will fail; otherwise, it
+// will succeed.
+//
+// Here is an example of how this function could be used:
+// EXPECT_THAT(firestore()->Terminate(), FutureSucceeds());
+testing::Matcher<const Future<void>&> FutureSucceeds();
+
+// Converts a `FutureStatus` value to its enumerator name, and returns it. For
+// example, if `kFutureStatusComplete` is specified then "kFutureStatusComplete"
+// will be returned. If the argument is invalid (i.e. not equal to any element
+// from `FutureStatus`) then this function will gracefully return a "name" to
+// indicate this.
+std::string ToEnumeratorName(FutureStatus status);
+
+}  // namespace firebase
+
+#endif  // FIREBASE_FIRESTORE_CLIENT_CPP_SRC_TESTS_UTIL_FUTURE_TEST_UTIL_H_