Skip to content

Commit ebb43ad

Browse files
Googlera-maurice
authored andcommitted
Automated g4 rollback of changelist 326267389.
*** Reason for rollback *** Roll forward cl/325229850: Improve the tests for ClearPersistence() to use the default Firebase app instead of a custom one. I've added a workaround for the broken test, a call to Terminate() in Android, and created b/168628900 to track the underlying bug. *** Original change description *** Automated g4 rollback of changelist 325229850. *** Reason for rollback *** firestore_test_android appears to have been broken by this change (b/163526671). *** Original change description *** Improve the tests for ClearPersistence() to use the default Firebase app instead of a custom one. This fixes a concern raised by @pengzk in cl/309775646 that the Firestore* pointers were leaking. By using the default Firestore object retrieved via firestore() instead of a custom Firestore object return... *** PiperOrigin-RevId: 331991549
1 parent 721c280 commit ebb43ad

File tree

1 file changed

+49
-23
lines changed

1 file changed

+49
-23
lines changed

firestore/src/tests/firestore_test.cc

Lines changed: 49 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1311,59 +1311,85 @@ TEST_F(FirestoreIntegrationTest,
13111311
EXPECT_EQ(await_pending_writes.status(), FutureStatus::kFutureStatusComplete);
13121312
}
13131313

1314-
TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) {
1315-
Firestore* db = CreateFirestore();
1316-
App* app = db->app();
1317-
std::string app_name = app->name();
1314+
TEST_F(FirestoreIntegrationTest, CanClearPersistenceTestHarnessVerification) {
1315+
// Verify that firestore() and DeleteFirestore() behave how we expect;
1316+
// otherwise, the tests for ClearPersistence() could yield false positives.
1317+
Firestore* db = firestore();
1318+
const std::string app_name = db->app()->name();
1319+
DocumentReference document = db->Collection("a").Document();
1320+
const std::string path = document.path();
1321+
WriteDocument(document, MapFieldValue{{"foo", FieldValue::Integer(42)}});
1322+
DeleteFirestore();
13181323

1324+
Firestore* db_2 = CachedFirestore(app_name);
1325+
DocumentReference document_2 = db_2->Document(path);
1326+
Future<DocumentSnapshot> get_future = document_2.Get(Source::kCache);
1327+
DocumentSnapshot snapshot_2 = *Await(get_future);
1328+
EXPECT_THAT(
1329+
snapshot_2.GetData(),
1330+
testing::ContainerEq(MapFieldValue{{"foo", FieldValue::Integer(42)}}));
1331+
}
1332+
1333+
TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) {
1334+
Firestore* db = firestore();
1335+
const std::string app_name = db->app()->name();
13191336
DocumentReference document = db->Collection("a").Document("b");
1320-
std::string path = document.path();
1337+
const std::string path = document.path();
13211338
WriteDocument(document, MapFieldValue{{"foo", FieldValue::Integer(42)}});
13221339

1323-
// ClearPersistence() requires Firestore to be terminated. Delete the app and
1324-
// the Firestore instance to emulate the way an end user would do this.
1340+
// Call ClearPersistence(), but call Terminate() first because
1341+
// ClearPersistence() requires Firestore to be terminated.
13251342
EXPECT_THAT(db->Terminate(), FutureSucceeds());
13261343
EXPECT_THAT(db->ClearPersistence(), FutureSucceeds());
1327-
Release(db);
1344+
// Call DeleteFirestore() to ensure that both the App and Firestore instances
1345+
// are deleted, which emulates the way an end user would experience their
1346+
// application being killed and later re-launched by the user.
1347+
DeleteFirestore();
13281348

13291349
// We restart the app with the same name and options to check that the
13301350
// previous instance's persistent storage is actually cleared after the
1331-
// restart. Calling firestore() here would create a new instance of firestore,
1332-
// which defeats the purpose of this test.
1333-
Firestore* db_2 = CreateFirestore(app_name);
1351+
// restart. Although calling firestore() here would do the same thing, we
1352+
// use CachedFirestore() to be explicit about getting a new Firestore instance
1353+
// for the same Firebase app.
1354+
Firestore* db_2 = CachedFirestore(app_name);
13341355
DocumentReference document_2 = db_2->Document(path);
13351356
Future<DocumentSnapshot> await_get = document_2.Get(Source::kCache);
13361357
Await(await_get);
13371358
EXPECT_EQ(await_get.status(), FutureStatus::kFutureStatusComplete);
13381359
EXPECT_EQ(await_get.error(), Error::kErrorUnavailable);
1339-
Release(db_2);
13401360
}
13411361

13421362
TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) {
1343-
Firestore* db = CreateFirestore();
1344-
App* app = db->app();
1345-
std::string app_name = app->name();
1346-
1363+
Firestore* db = firestore();
1364+
const std::string app_name = db->app()->name();
13471365
DocumentReference document = db->Collection("a").Document("b");
1348-
std::string path = document.path();
1366+
const std::string path = document.path();
13491367
WriteDocument(document, MapFieldValue{{"foo", FieldValue::Integer(42)}});
13501368

1369+
#if defined(__ANDROID__)
1370+
// TODO(b/168628900) Remove this call to Terminate() once deleting the
1371+
// Firestore* instance removes the underlying Java object from the instance
1372+
// cache in Android.
13511373
EXPECT_THAT(db->Terminate(), FutureSucceeds());
1352-
delete db;
1353-
delete app;
1374+
#endif
1375+
1376+
// Call DeleteFirestore() to ensure that both the App and Firestore instances
1377+
// are deleted, which emulates the way an end user would experience their
1378+
// application being killed and later re-launched by the user.
1379+
DeleteFirestore();
13541380

13551381
// We restart the app with the same name and options to check that the
13561382
// previous instance's persistent storage is actually cleared after the
1357-
// restart. Calling firestore() here would create a new instance of firestore,
1358-
// which defeats the purpose of this test.
1359-
Firestore* db_2 = CreateFirestore(app_name);
1383+
// restart. Although calling firestore() here would do the same thing, we
1384+
// use CachedFirestore() to be explicit about getting a new Firestore instance
1385+
// for the same Firebase app.
1386+
Firestore* db_2 = CachedFirestore(app_name);
13601387
EXPECT_THAT(db_2->ClearPersistence(), FutureSucceeds());
13611388
DocumentReference document_2 = db_2->Document(path);
13621389
Future<DocumentSnapshot> await_get = document_2.Get(Source::kCache);
13631390
Await(await_get);
13641391
EXPECT_EQ(await_get.status(), FutureStatus::kFutureStatusComplete);
13651392
EXPECT_EQ(await_get.error(), Error::kErrorUnavailable);
1366-
Release(db_2);
13671393
}
13681394

13691395
TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) {

0 commit comments

Comments
 (0)