From 41a85e1ad7c502009ea5727a54c782bef380e2c0 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 14 Apr 2023 16:38:03 -0700 Subject: [PATCH 01/24] debugging the memory error --- firestore/src/common/firestore.cc | 37 +++++++++++++++------- firestore/src/include/firebase/firestore.h | 1 + 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 9fe7e3c45c..14d81e6d32 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -63,25 +63,36 @@ const char* GetPlatform() { #endif } +using FirestoreMap = std::map, Firestore*>; + +FirestoreMap::key_type MakeKey(App* app, const char* database_id) { + return std::make_pair(std::string(app->name()), + database_id ? database_id : kDefaultDatabase); +} + Mutex* g_firestores_lock = new Mutex(); -std::map* g_firestores = nullptr; +static FirestoreMap* g_firestores = nullptr; // Ensures that the cache is initialized. // Prerequisite: `g_firestores_lock` must be locked before calling this // function. -std::map* FirestoreCache() { +FirestoreMap* FirestoreCache() { if (!g_firestores) { - g_firestores = new std::map(); + g_firestores = new FirestoreMap(); } return g_firestores; } // Prerequisite: `g_firestores_lock` must be locked before calling this // function. -Firestore* FindFirestoreInCache(App* app, InitResult* init_result_out) { +Firestore* FindFirestoreInCache(App* app, + const char* database_id, + InitResult* init_result_out) { auto* cache = FirestoreCache(); - auto found = cache->find(app); + FirestoreMap::key_type key = MakeKey(app, database_id); + FirestoreMap::iterator found = g_firestores->find(key); + if (found != cache->end()) { if (init_result_out) *init_result_out = kInitResultSuccess; return found->second; @@ -150,12 +161,12 @@ Firestore* Firestore::GetInstance(App* app, ValidateDatabase(db_name); MutexLock lock(*g_firestores_lock); - Firestore* from_cache = FindFirestoreInCache(app, init_result_out); + Firestore* from_cache = FindFirestoreInCache(app, db_name, init_result_out); if (from_cache) { return from_cache; } - return AddFirestoreToCache(new Firestore(app, db_name), init_result_out); + return AddFirestoreToCache(new Firestore(app, db_name),db_name, init_result_out); } Firestore* Firestore::CreateFirestore(App* app, @@ -167,14 +178,15 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - Firestore* from_cache = FindFirestoreInCache(app, init_result_out); + Firestore* from_cache = FindFirestoreInCache(app, nullptr, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, "Firestore must not be created already"); - return AddFirestoreToCache(new Firestore(internal), init_result_out); + return AddFirestoreToCache(new Firestore(internal),nullptr, init_result_out); } Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, + const char* db_name, InitResult* init_result_out) { InitResult init_result = CheckInitialized(*firestore->internal_); if (init_result_out) { @@ -185,7 +197,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreCache()->emplace(firestore->app(), firestore); + FirestoreCache()->emplace(MakeKey(firestore->app(), db_name), firestore); return firestore; } @@ -226,6 +238,7 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); + const std::string& database_id = internal_->database_id().database_id(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -249,7 +262,7 @@ void Firestore::DeleteInternal() { delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreCache()->erase(my_app); + FirestoreCache()->erase(MakeKey(my_app, database_id.c_str())); // If it's the last one, delete the map. if (g_firestores->empty()) { delete g_firestores; @@ -362,7 +375,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreCache()->erase(app()); + FirestoreCache()->erase(MakeKey(app(), nullptr)); return internal_->Terminate(); } diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index a2befc375d..9c745a2127 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -503,6 +503,7 @@ class Firestore { FirestoreInternal* internal, InitResult* init_result_out); static Firestore* AddFirestoreToCache(Firestore* firestore, + const char* database_id, InitResult* init_result_out); static void SetClientLanguage(const std::string& language_token); From 7a6bbc201b57ae0039b4f7a43507df846ed57bd9 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 17 Apr 2023 10:16:10 -0700 Subject: [PATCH 02/24] change the firestore cache map key type --- .../src/integration_test.cc | 9 +++-- firestore/src/common/firestore.cc | 37 +++++++++++++++---- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/firestore/integration_test_internal/src/integration_test.cc b/firestore/integration_test_internal/src/integration_test.cc index 13b36d59f6..5ee48f5eb4 100644 --- a/firestore/integration_test_internal/src/integration_test.cc +++ b/firestore/integration_test_internal/src/integration_test.cc @@ -212,11 +212,14 @@ FirebaseFirestoreBasicTest::~FirebaseFirestoreBasicTest() { } void FirebaseFirestoreBasicTest::SetUp() { + std::cout << "SetUp" << std::endl; + FirebaseTest::SetUp(); InitializeFirestore(); } void FirebaseFirestoreBasicTest::TearDown() { + std::cout << "tear down" << std::endl; // Delete the shared path, if there is one. if (initialized_) { if (!cleanup_documents_.empty() && firestore_ && shared_app_) { @@ -343,9 +346,9 @@ firebase::firestore::DocumentReference FirebaseFirestoreBasicTest::Doc( // Test cases below. -TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { - // Already tested via SetUp() and TearDown(). -} +//TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { +// // Already tested via SetUp() and TearDown(). +//} TEST_F(FirebaseFirestoreBasicTest, TestSignIn) { SKIP_TEST_ON_QUICK_CHECK; diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 14d81e6d32..911bdafe72 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include "app/meta/move.h" #include "app/src/cleanup_notifier.h" @@ -63,23 +64,28 @@ const char* GetPlatform() { #endif } -using FirestoreMap = std::map, Firestore*>; +using FirestoreMap = std::map, Firestore*>; FirestoreMap::key_type MakeKey(App* app, const char* database_id) { - return std::make_pair(std::string(app->name()), - database_id ? database_id : kDefaultDatabase); + return std::make_pair(app, + std::string (database_id ? database_id : kDefaultDatabase));// check db is not null or empty } Mutex* g_firestores_lock = new Mutex(); -static FirestoreMap* g_firestores = nullptr; +FirestoreMap* g_firestores = nullptr; // Ensures that the cache is initialized. // Prerequisite: `g_firestores_lock` must be locked before calling this // function. FirestoreMap* FirestoreCache() { + std::cout<<"FirestoreCache"<size()<internal_); if (init_result_out) { *init_result_out = init_result; @@ -197,7 +204,8 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreCache()->emplace(MakeKey(firestore->app(), db_name), firestore); + FirestoreMap::key_type key = MakeKey(firestore->app(), db_name); + FirestoreCache()->emplace(key, firestore); return firestore; } @@ -208,6 +216,7 @@ Firestore::Firestore(FirestoreInternal* internal) // TODO(wuandy): use make_unique once it is supported for our build here. : internal_(internal) { internal_->set_firestore_public(this); + std::cout<<"Firestore"<initialized()) { + std::cout<<"if initialized"<RegisterObject(this, [](void* object) { Firestore* firestore = reinterpret_cast(object); LogWarning( @@ -233,12 +246,15 @@ Firestore::Firestore(FirestoreInternal* internal) Firestore::~Firestore() { DeleteInternal(); } void Firestore::DeleteInternal() { + std::cout<<"DeleteInternal"<database_id().database_id(); + std::cout<initialized()) { @@ -262,7 +278,11 @@ void Firestore::DeleteInternal() { delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreCache()->erase(MakeKey(my_app, database_id.c_str())); + FirestoreMap::key_type key = MakeKey(my_app, database_id.c_str()); + + g_firestores->erase(key); + std::cout<<"after erase"<< g_firestores->size()<empty()) { delete g_firestores; @@ -375,7 +395,10 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); - FirestoreCache()->erase(MakeKey(app(), nullptr)); + const std::string& database_id = internal_->database_id().database_id(); + FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); + + FirestoreCache()->erase(key); return internal_->Terminate(); } From 7bf521a3c70495ef96ecc7b0300efdf831555521 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 17 Apr 2023 21:35:25 -0700 Subject: [PATCH 03/24] fill in default database id --- firestore/src/common/firestore.cc | 32 +++++++++++-------------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 911bdafe72..f3e704c600 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -19,7 +19,6 @@ #include #include #include -#include #include "app/meta/move.h" #include "app/src/cleanup_notifier.h" @@ -67,8 +66,9 @@ const char* GetPlatform() { using FirestoreMap = std::map, Firestore*>; FirestoreMap::key_type MakeKey(App* app, const char* database_id) { - return std::make_pair(app, - std::string (database_id ? database_id : kDefaultDatabase));// check db is not null or empty + return std::make_pair( + app, + std::string(database_id)); // check db is not null or empty } Mutex* g_firestores_lock = new Mutex(); @@ -78,13 +78,9 @@ FirestoreMap* g_firestores = nullptr; // Prerequisite: `g_firestores_lock` must be locked before calling this // function. FirestoreMap* FirestoreCache() { - std::cout<<"FirestoreCache"<size()<internal_); if (init_result_out) { *init_result_out = init_result; @@ -206,6 +203,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, FirestoreMap::key_type key = MakeKey(firestore->app(), db_name); FirestoreCache()->emplace(key, firestore); + return firestore; } @@ -216,7 +214,6 @@ Firestore::Firestore(FirestoreInternal* internal) // TODO(wuandy): use make_unique once it is supported for our build here. : internal_(internal) { internal_->set_firestore_public(this); - std::cout<<"Firestore"<initialized()) { - std::cout<<"if initialized"<RegisterObject(this, [](void* object) { Firestore* firestore = reinterpret_cast(object); @@ -246,15 +240,12 @@ Firestore::Firestore(FirestoreInternal* internal) Firestore::~Firestore() { DeleteInternal(); } void Firestore::DeleteInternal() { - std::cout<<"DeleteInternal"<database_id().database_id(); - std::cout<initialized()) { @@ -278,12 +269,11 @@ void Firestore::DeleteInternal() { delete internal_; internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreMap::key_type key = MakeKey(my_app, database_id.c_str()); - g_firestores->erase(key); - std::cout<<"after erase"<< g_firestores->size()<erase(key); if (g_firestores->empty()) { delete g_firestores; g_firestores = nullptr; From 6c46314bebd0ffa5fb14af73dacdc095383f8481 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 17 Apr 2023 21:38:49 -0700 Subject: [PATCH 04/24] remove std::cout --- .../integration_test_internal/src/integration_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/firestore/integration_test_internal/src/integration_test.cc b/firestore/integration_test_internal/src/integration_test.cc index 5ee48f5eb4..4835ecd434 100644 --- a/firestore/integration_test_internal/src/integration_test.cc +++ b/firestore/integration_test_internal/src/integration_test.cc @@ -212,14 +212,11 @@ FirebaseFirestoreBasicTest::~FirebaseFirestoreBasicTest() { } void FirebaseFirestoreBasicTest::SetUp() { - std::cout << "SetUp" << std::endl; - FirebaseTest::SetUp(); InitializeFirestore(); } void FirebaseFirestoreBasicTest::TearDown() { - std::cout << "tear down" << std::endl; // Delete the shared path, if there is one. if (initialized_) { if (!cleanup_documents_.empty() && firestore_ && shared_app_) { @@ -346,9 +343,9 @@ firebase::firestore::DocumentReference FirebaseFirestoreBasicTest::Doc( // Test cases below. -//TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { -// // Already tested via SetUp() and TearDown(). -//} +// TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { +// // Already tested via SetUp() and TearDown(). +// } TEST_F(FirebaseFirestoreBasicTest, TestSignIn) { SKIP_TEST_ON_QUICK_CHECK; From 81d607e165da0f80f567833716ac276320d641d3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 17 Apr 2023 22:29:32 -0700 Subject: [PATCH 05/24] format --- .../src/integration_test.cc | 6 +++--- .../src/util/integration_test_util.cc | 10 +++++++-- firestore/src/common/firestore.cc | 21 +++++++++---------- 3 files changed, 21 insertions(+), 16 deletions(-) diff --git a/firestore/integration_test_internal/src/integration_test.cc b/firestore/integration_test_internal/src/integration_test.cc index 4835ecd434..13b36d59f6 100644 --- a/firestore/integration_test_internal/src/integration_test.cc +++ b/firestore/integration_test_internal/src/integration_test.cc @@ -343,9 +343,9 @@ firebase::firestore::DocumentReference FirebaseFirestoreBasicTest::Doc( // Test cases below. -// TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { -// // Already tested via SetUp() and TearDown(). -// } +TEST_F(FirebaseFirestoreBasicTest, TestInitializeAndTerminate) { + // Already tested via SetUp() and TearDown(). +} TEST_F(FirebaseFirestoreBasicTest, TestSignIn) { SKIP_TEST_ON_QUICK_CHECK; diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index 192238def0..d010b64d9d 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -34,10 +34,11 @@ namespace firebase { namespace firestore { struct TestFriend { - static FirestoreInternal* CreateTestFirestoreInternal(App* app) { + static FirestoreInternal* CreateTestFirestoreInternal( + App* app, const char* database_id = kDefaultDatabase) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, /*database_id=*/"(default)", + app, database_id, absl::make_unique(), absl::make_unique()); #else @@ -83,5 +84,10 @@ FirestoreInternal* CreateTestFirestoreInternal(App* app) { return TestFriend::CreateTestFirestoreInternal(app); } +FirestoreInternal* CreateTestFirestoreInternal(App* app, + const char* database_id) { + return TestFriend::CreateTestFirestoreInternal(app, database_id); +} + } // namespace firestore } // namespace firebase diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index f3e704c600..f88bf910e1 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -63,12 +63,12 @@ const char* GetPlatform() { #endif } +// Use the combination of the App address and database ID to identify a +// firestore instance in cache. using FirestoreMap = std::map, Firestore*>; FirestoreMap::key_type MakeKey(App* app, const char* database_id) { - return std::make_pair( - app, - std::string(database_id)); // check db is not null or empty + return std::make_pair(app, std::string(database_id)); } Mutex* g_firestores_lock = new Mutex(); @@ -81,7 +81,6 @@ FirestoreMap* FirestoreCache() { if (!g_firestores) { g_firestores = new FirestoreMap(); } - return g_firestores; } @@ -120,8 +119,8 @@ void ValidateApp(App* app) { } } -void ValidateDatabase(const char* db_name) { - if (!db_name) { +void ValidateDatabase(const char* database_id) { + if (!database_id) { SimpleThrowInvalidArgument( "Provided database ID must not be null. Use other " "firebase::App::GetInstance() if you'd like to use the default " @@ -190,7 +189,7 @@ Firestore* Firestore::CreateFirestore(App* app, } Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, - const char* db_name, + const char* database_id, InitResult* init_result_out) { InitResult init_result = CheckInitialized(*firestore->internal_); if (init_result_out) { @@ -201,7 +200,7 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, return nullptr; } - FirestoreMap::key_type key = MakeKey(firestore->app(), db_name); + FirestoreMap::key_type key = MakeKey(firestore->app(), database_id); FirestoreCache()->emplace(key, firestore); return firestore; @@ -224,7 +223,6 @@ Firestore::Firestore(FirestoreInternal* internal) if (internal_->initialized()) { CleanupNotifier* app_notifier = CleanupNotifier::FindByOwner(app()); assert(app_notifier); - app_notifier->RegisterObject(this, [](void* object) { Firestore* firestore = reinterpret_cast(object); LogWarning( @@ -271,7 +269,7 @@ void Firestore::DeleteInternal() { // If a Firestore is explicitly deleted, remove it from our cache. FirestoreMap::key_type key = MakeKey( - my_app, database_id != "" ? database_id.c_str() : kDefaultDatabase); + my_app, database_id == "" ? kDefaultDatabase : database_id.c_str()); FirestoreCache()->erase(key); if (g_firestores->empty()) { @@ -386,7 +384,8 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); const std::string& database_id = internal_->database_id().database_id(); - FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); + FirestoreMap::key_type key = MakeKey( + app(), database_id == "" ? kDefaultDatabase : database_id.c_str()); FirestoreCache()->erase(key); return internal_->Terminate(); From 258253e9225c14ed9d12d9cb7b78aa77fd091ea2 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 18 Apr 2023 15:25:52 -0700 Subject: [PATCH 06/24] Update validation_test.cc --- .../src/validation_test.cc | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index c33157bd5c..2e7df96e98 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -472,6 +472,117 @@ TEST_F(ValidationTest, EXPECT_NE(Firestore::GetInstance("foo"), nullptr); } +TEST_F(ValidationTest, + FirestoreGetInstanceCalledMultipleTimeReturnSameInstance) { + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance(&result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(&result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance(app(), &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance("foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance("foo", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance(app(), "foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), "foo", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } +} + +TEST_F(ValidationTest, + differentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance(&result1); + EXPECT_EQ(kInitResultSuccess, result1); + + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), &result2); + EXPECT_EQ(kInitResultSuccess, result2); + + InitResult result3; + Firestore* instance3 = Firestore::GetInstance("(default)", &result3); + EXPECT_EQ(kInitResultSuccess, result3); + + InitResult result4; + Firestore* instance4 = Firestore::GetInstance(app(), "(default)", &result4); + EXPECT_EQ(kInitResultSuccess, result4); + + EXPECT_EQ(result1, result2); + EXPECT_EQ(result1, result3); + EXPECT_EQ(result1, result4); + EXPECT_EQ(result2, result3); + EXPECT_EQ(result2, result4); + EXPECT_EQ(result3, result4); +} + +TEST_F( + ValidationTest, + differentFirestoreGetInstanceWithSameDatabaseNameShouldGetSameFirestoreInstance) { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance("foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), "foo", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + + EXPECT_EQ(result1, result2); +} + +TEST_F( + ValidationTest, + differentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance("foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance("bar", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance(app(), "foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), "bar", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } + { + InitResult result1; + Firestore* instance1 = Firestore::GetInstance("foo", &result1); + EXPECT_EQ(kInitResultSuccess, result1); + InitResult result2; + Firestore* instance2 = Firestore::GetInstance(app(), "bar", &result2); + EXPECT_EQ(kInitResultSuccess, result2); + EXPECT_EQ(result1, result2); + } +} TEST_F(ValidationTest, CollectionPathsMustBeOddLength) { Firestore* db = TestFirestore(); DocumentReference base_document = db->Document("foo/bar"); From dba935156c4ec6a6d28ca2246535a8aedaa3eca1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 18 Apr 2023 17:01:39 -0700 Subject: [PATCH 07/24] remove getInstance success check --- .../src/validation_test.cc | 120 ++++++------------ 1 file changed, 40 insertions(+), 80 deletions(-) diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index 2e7df96e98..5768375b6e 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -475,112 +475,72 @@ TEST_F(ValidationTest, TEST_F(ValidationTest, FirestoreGetInstanceCalledMultipleTimeReturnSameInstance) { { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance(&result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(&result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance(); + EXPECT_EQ(instance1, instance2); } { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance(app(), &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance(app()); + Firestore* instance2 = Firestore::GetInstance(app()); + EXPECT_EQ(instance1, instance2); } { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance("foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance("foo", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance("foo"); + EXPECT_EQ(instance1, instance2); } { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance(app(), "foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), "foo", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance(app(), "foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "foo"); + EXPECT_EQ(instance1, instance2); } } TEST_F(ValidationTest, differentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance(&result1); - EXPECT_EQ(kInitResultSuccess, result1); - - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), &result2); - EXPECT_EQ(kInitResultSuccess, result2); - - InitResult result3; - Firestore* instance3 = Firestore::GetInstance("(default)", &result3); - EXPECT_EQ(kInitResultSuccess, result3); - - InitResult result4; - Firestore* instance4 = Firestore::GetInstance(app(), "(default)", &result4); - EXPECT_EQ(kInitResultSuccess, result4); - - EXPECT_EQ(result1, result2); - EXPECT_EQ(result1, result3); - EXPECT_EQ(result1, result4); - EXPECT_EQ(result2, result3); - EXPECT_EQ(result2, result4); - EXPECT_EQ(result3, result4); + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance(app()); + Firestore* instance3 = Firestore::GetInstance("(default)"); + Firestore* instance4 = Firestore::GetInstance(app(), "(default)"); + + EXPECT_EQ(instance1, instance2); + EXPECT_EQ(instance1, instance3); + EXPECT_EQ(instance1, instance4); + EXPECT_EQ(instance2, instance3); + EXPECT_EQ(instance2, instance4); + EXPECT_EQ(instance3, instance4); } TEST_F( ValidationTest, differentFirestoreGetInstanceWithSameDatabaseNameShouldGetSameFirestoreInstance) { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance("foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), "foo", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "foo"); + EXPECT_EQ(instance1, instance2); } TEST_F( ValidationTest, differentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance("foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance("bar", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance(); + Firestore* instance2 = Firestore::GetInstance("bar"); + EXPECT_NE(instance1, instance2); + } + { + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance("bar"); + EXPECT_NE(instance1, instance2); } { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance(app(), "foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), "bar", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance(app(), "foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "bar"); + EXPECT_NE(instance1, instance2); } { - InitResult result1; - Firestore* instance1 = Firestore::GetInstance("foo", &result1); - EXPECT_EQ(kInitResultSuccess, result1); - InitResult result2; - Firestore* instance2 = Firestore::GetInstance(app(), "bar", &result2); - EXPECT_EQ(kInitResultSuccess, result2); - EXPECT_EQ(result1, result2); + Firestore* instance1 = Firestore::GetInstance("foo"); + Firestore* instance2 = Firestore::GetInstance(app(), "bar"); + EXPECT_NE(instance1, instance2); } } TEST_F(ValidationTest, CollectionPathsMustBeOddLength) { From bdf4a1c81dd0027e21d337c902098e13d2335d60 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 19 Apr 2023 12:27:29 -0700 Subject: [PATCH 08/24] update test firestore create function --- .../src/firestore_integration_test.h | 2 +- .../src/util/integration_test_util.cc | 10 +++------- firestore/src/common/firestore.cc | 8 ++++++-- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 249fbe3b0d..e45252247f 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -49,7 +49,7 @@ const int kCheckIntervalMillis = 100; // The timeout of waiting for a Future or a listener. const int kTimeOutMillis = 15000; -FirestoreInternal* CreateTestFirestoreInternal(App* app); +FirestoreInternal* CreateTestFirestoreInternal(App* app, const char* database_id = kDefaultDatabase); App* GetApp(); App* GetApp(const char* name, const std::string& override_project_id); diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index d010b64d9d..93d536d480 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -35,7 +35,7 @@ namespace firestore { struct TestFriend { static FirestoreInternal* CreateTestFirestoreInternal( - App* app, const char* database_id = kDefaultDatabase) { + App* app, const char* database_id) { #if !defined(__ANDROID__) return new FirestoreInternal( app, database_id, @@ -80,12 +80,8 @@ App* GetApp(const char* name, const std::string& override_project_id) { App* GetApp() { return GetApp(/*name=*/nullptr, /*project_id=*/""); } -FirestoreInternal* CreateTestFirestoreInternal(App* app) { - return TestFriend::CreateTestFirestoreInternal(app); -} - -FirestoreInternal* CreateTestFirestoreInternal(App* app, - const char* database_id) { +FirestoreInternal* CreateTestFirestoreInternal( + App* app, const char* database_id = kDefaultDatabase) { return TestFriend::CreateTestFirestoreInternal(app, database_id); } diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index f88bf910e1..8877dda84e 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include "app/meta/move.h" #include "app/src/cleanup_notifier.h" @@ -163,8 +164,10 @@ Firestore* Firestore::GetInstance(App* app, MutexLock lock(*g_firestores_lock); Firestore* from_cache = FindFirestoreInCache(app, db_name, init_result_out); - + std::cout<<"FirestoreCache db_name:"<internal_); if (init_result_out) { *init_result_out = init_result; @@ -202,7 +207,6 @@ Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, FirestoreMap::key_type key = MakeKey(firestore->app(), database_id); FirestoreCache()->emplace(key, firestore); - return firestore; } From 59e07101fc2ccaf6900e1fa8cf47dd4cb49e1bc3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 19 Apr 2023 13:49:55 -0700 Subject: [PATCH 09/24] Skip the integration test --- .../src/firestore_integration_test.h | 2 +- .../src/firestore_test.cc | 50 +++++++++++++++++++ .../src/integration_test.cc | 2 + .../src/util/integration_test_util.cc | 11 ++-- firestore/src/common/firestore.cc | 6 --- 5 files changed, 58 insertions(+), 13 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index e45252247f..249fbe3b0d 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -49,7 +49,7 @@ const int kCheckIntervalMillis = 100; // The timeout of waiting for a Future or a listener. const int kTimeOutMillis = 15000; -FirestoreInternal* CreateTestFirestoreInternal(App* app, const char* database_id = kDefaultDatabase); +FirestoreInternal* CreateTestFirestoreInternal(App* app); App* GetApp(); App* GetApp(const char* name, const std::string& override_project_id); diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 0569448f7d..844fd82c91 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -87,6 +87,20 @@ TEST_F(FirestoreIntegrationTest, GetInstance) { delete auth; } +TEST_F(FirestoreIntegrationTest, GetInstanceWithNamedDatabase) { + App* app = this->app(); + EXPECT_NE(nullptr, app); + + InitResult result; + Firestore* instance = Firestore::GetInstance(app,"foo", &result); + EXPECT_EQ(kInitResultSuccess, result); + EXPECT_NE(nullptr, instance); + EXPECT_EQ(app, instance->app()); + // TODO(Mila): check the database name is "foo". + + delete instance; +} + // Sanity test for stubs. TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { Firestore* db = TestFirestore(); @@ -1348,6 +1362,42 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { delete db1; } +TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { + // TODO(Mila): Run this test against emulator. + GTEST_SKIP(); + + App* app = App::GetInstance(); + InitResult init_result; + Firestore* db1 = Firestore::GetInstance(app,"test-db", &init_result); + ASSERT_EQ(kInitResultSuccess, init_result); + + // Create a document that we can use for verification later. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + + // Terminate `db1` so that it will be removed from the instance cache. + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + // Verify that GetInstance() returns a new instance since the old instance has + // been terminated. + Firestore* db2 = Firestore::GetInstance(app,"test-db", &init_result); + ASSERT_EQ(kInitResultSuccess, init_result); + EXPECT_NE(db1, db2); + + // Verify that the new instance points to the same database by verifying that + // the document created with the old instance exists in the new instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); + ASSERT_NE(snapshot2, nullptr); + EXPECT_TRUE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + + delete db2; + delete db1; +} + TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); diff --git a/firestore/integration_test_internal/src/integration_test.cc b/firestore/integration_test_internal/src/integration_test.cc index 13b36d59f6..22e8165f2e 100644 --- a/firestore/integration_test_internal/src/integration_test.cc +++ b/firestore/integration_test_internal/src/integration_test.cc @@ -236,6 +236,7 @@ void FirebaseFirestoreBasicTest::TearDown() { FirebaseTest::TearDown(); } +// TODO(Mila): initialize named DB void FirebaseFirestoreBasicTest::InitializeFirestore() { LogDebug("Initializing Firebase Firestore."); @@ -261,6 +262,7 @@ void FirebaseFirestoreBasicTest::InitializeFirestore() { initialized_ = true; } +// TODO(Mila): terminate named DB void FirebaseFirestoreBasicTest::TerminateFirestore() { if (!initialized_) return; diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index 93d536d480..b8e6b7fe23 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -34,11 +34,11 @@ namespace firebase { namespace firestore { struct TestFriend { - static FirestoreInternal* CreateTestFirestoreInternal( - App* app, const char* database_id) { + static FirestoreInternal* CreateTestFirestoreInternal(App* app) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, database_id, + app, + /*database_id=*/"(default)", // TODO(Mila): use dynamic database ID absl::make_unique(), absl::make_unique()); #else @@ -80,9 +80,8 @@ App* GetApp(const char* name, const std::string& override_project_id) { App* GetApp() { return GetApp(/*name=*/nullptr, /*project_id=*/""); } -FirestoreInternal* CreateTestFirestoreInternal( - App* app, const char* database_id = kDefaultDatabase) { - return TestFriend::CreateTestFirestoreInternal(app, database_id); +FirestoreInternal* CreateTestFirestoreInternal(App* app) { + return TestFriend::CreateTestFirestoreInternal(app); } } // namespace firestore diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 8877dda84e..85285717d6 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -19,7 +19,6 @@ #include #include #include -#include #include "app/meta/move.h" #include "app/src/cleanup_notifier.h" @@ -164,10 +163,7 @@ Firestore* Firestore::GetInstance(App* app, MutexLock lock(*g_firestores_lock); Firestore* from_cache = FindFirestoreInCache(app, db_name, init_result_out); - std::cout<<"FirestoreCache db_name:"<internal_); if (init_result_out) { *init_result_out = init_result; From 6433b907f95470239d210ab57030fbbb00584530 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 19 Apr 2023 13:51:49 -0700 Subject: [PATCH 10/24] format --- .../integration_test_internal/src/firestore_test.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 844fd82c91..e8fcc4fe24 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -92,7 +92,7 @@ TEST_F(FirestoreIntegrationTest, GetInstanceWithNamedDatabase) { EXPECT_NE(nullptr, app); InitResult result; - Firestore* instance = Firestore::GetInstance(app,"foo", &result); + Firestore* instance = Firestore::GetInstance(app, "foo", &result); EXPECT_EQ(kInitResultSuccess, result); EXPECT_NE(nullptr, instance); EXPECT_EQ(app, instance->app()); @@ -1362,13 +1362,14 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { delete db1; } -TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { +TEST_F(FirestoreIntegrationTest, + RestartFirestoreLeadsToNewNamedDatabaseInstance) { // TODO(Mila): Run this test against emulator. GTEST_SKIP(); App* app = App::GetInstance(); InitResult init_result; - Firestore* db1 = Firestore::GetInstance(app,"test-db", &init_result); + Firestore* db1 = Firestore::GetInstance(app, "test-db", &init_result); ASSERT_EQ(kInitResultSuccess, init_result); // Create a document that we can use for verification later. @@ -1381,7 +1382,7 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewNamedDatabaseInstance // Verify that GetInstance() returns a new instance since the old instance has // been terminated. - Firestore* db2 = Firestore::GetInstance(app,"test-db", &init_result); + Firestore* db2 = Firestore::GetInstance(app, "test-db", &init_result); ASSERT_EQ(kInitResultSuccess, init_result); EXPECT_NE(db1, db2); From 51f8becf8140ff743cc21bc05df0b83f01cd11ed Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Apr 2023 12:29:48 -0700 Subject: [PATCH 11/24] test firestore terminate() function --- .../integration_test_internal/src/firestore_test.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index e8fcc4fe24..530cee2640 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1304,6 +1304,16 @@ TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS +TEST_F(FirestoreIntegrationTest, canTerminateFirestoreInstance) { + App* app = App::GetInstance(); + InitResult init_result; + Firestore* db = Firestore::GetInstance(app, "foo", &init_result); + ASSERT_EQ(kInitResultSuccess, init_result); + + EXPECT_THAT(db->Terminate(), FutureSucceeds()); + delete db; +} + TEST_F(FirestoreIntegrationTest, MaintainsPersistenceAfterRestarting) { Firestore* db = TestFirestore(); App* app = db->app(); From 86cced1216c6c3850e242f02afa0142c3ce8293c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 20 Apr 2023 14:14:12 -0700 Subject: [PATCH 12/24] change database_id reference to value --- firestore/src/common/firestore.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 85285717d6..85768cef31 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -241,7 +241,7 @@ void Firestore::DeleteInternal() { if (!internal_) return; App* my_app = app(); - const std::string& database_id = internal_->database_id().database_id(); + const std::string database_id = internal_->database_id().database_id(); // Only need to unregister if internal_ is initialized. if (internal_->initialized()) { @@ -266,8 +266,7 @@ void Firestore::DeleteInternal() { internal_ = nullptr; // If a Firestore is explicitly deleted, remove it from our cache. - FirestoreMap::key_type key = MakeKey( - my_app, database_id == "" ? kDefaultDatabase : database_id.c_str()); + FirestoreMap::key_type key = MakeKey(my_app, database_id.c_str()); FirestoreCache()->erase(key); if (g_firestores->empty()) { @@ -382,8 +381,7 @@ Future Firestore::EnableNetwork() { Future Firestore::Terminate() { if (!internal_) return FailedFuture(); const std::string& database_id = internal_->database_id().database_id(); - FirestoreMap::key_type key = MakeKey( - app(), database_id == "" ? kDefaultDatabase : database_id.c_str()); + FirestoreMap::key_type key = MakeKey(app(), database_id.c_str()); FirestoreCache()->erase(key); return internal_->Terminate(); From b19d9a61ec971113860ae03e642b7d379af634f2 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Tue, 25 Apr 2023 15:37:15 -0700 Subject: [PATCH 13/24] Apply suggestions from code review Co-authored-by: Tom Andersen --- firestore/integration_test_internal/src/firestore_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 530cee2640..77a90a876b 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1304,7 +1304,7 @@ TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, canTerminateFirestoreInstance) { +TEST_F(FirestoreIntegrationTest, CanTerminateFirestoreInstance) { App* app = App::GetInstance(); InitResult init_result; Firestore* db = Firestore::GetInstance(app, "foo", &init_result); From f5370f1f0775531c8c4bc033628e605310473fca Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 1 May 2023 12:46:44 -0400 Subject: [PATCH 14/24] test against emulator --- .../src/firestore_integration_test.h | 1 + .../src/firestore_test.cc | 30 +++++++++++++++---- .../src/util/locate_emulator.cc | 9 ++++++ .../src/util/locate_emulator.h | 2 ++ firestore/src/include/firebase/firestore.h | 1 + firestore/src/main/firestore_main.h | 2 ++ 6 files changed, 39 insertions(+), 6 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 249fbe3b0d..782a89299f 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -232,6 +232,7 @@ class FirestoreDebugLogEnabler { // Note it keeps a cache of created Firestore instances, and is thread-unsafe. class FirestoreIntegrationTest : public testing::Test { friend class TransactionTester; + friend class FirestoreInternal; public: FirestoreIntegrationTest(); diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 77a90a876b..eb91dd5438 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -40,11 +40,13 @@ #include "util/future_test_util.h" #if !defined(__ANDROID__) #include "Firestore/core/src/util/autoid.h" +#include "firestore/src/main/firestore_main.h" #else #include "android/util_autoid.h" #endif // !defined(__ANDROID__) #include "Firestore/core/src/util/firestore_exceptions.h" #include "firebase_test_framework.h" +#include "util/locate_emulator.h" // These test cases are in sync with native iOS client SDK test // Firestore/Example/Tests/Integration/API/FIRDatabaseTests.mm @@ -60,7 +62,17 @@ using ::firebase::auth::Auth; using ::testing::ContainerEq; using ::testing::HasSubstr; -TEST_F(FirestoreIntegrationTest, GetInstance) { + +class FirestoreTest : public FirestoreIntegrationTest { + protected: + const std::string GetFirestoreDatabaseId(Firestore* firestore) { +// return ""; + return firestore->internal_->database_id().database_id(); + } +}; + + +TEST_F(FirestoreTest, GetInstance) { // Create App. App* app = this->app(); EXPECT_NE(nullptr, app); @@ -87,7 +99,7 @@ TEST_F(FirestoreIntegrationTest, GetInstance) { delete auth; } -TEST_F(FirestoreIntegrationTest, GetInstanceWithNamedDatabase) { +TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { App* app = this->app(); EXPECT_NE(nullptr, app); @@ -96,11 +108,13 @@ TEST_F(FirestoreIntegrationTest, GetInstanceWithNamedDatabase) { EXPECT_EQ(kInitResultSuccess, result); EXPECT_NE(nullptr, instance); EXPECT_EQ(app, instance->app()); - // TODO(Mila): check the database name is "foo". + EXPECT_EQ(GetFirestoreDatabaseId(instance), "foo"); delete instance; } +// TODO(Mila): change test names below to subclass + // Sanity test for stubs. TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { Firestore* db = TestFirestore(); @@ -1374,25 +1388,29 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { - // TODO(Mila): Run this test against emulator. - GTEST_SKIP(); + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if(!firestore::isUsingFirestoreEmulator()){ + GTEST_SKIP(); + } App* app = App::GetInstance(); InitResult init_result; Firestore* db1 = Firestore::GetInstance(app, "test-db", &init_result); + firestore::LocateEmulator(db1); ASSERT_EQ(kInitResultSuccess, init_result); // Create a document that we can use for verification later. DocumentReference doc1 = db1->Collection("abc").Document(); const std::string doc_path = doc1.path(); EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); - + std::cout<<"======="<Terminate(), FutureSucceeds()); // Verify that GetInstance() returns a new instance since the old instance has // been terminated. Firestore* db2 = Firestore::GetInstance(app, "test-db", &init_result); + firestore::LocateEmulator(db2); ASSERT_EQ(kInitResultSuccess, init_result); EXPECT_NE(db1, db2); diff --git a/firestore/integration_test_internal/src/util/locate_emulator.cc b/firestore/integration_test_internal/src/util/locate_emulator.cc index 89f7470e1d..ecb036fff4 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.cc +++ b/firestore/integration_test_internal/src/util/locate_emulator.cc @@ -30,6 +30,7 @@ void LocateEmulator(Firestore* db) { return; } + #if defined(__ANDROID__) // Special IP to access the hosting OS from Android Emulator. std::string local_host = "10.0.2.2"; @@ -53,5 +54,13 @@ void LocateEmulator(Firestore* db) { db->set_settings(settings); } +// Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR +bool isUsingFirestoreEmulator() { + // Use emulator as long as this env variable is set, regardless its value. + if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { + return false; + } + return true; +} } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/locate_emulator.h b/firestore/integration_test_internal/src/util/locate_emulator.h index 99225985db..27ce5f4bc6 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.h +++ b/firestore/integration_test_internal/src/util/locate_emulator.h @@ -24,6 +24,8 @@ namespace firestore { // Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR void LocateEmulator(Firestore* db); +bool isUsingFirestoreEmulator(); + } // namespace firestore } // namespace firebase diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index 9c745a2127..3f9558789a 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -491,6 +491,7 @@ class Firestore { friend class IncludesTest; template friend struct CleanupFn; + friend class FirestoreTest; friend class csharp::ApiHeaders; friend class csharp::TransactionManager; diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 4e7f164a58..f654ce1f3f 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -135,6 +135,8 @@ class FirestoreInternal { private: friend class TestFriend; + friend class FirestoreTest; + enum class AsyncApi { kEnableNetwork = 0, From 63a7f7cc9cb955d9e6abe4e3da376d678df41e61 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 1 May 2023 14:30:41 -0400 Subject: [PATCH 15/24] format --- .../src/firestore_integration_test.h | 1 - .../src/firestore_test.cc | 160 ++++++++---------- .../src/util/locate_emulator.cc | 7 +- .../src/util/locate_emulator.h | 3 +- firestore/src/main/firestore_main.h | 1 - 5 files changed, 80 insertions(+), 92 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 782a89299f..249fbe3b0d 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -232,7 +232,6 @@ class FirestoreDebugLogEnabler { // Note it keeps a cache of created Firestore instances, and is thread-unsafe. class FirestoreIntegrationTest : public testing::Test { friend class TransactionTester; - friend class FirestoreInternal; public: FirestoreIntegrationTest(); diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index eb91dd5438..aa2248e695 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -62,16 +62,13 @@ using ::firebase::auth::Auth; using ::testing::ContainerEq; using ::testing::HasSubstr; - class FirestoreTest : public FirestoreIntegrationTest { protected: const std::string GetFirestoreDatabaseId(Firestore* firestore) { -// return ""; return firestore->internal_->database_id().database_id(); } }; - TEST_F(FirestoreTest, GetInstance) { // Create App. App* app = this->app(); @@ -113,10 +110,8 @@ TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { delete instance; } -// TODO(Mila): change test names below to subclass - // Sanity test for stubs. -TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { +TEST_F(FirestoreTest, TestCanCreateCollectionAndDocumentReferences) { Firestore* db = TestFirestore(); CollectionReference c = db->Collection("a/b/c").Document("d").Parent(); DocumentReference d = db->Document("a/b").Collection("c/d/e").Parent(); @@ -130,7 +125,7 @@ TEST_F(FirestoreIntegrationTest, TestCanCreateCollectionAndDocumentReferences) { // If any of these assert, the test will fail. } -TEST_F(FirestoreIntegrationTest, TestCanReadNonExistentDocuments) { +TEST_F(FirestoreTest, TestCanReadNonExistentDocuments) { DocumentReference doc = Collection("rooms").Document(); DocumentSnapshot snap = ReadDocument(doc); @@ -138,7 +133,7 @@ TEST_F(FirestoreIntegrationTest, TestCanReadNonExistentDocuments) { EXPECT_THAT(snap.GetData(), ContainerEq(MapFieldValue())); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateAnExistingDocument) { +TEST_F(FirestoreTest, TestCanUpdateAnExistingDocument) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -159,7 +154,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateAnExistingDocument) { {"email", FieldValue::String("new@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateAnUnknownDocument) { +TEST_F(FirestoreTest, TestCanUpdateAnUnknownDocument) { DocumentReference writer_reference = TestFirestore("writer")->Collection("collection").Document(); DocumentReference reader_reference = TestFirestore("reader") @@ -193,7 +188,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateAnUnknownDocument) { EXPECT_FALSE(reader_snapshot.metadata().is_from_cache()); } -TEST_F(FirestoreIntegrationTest, TestCanOverwriteAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanOverwriteAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -214,8 +209,7 @@ TEST_F(FirestoreIntegrationTest, TestCanOverwriteAnExistingDocumentUsingSet) { FieldValue::Map({{"name", FieldValue::String("Sebastian")}})}})); } -TEST_F(FirestoreIntegrationTest, - TestCanMergeDataWithAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanMergeDataWithAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -240,7 +234,7 @@ TEST_F(FirestoreIntegrationTest, {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanMergeServerTimestamps) { +TEST_F(FirestoreTest, TestCanMergeServerTimestamps) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{{"untouched", FieldValue::Boolean(true)}})); Await(document.Set( @@ -254,7 +248,7 @@ TEST_F(FirestoreIntegrationTest, TestCanMergeServerTimestamps) { EXPECT_TRUE(snapshot.Get("nested.time").is_timestamp()); } -TEST_F(FirestoreIntegrationTest, TestCanMergeEmptyObject) { +TEST_F(FirestoreTest, TestCanMergeEmptyObject) { DocumentReference document = Document(); EventAccumulator accumulator; ListenerRegistration registration = @@ -290,7 +284,7 @@ TEST_F(FirestoreIntegrationTest, TestCanMergeEmptyObject) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMerge) { +TEST_F(FirestoreTest, TestCanDeleteFieldUsingMerge) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -315,7 +309,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMerge) { EXPECT_FALSE(snapshot.Get("nested.foo").is_valid()); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMergeFields) { +TEST_F(FirestoreTest, TestCanDeleteFieldUsingMergeFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -342,7 +336,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteFieldUsingMergeFields) { FieldValue::Map({{"untouched", FieldValue::Boolean(true)}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSetServerTimestampsUsingMergeFields) { +TEST_F(FirestoreTest, TestCanSetServerTimestampsUsingMergeFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -363,7 +357,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSetServerTimestampsUsingMergeFields) { EXPECT_TRUE(snapshot.Get("nested.foo").is_timestamp()); } -TEST_F(FirestoreIntegrationTest, TestMergeReplacesArrays) { +TEST_F(FirestoreTest, TestMergeReplacesArrays) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"untouched", FieldValue::Boolean(true)}, @@ -391,8 +385,7 @@ TEST_F(FirestoreIntegrationTest, TestMergeReplacesArrays) { {{"data", FieldValue::String("new")}})})}})); } -TEST_F(FirestoreIntegrationTest, - TestCanDeepMergeDataWithAnExistingDocumentUsingSet) { +TEST_F(FirestoreTest, TestCanDeepMergeDataWithAnExistingDocumentUsingSet) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"owner.data", @@ -419,7 +412,7 @@ TEST_F(FirestoreIntegrationTest, #if defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS // TODO(b/136012313): iOS currently doesn't rethrow native exceptions as C++ // exceptions. -TEST_F(FirestoreIntegrationTest, TestFieldMaskCannotContainMissingFields) { +TEST_F(FirestoreTest, TestFieldMaskCannotContainMissingFields) { DocumentReference document = Collection("rooms").Document(); try { document.Set(MapFieldValue{{"desc", FieldValue::String("NewDescription")}}, @@ -434,7 +427,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldMaskCannotContainMissingFields) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, TestFieldsNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldsNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -456,7 +449,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldsNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestFieldDeletesNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldDeletesNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -478,7 +471,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldDeletesNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestFieldTransformsNotInFieldMaskAreIgnored) { +TEST_F(FirestoreTest, TestFieldTransformsNotInFieldMaskAreIgnored) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -500,7 +493,7 @@ TEST_F(FirestoreIntegrationTest, TestFieldTransformsNotInFieldMaskAreIgnored) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSetEmptyFieldMask) { +TEST_F(FirestoreTest, TestCanSetEmptyFieldMask) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -521,7 +514,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSetEmptyFieldMask) { {"email", FieldValue::String("abc@xyz.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { +TEST_F(FirestoreTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -546,7 +539,7 @@ TEST_F(FirestoreIntegrationTest, TestCanSpecifyFieldsMultipleTimesInFieldMask) { {"email", FieldValue::String("new@new.com")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanDeleteAFieldWithAnUpdate) { +TEST_F(FirestoreTest, TestCanDeleteAFieldWithAnUpdate) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"desc", FieldValue::String("Description")}, @@ -563,7 +556,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDeleteAFieldWithAnUpdate) { FieldValue::Map({{"name", FieldValue::String("Jonny")}})}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithDots) { +TEST_F(FirestoreTest, TestCanUpdateFieldsWithDots) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{{"a.b", FieldValue::String("old")}, {"c.d", FieldValue::String("old")}, @@ -578,7 +571,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithDots) { {"e.f", FieldValue::String("old")}})); } -TEST_F(FirestoreIntegrationTest, TestCanUpdateNestedFields) { +TEST_F(FirestoreTest, TestCanUpdateNestedFields) { DocumentReference document = Collection("rooms").Document("eros"); Await(document.Set(MapFieldValue{ {"a", FieldValue::Map({{"b", FieldValue::String("old")}})}, @@ -597,7 +590,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateNestedFields) { // Verify that multiple deletes in a single update call work. // https://github.com/firebase/quickstart-unity/issues/882 -TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithMultipleDeletes) { +TEST_F(FirestoreTest, TestCanUpdateFieldsWithMultipleDeletes) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{{"key1", FieldValue::String("value1")}, {"key2", FieldValue::String("value2")}, @@ -614,7 +607,7 @@ TEST_F(FirestoreIntegrationTest, TestCanUpdateFieldsWithMultipleDeletes) { {"key4", FieldValue::String("value4")}})); } -TEST_F(FirestoreIntegrationTest, TestDeleteDocument) { +TEST_F(FirestoreTest, TestDeleteDocument) { DocumentReference document = Collection("rooms").Document("eros"); WriteDocument(document, MapFieldValue{{"value", FieldValue::String("bar")}}); DocumentSnapshot snapshot = ReadDocument(document); @@ -627,7 +620,7 @@ TEST_F(FirestoreIntegrationTest, TestDeleteDocument) { EXPECT_FALSE(snapshot.exists()); } -TEST_F(FirestoreIntegrationTest, TestCannotUpdateNonexistentDocument) { +TEST_F(FirestoreTest, TestCannotUpdateNonexistentDocument) { DocumentReference document = Collection("rooms").Document(); Future future = document.Update(MapFieldValue{{"owner", FieldValue::String("abc")}}); @@ -638,7 +631,7 @@ TEST_F(FirestoreIntegrationTest, TestCannotUpdateNonexistentDocument) { EXPECT_FALSE(snapshot.exists()); } -TEST_F(FirestoreIntegrationTest, TestCanRetrieveNonexistentDocument) { +TEST_F(FirestoreTest, TestCanRetrieveNonexistentDocument) { DocumentReference document = Collection("rooms").Document(); DocumentSnapshot snapshot = ReadDocument(document); EXPECT_FALSE(snapshot.exists()); @@ -651,7 +644,7 @@ TEST_F(FirestoreIntegrationTest, TestCanRetrieveNonexistentDocument) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, +TEST_F(FirestoreTest, TestAddingToACollectionYieldsTheCorrectDocumentReference) { DocumentReference document = Collection("rooms").Document(); Await(document.Set(MapFieldValue{{"foo", FieldValue::Double(1.0)}})); @@ -661,8 +654,7 @@ TEST_F(FirestoreIntegrationTest, ContainerEq(MapFieldValue{{"foo", FieldValue::Double(1.0)}})); } -TEST_F(FirestoreIntegrationTest, - TestSnapshotsInSyncListenerFiresAfterListenersInSync) { +TEST_F(FirestoreTest, TestSnapshotsInSyncListenerFiresAfterListenersInSync) { class TestData { public: void AddEvent(const std::string& event) { @@ -751,7 +743,7 @@ TEST_F(FirestoreIntegrationTest, sync_registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQueriesAreValidatedOnClient) { +TEST_F(FirestoreTest, TestQueriesAreValidatedOnClient) { // NOTE: Failure cases are validated in ValidationTest. CollectionReference collection = Collection(); Query query = @@ -783,7 +775,7 @@ TEST_F(FirestoreIntegrationTest, TestQueriesAreValidatedOnClient) { // The test harness will generate Java JUnit test regardless whether this is // inside a #if or not. So we move #if inside instead of enclose the whole case. -TEST_F(FirestoreIntegrationTest, TestListenCanBeCalledMultipleTimes) { +TEST_F(FirestoreTest, TestListenCanBeCalledMultipleTimes) { class TestData { public: void SetDocumentSnapshot(const DocumentSnapshot& document_snapshot) { @@ -831,7 +823,7 @@ TEST_F(FirestoreIntegrationTest, TestListenCanBeCalledMultipleTimes) { ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsNonExistent) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsNonExistent) { DocumentReference document = Collection("rooms").Document(); TestEventListener listener("TestNonExistent"); ListenerRegistration registration = @@ -843,7 +835,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsNonExistent) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForAdd) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForAdd) { DocumentReference document = Collection("rooms").Document(); TestEventListener listener("TestForAdd"); ListenerRegistration registration = @@ -867,7 +859,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForAdd) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForChange) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForChange) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -901,7 +893,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForChange) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForDelete) { +TEST_F(FirestoreTest, TestDocumentSnapshotEventsForDelete) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -925,7 +917,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotEventsForDelete) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotErrorReporting) { +TEST_F(FirestoreTest, TestDocumentSnapshotErrorReporting) { DocumentReference document = Collection("col").Document("__badpath__"); TestEventListener listener("TestBadPath"); ListenerRegistration registration = @@ -938,7 +930,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentSnapshotErrorReporting) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForAdd) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForAdd) { CollectionReference collection = Collection(); DocumentReference document = collection.Document(); TestEventListener listener("TestForCollectionAdd"); @@ -965,7 +957,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForAdd) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForChange) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForChange) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -999,7 +991,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForChange) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForDelete) { +TEST_F(FirestoreTest, TestQuerySnapshotEventsForDelete) { CollectionReference collection = Collection(std::map{ {"doc", MapFieldValue{{"a", FieldValue::Double(1.0)}}}}); @@ -1023,7 +1015,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotEventsForDelete) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestQuerySnapshotErrorReporting) { +TEST_F(FirestoreTest, TestQuerySnapshotErrorReporting) { CollectionReference collection = Collection("a").Document("__badpath__").Collection("b"); TestEventListener listener("TestBadPath"); @@ -1037,8 +1029,7 @@ TEST_F(FirestoreIntegrationTest, TestQuerySnapshotErrorReporting) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, - TestMetadataOnlyChangesAreNotFiredWhenNoOptionsProvided) { +TEST_F(FirestoreTest, TestMetadataOnlyChangesAreNotFiredWhenNoOptionsProvided) { DocumentReference document = Collection().Document(); TestEventListener listener("TestForNoMetadataOnlyChanges"); ListenerRegistration registration = listener.AttachTo(&document); @@ -1055,7 +1046,7 @@ TEST_F(FirestoreIntegrationTest, registration.Remove(); } -TEST_F(FirestoreIntegrationTest, TestDocumentReferenceExposesFirestore) { +TEST_F(FirestoreTest, TestDocumentReferenceExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Document("foo/bar").firestore()); // TODO(varconst): use the commented out check above. @@ -1069,19 +1060,19 @@ TEST_F(FirestoreIntegrationTest, TestDocumentReferenceExposesFirestore) { EXPECT_NE(nullptr, db->Document("foo/bar").firestore()); } -TEST_F(FirestoreIntegrationTest, TestCollectionReferenceExposesFirestore) { +TEST_F(FirestoreTest, TestCollectionReferenceExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Collection("foo").firestore()); EXPECT_NE(nullptr, db->Collection("foo").firestore()); } -TEST_F(FirestoreIntegrationTest, TestQueryExposesFirestore) { +TEST_F(FirestoreTest, TestQueryExposesFirestore) { Firestore* db = TestFirestore(); // EXPECT_EQ(db, db->Collection("foo").Limit(5).firestore()); EXPECT_NE(nullptr, db->Collection("foo").Limit(5).firestore()); } -TEST_F(FirestoreIntegrationTest, TestDocumentReferenceEquality) { +TEST_F(FirestoreTest, TestDocumentReferenceEquality) { Firestore* db = TestFirestore(); DocumentReference document = db->Document("foo/bar"); EXPECT_EQ(document, db->Document("foo/bar")); @@ -1093,7 +1084,7 @@ TEST_F(FirestoreIntegrationTest, TestDocumentReferenceEquality) { EXPECT_NE(document, another_db->Document("foo/bar")); } -TEST_F(FirestoreIntegrationTest, TestQueryReferenceEquality) { +TEST_F(FirestoreTest, TestQueryReferenceEquality) { Firestore* db = TestFirestore(); Query query = db->Collection("foo").OrderBy("bar").WhereEqualTo( "baz", FieldValue::Integer(42)); @@ -1109,7 +1100,7 @@ TEST_F(FirestoreIntegrationTest, TestQueryReferenceEquality) { // So we skip the testing of two queries with different Firestore instance. } -TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionsAndDocuments) { +TEST_F(FirestoreTest, TestCanTraverseCollectionsAndDocuments) { Firestore* db = TestFirestore(); // doc path from root Firestore. @@ -1125,7 +1116,7 @@ TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionsAndDocuments) { EXPECT_EQ("a/b/c/d/e", db->Document("a/b").Collection("c/d/e").path()); } -TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionAndDocumentParents) { +TEST_F(FirestoreTest, TestCanTraverseCollectionAndDocumentParents) { Firestore* db = TestFirestore(); CollectionReference collection = db->Collection("a/b/c"); EXPECT_EQ("a/b/c", collection.path()); @@ -1140,17 +1131,17 @@ TEST_F(FirestoreIntegrationTest, TestCanTraverseCollectionAndDocumentParents) { EXPECT_FALSE(invalidDoc.is_valid()); } -TEST_F(FirestoreIntegrationTest, TestCollectionId) { +TEST_F(FirestoreTest, TestCollectionId) { EXPECT_EQ("foo", TestFirestore()->Collection("foo").id()); EXPECT_EQ("baz", TestFirestore()->Collection("foo/bar/baz").id()); } -TEST_F(FirestoreIntegrationTest, TestDocumentId) { +TEST_F(FirestoreTest, TestDocumentId) { EXPECT_EQ(TestFirestore()->Document("foo/bar").id(), "bar"); EXPECT_EQ(TestFirestore()->Document("foo/bar/baz/qux").id(), "qux"); } -TEST_F(FirestoreIntegrationTest, TestCanQueueWritesWhileOffline) { +TEST_F(FirestoreTest, TestCanQueueWritesWhileOffline) { // Arrange DocumentReference document = Collection("rooms").Document("eros"); @@ -1178,7 +1169,7 @@ TEST_F(FirestoreIntegrationTest, TestCanQueueWritesWhileOffline) { EXPECT_FALSE(snapshot.metadata().is_from_cache()); } -TEST_F(FirestoreIntegrationTest, TestCanGetDocumentsWhileOffline) { +TEST_F(FirestoreTest, TestCanGetDocumentsWhileOffline) { DocumentReference document = Collection("rooms").Document(); Await(TestFirestore()->DisableNetwork()); Future future = document.Get(); @@ -1228,7 +1219,7 @@ TEST_F(FirestoreIntegrationTest, TestCanGetDocumentsWhileOffline) { // really unit tests that have to be run in integration tests setup. The // existing Objective-C and Android tests cover these cases fairly well. -TEST_F(FirestoreIntegrationTest, TestCanDisableAndEnableNetworking) { +TEST_F(FirestoreTest, TestCanDisableAndEnableNetworking) { // There's not currently a way to check if networking is in fact disabled, // so for now just test that the method is well-behaved and doesn't throw. Firestore* db = TestFirestore(); @@ -1240,7 +1231,7 @@ TEST_F(FirestoreIntegrationTest, TestCanDisableAndEnableNetworking) { } // TODO(varconst): split this test. -TEST_F(FirestoreIntegrationTest, TestToString) { +TEST_F(FirestoreTest, TestToString) { Settings settings; settings.set_host("foo.bar"); settings.set_ssl_enabled(false); @@ -1270,12 +1261,12 @@ TEST_F(FirestoreIntegrationTest, TestToString) { // TODO(wuandy): Enable this for other platforms when they can handle // exceptions. #if defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, ClientCallsAfterTerminateFails) { +TEST_F(FirestoreTest, ClientCallsAfterTerminateFails) { EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); EXPECT_THROW(Await(TestFirestore()->DisableNetwork()), std::logic_error); } -TEST_F(FirestoreIntegrationTest, NewOperationThrowsAfterFirestoreTerminate) { +TEST_F(FirestoreTest, NewOperationThrowsAfterFirestoreTerminate) { auto instance = TestFirestore(); DocumentReference reference = TestFirestore()->Document("abc/123"); Await(reference.Set({{"Field", FieldValue::Integer(100)}})); @@ -1301,7 +1292,7 @@ TEST_F(FirestoreIntegrationTest, NewOperationThrowsAfterFirestoreTerminate) { std::logic_error); } -TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { +TEST_F(FirestoreTest, TerminateCanBeCalledMultipleTimes) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); Await(reference.Set({{"Field", FieldValue::Integer(100)}})); @@ -1318,7 +1309,7 @@ TEST_F(FirestoreIntegrationTest, TerminateCanBeCalledMultipleTimes) { } #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS -TEST_F(FirestoreIntegrationTest, CanTerminateFirestoreInstance) { +TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { App* app = App::GetInstance(); InitResult init_result; Firestore* db = Firestore::GetInstance(app, "foo", &init_result); @@ -1328,7 +1319,7 @@ TEST_F(FirestoreIntegrationTest, CanTerminateFirestoreInstance) { delete db; } -TEST_F(FirestoreIntegrationTest, MaintainsPersistenceAfterRestarting) { +TEST_F(FirestoreTest, MaintainsPersistenceAfterRestarting) { Firestore* db = TestFirestore(); App* app = db->app(); DocumentReference doc = db->Collection("col1").Document("doc1"); @@ -1342,7 +1333,7 @@ TEST_F(FirestoreIntegrationTest, MaintainsPersistenceAfterRestarting) { EXPECT_TRUE(snap->exists()); } -TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { +TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { // Get App and Settings objects to use in the test. Firestore* db_template = TestFirestore("restart_firestore_new_instance_test"); App* app = db_template->app(); @@ -1386,11 +1377,10 @@ TEST_F(FirestoreIntegrationTest, RestartFirestoreLeadsToNewInstance) { delete db1; } -TEST_F(FirestoreIntegrationTest, - RestartFirestoreLeadsToNewNamedDatabaseInstance) { +TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. - if(!firestore::isUsingFirestoreEmulator()){ - GTEST_SKIP(); + if (!firestore::IsUsingFirestoreEmulator()) { + GTEST_SKIP(); } App* app = App::GetInstance(); @@ -1403,7 +1393,7 @@ TEST_F(FirestoreIntegrationTest, DocumentReference doc1 = db1->Collection("abc").Document(); const std::string doc_path = doc1.path(); EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); - std::cout<<"======="<Terminate(), FutureSucceeds()); @@ -1427,7 +1417,7 @@ TEST_F(FirestoreIntegrationTest, delete db1; } -TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) { +TEST_F(FirestoreTest, CanStopListeningAfterTerminate) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); EventAccumulator accumulator; @@ -1443,7 +1433,7 @@ TEST_F(FirestoreIntegrationTest, CanStopListeningAfterTerminate) { registration.Remove(); } -TEST_F(FirestoreIntegrationTest, WaitForPendingWritesResolves) { +TEST_F(FirestoreTest, WaitForPendingWritesResolves) { DocumentReference document = Collection("abc").Document("123"); Await(TestFirestore()->DisableNetwork()); @@ -1469,9 +1459,9 @@ TEST_F(FirestoreIntegrationTest, WaitForPendingWritesResolves) { // TODO(wuandy): This test requires to create underlying firestore instance with // a MockCredentialProvider first. -// TEST_F(FirestoreIntegrationTest, WaitForPendingWritesFailsWhenUserChanges) {} +// TEST_F(FirestoreTest, WaitForPendingWritesFailsWhenUserChanges) {} -TEST_F(FirestoreIntegrationTest, +TEST_F(FirestoreTest, WaitForPendingWritesResolvesWhenOfflineIfThereIsNoPending) { Await(TestFirestore()->DisableNetwork()); Future await_pending_writes = TestFirestore()->WaitForPendingWrites(); @@ -1482,7 +1472,7 @@ TEST_F(FirestoreIntegrationTest, EXPECT_EQ(await_pending_writes.status(), FutureStatus::kFutureStatusComplete); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceTestHarnessVerification) { +TEST_F(FirestoreTest, CanClearPersistenceTestHarnessVerification) { // Verify that TestFirestore(), DeleteFirestore(), and DeleteApp() behave how // we expect; otherwise, the tests for ClearPersistence() could yield false // positives. @@ -1505,7 +1495,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceTestHarnessVerification) { ContainerEq(MapFieldValue{{"foo", FieldValue::Integer(42)}})); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) { +TEST_F(FirestoreTest, CanClearPersistenceAfterRestarting) { Firestore* db = TestFirestore(); App* app = db->app(); const std::string app_name = app->name(); @@ -1537,7 +1527,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceAfterRestarting) { EXPECT_EQ(await_get.error(), Error::kErrorUnavailable); } -TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) { +TEST_F(FirestoreTest, CanClearPersistenceOnANewFirestoreInstance) { Firestore* db = TestFirestore(); App* app = db->app(); const std::string app_name = app->name(); @@ -1566,7 +1556,7 @@ TEST_F(FirestoreIntegrationTest, CanClearPersistenceOnANewFirestoreInstance) { EXPECT_EQ(await_get.error(), Error::kErrorUnavailable); } -TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { +TEST_F(FirestoreTest, ClearPersistenceWhileRunningFails) { // Call EnableNetwork() in order to ensure that Firestore is fully // initialized before clearing persistence. EnableNetwork() is chosen because // it is easy to call. @@ -1579,12 +1569,12 @@ TEST_F(FirestoreIntegrationTest, ClearPersistenceWhileRunningFails) { } // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, DomainObjectsReferToSameFirestoreInstance) { +TEST_F(FirestoreTest, DomainObjectsReferToSameFirestoreInstance) { EXPECT_EQ(TestFirestore(), TestFirestore()->Document("foo/bar").firestore()); EXPECT_EQ(TestFirestore(), TestFirestore()->Collection("foo").firestore()); } -TEST_F(FirestoreIntegrationTest, AuthWorks) { +TEST_F(FirestoreTest, AuthWorks) { SKIP_TEST_ON_QUICK_CHECK; // This app instance is managed by the text fixture. App* app = GetApp(); @@ -1628,7 +1618,7 @@ TEST_F(FirestoreIntegrationTest, AuthWorks) { // This test is to ensure b/172986326 doesn't regress. // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) { +TEST_F(FirestoreTest, FirestoreCanBeDeletedFromTransactionAsync) { Firestore* db = TestFirestore(); DisownFirestore(db); @@ -1650,7 +1640,7 @@ TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransactionAsync) { // This test is to ensure b/172986326 doesn't regress. // Note: this test only exists in C++. -TEST_F(FirestoreIntegrationTest, FirestoreCanBeDeletedFromTransaction) { +TEST_F(FirestoreTest, FirestoreCanBeDeletedFromTransaction) { Firestore* db = TestFirestore(); DisownFirestore(db); diff --git a/firestore/integration_test_internal/src/util/locate_emulator.cc b/firestore/integration_test_internal/src/util/locate_emulator.cc index ecb036fff4..4e6fb4ae37 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.cc +++ b/firestore/integration_test_internal/src/util/locate_emulator.cc @@ -30,7 +30,6 @@ void LocateEmulator(Firestore* db) { return; } - #if defined(__ANDROID__) // Special IP to access the hosting OS from Android Emulator. std::string local_host = "10.0.2.2"; @@ -54,9 +53,9 @@ void LocateEmulator(Firestore* db) { db->set_settings(settings); } -// Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR -bool isUsingFirestoreEmulator() { - // Use emulator as long as this env variable is set, regardless its value. +// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. +bool IsUsingFirestoreEmulator() { + // Emulator is used if the env variable is set, regardless of its value. if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) { return false; } diff --git a/firestore/integration_test_internal/src/util/locate_emulator.h b/firestore/integration_test_internal/src/util/locate_emulator.h index 27ce5f4bc6..a19ce284ea 100644 --- a/firestore/integration_test_internal/src/util/locate_emulator.h +++ b/firestore/integration_test_internal/src/util/locate_emulator.h @@ -24,7 +24,8 @@ namespace firestore { // Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR void LocateEmulator(Firestore* db); -bool isUsingFirestoreEmulator(); +// Check if firestore is using Firestore Emulator via USE_FIRESTORE_EMULATOR. +bool IsUsingFirestoreEmulator(); } // namespace firestore } // namespace firebase diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index f654ce1f3f..06593a7d03 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -137,7 +137,6 @@ class FirestoreInternal { friend class TestFriend; friend class FirestoreTest; - enum class AsyncApi { kEnableNetwork = 0, kDisableNetwork, From 4632ea017e6e9b37c3988f9c1c2fad1f8cf5e11a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 1 May 2023 15:09:21 -0400 Subject: [PATCH 16/24] enrich integration test cases --- .../src/firestore_test.cc | 81 +++++++++++++++++-- 1 file changed, 76 insertions(+), 5 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index aa2248e695..f8e1a3f245 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1384,10 +1384,8 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { } App* app = App::GetInstance(); - InitResult init_result; - Firestore* db1 = Firestore::GetInstance(app, "test-db", &init_result); + Firestore* db1 = Firestore::GetInstance(app, "test-db"); firestore::LocateEmulator(db1); - ASSERT_EQ(kInitResultSuccess, init_result); // Create a document that we can use for verification later. DocumentReference doc1 = db1->Collection("abc").Document(); @@ -1399,9 +1397,8 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { // Verify that GetInstance() returns a new instance since the old instance has // been terminated. - Firestore* db2 = Firestore::GetInstance(app, "test-db", &init_result); + Firestore* db2 = Firestore::GetInstance(app, "test-db"); firestore::LocateEmulator(db2); - ASSERT_EQ(kInitResultSuccess, init_result); EXPECT_NE(db1, db2); // Verify that the new instance points to the same database by verifying that @@ -1417,6 +1414,80 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { delete db1; } +TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!firestore::IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two DB instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = Firestore::GetInstance(app, "db1"); + firestore::LocateEmulator(db1); + + Firestore* db2 = Firestore::GetInstance(app, "db2"); + firestore::LocateEmulator(db2); + EXPECT_NE(db1, db2); + + // Create a document in the first DB instance. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + const DocumentSnapshot* snapshot1 = Await(doc1.Get()); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + + // Verify that the previously saved document only exists in the first DB + // instance by verifying that the document does not exist in the second + // instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get()); + EXPECT_FALSE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); + + delete db2; + delete db1; +} + +TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { + // TODO(Mila): Remove the emulator env check after prod supports multiDB. + if (!firestore::IsUsingFirestoreEmulator()) { + GTEST_SKIP(); + } + + // Create two DB instances in the same app. + App* app = App::GetInstance(); + Firestore* db1 = Firestore::GetInstance(app, "db1"); + firestore::LocateEmulator(db1); + + Firestore* db2 = Firestore::GetInstance(app, "db2"); + firestore::LocateEmulator(db2); + EXPECT_NE(db1, db2); + + DisableNetwork(); + DisableNetwork(); + + // Create a document in the first DB instance. + DocumentReference doc1 = db1->Collection("abc").Document(); + const std::string doc_path = doc1.path(); + EXPECT_THAT(doc1.Set({{"foo", FieldValue::String("bar")}}), FutureSucceeds()); + const DocumentSnapshot* snapshot1 = Await(doc1.Get(Source::kCache)); + EXPECT_TRUE(snapshot1->exists()); + EXPECT_THAT(snapshot1->GetData(), + ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + // Verify that the previously saved document only exists in the first DB + // instance by verifying that the document does not exist in the second + // instance. + DocumentReference doc2 = db2->Document(doc_path); + const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); + EXPECT_FALSE(snapshot2->exists()); + EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); + + delete db2; + delete db1; +} + TEST_F(FirestoreTest, CanStopListeningAfterTerminate) { auto instance = TestFirestore(); DocumentReference reference = instance->Document("abc/123"); From 3bde5184d98f32a013f619d9943f484019eb205c Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 1 May 2023 21:31:25 -0400 Subject: [PATCH 17/24] add terminate default db test case --- .../integration_test_internal/src/firestore_test.cc | 10 ++++++++++ .../integration_test_internal/src/integration_test.cc | 2 -- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index f8e1a3f245..84542da188 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1310,6 +1310,16 @@ TEST_F(FirestoreTest, TerminateCanBeCalledMultipleTimes) { #endif // defined(__ANDROID__) && FIRESTORE_HAVE_EXCEPTIONS TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { + App* app = App::GetInstance(); + InitResult init_result; + Firestore* db = Firestore::GetInstance(app, &init_result); + ASSERT_EQ(kInitResultSuccess, init_result); + + EXPECT_THAT(db->Terminate(), FutureSucceeds()); + delete db; +} + +TEST_F(FirestoreTest, CanTerminateNamedFirestoreInstance) { App* app = App::GetInstance(); InitResult init_result; Firestore* db = Firestore::GetInstance(app, "foo", &init_result); diff --git a/firestore/integration_test_internal/src/integration_test.cc b/firestore/integration_test_internal/src/integration_test.cc index 22e8165f2e..13b36d59f6 100644 --- a/firestore/integration_test_internal/src/integration_test.cc +++ b/firestore/integration_test_internal/src/integration_test.cc @@ -236,7 +236,6 @@ void FirebaseFirestoreBasicTest::TearDown() { FirebaseTest::TearDown(); } -// TODO(Mila): initialize named DB void FirebaseFirestoreBasicTest::InitializeFirestore() { LogDebug("Initializing Firebase Firestore."); @@ -262,7 +261,6 @@ void FirebaseFirestoreBasicTest::InitializeFirestore() { initialized_ = true; } -// TODO(Mila): terminate named DB void FirebaseFirestoreBasicTest::TerminateFirestore() { if (!initialized_) return; From d5491478b9b09ad36a2020ec9b77208afb6f6bd8 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 3 May 2023 11:23:37 -0400 Subject: [PATCH 18/24] Use test helper to run tests on emulator --- .../src/firestore_integration_test.cc | 17 +++-- .../src/firestore_integration_test.h | 20 +++-- .../src/firestore_test.cc | 75 ++++++++++--------- .../src/util/integration_test_util.cc | 12 +-- 4 files changed, 72 insertions(+), 52 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index df1854044f..e018028653 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -116,11 +116,17 @@ Firestore* FirestoreIntegrationTest::TestFirestore( return TestFirestoreWithProjectId(name, /*project_id=*/""); } +Firestore* FirestoreIntegrationTest::TestFirestoreWithDatabaseId( + const std::string& name, const std::string& database_id) const { + return TestFirestoreWithProjectId(name, /*project_id=*/"", database_id); +} + Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( - const std::string& name, const std::string& project_id) const { - for (const auto& entry : firestores_) { + const std::string& name, const std::string& project_id, const std::string& database_id) const { + + for (const auto& entry : firestores_) { const FirestoreInfo& firestore_info = entry.second; - if (firestore_info.name() == name) { + if (firestore_info.name() == name && firestore_info.database_id() == database_id) { return firestore_info.firestore(); } } @@ -129,9 +135,8 @@ Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( if (apps_.find(app) == apps_.end()) { apps_[app] = std::unique_ptr(app); } - - Firestore* db = new Firestore(CreateTestFirestoreInternal(app)); - firestores_[db] = FirestoreInfo(name, std::unique_ptr(db)); + Firestore* db = new Firestore(CreateTestFirestoreInternal(app,database_id)); + firestores_[db] = FirestoreInfo(name, database_id,std::unique_ptr(db)); firestore::LocateEmulator(db); return db; diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 249fbe3b0d..3ed02cd5dc 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -49,7 +49,7 @@ const int kCheckIntervalMillis = 100; // The timeout of waiting for a Future or a listener. const int kTimeOutMillis = 15000; -FirestoreInternal* CreateTestFirestoreInternal(App* app); +FirestoreInternal* CreateTestFirestoreInternal(App* app, const std::string& database_id = kDefaultDatabase); App* GetApp(); App* GetApp(const char* name, const std::string& override_project_id); @@ -254,7 +254,12 @@ class FirestoreIntegrationTest : public testing::Test { // Returns a Firestore instance for an app with the given `name`, associated // with the database with the given `project_id`. Firestore* TestFirestoreWithProjectId(const std::string& name, - const std::string& project_id) const; + const std::string& project_id, + const std::string& database_id = kDefaultDatabase) const; + + // Returns a Firestore instance for an app with the given `name`, associated + // with the database with the given `database_id`. + Firestore* TestFirestoreWithDatabaseId(const std::string& name, const std::string& database_id) const; // Deletes the given `Firestore` instance, which must have been returned by a // previous invocation of `TestFirestore()`, and removes it from the cache of @@ -419,16 +424,19 @@ class FirestoreIntegrationTest : public testing::Test { class FirestoreInfo { public: FirestoreInfo() = default; - FirestoreInfo(const std::string& name, std::unique_ptr firestore) - : name_(name), firestore_(std::move(firestore)) {} + FirestoreInfo(const std::string& name, const std::string& database_id, std::unique_ptr firestore) + : name_(name), database_id_(database_id) ,firestore_(std::move(firestore)) {} const std::string& name() const { return name_; } Firestore* firestore() const { return firestore_.get(); } - void ReleaseFirestore() { firestore_.release(); } + const std::string& database_id() const { return database_id_; } + + void ReleaseFirestore() { firestore_.release(); } private: std::string name_; - std::unique_ptr firestore_; + std::string database_id_; + std::unique_ptr firestore_; }; // The Firestore and App instance caches. diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 84542da188..a206f5a9d3 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1311,22 +1311,40 @@ TEST_F(FirestoreTest, TerminateCanBeCalledMultipleTimes) { TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { App* app = App::GetInstance(); - InitResult init_result; - Firestore* db = Firestore::GetInstance(app, &init_result); - ASSERT_EQ(kInitResultSuccess, init_result); + InitResult init_result1; + auto db1 = + std::unique_ptr(Firestore::GetInstance(app, &init_result1)); + ASSERT_EQ(kInitResultSuccess, init_result1); - EXPECT_THAT(db->Terminate(), FutureSucceeds()); - delete db; + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + InitResult init_result2; + auto db2 = + std::unique_ptr(Firestore::GetInstance(app, &init_result2)); + ASSERT_EQ(kInitResultSuccess, init_result2); + + EXPECT_NE(db1, db2); + db1.reset(); + db2.reset(); } TEST_F(FirestoreTest, CanTerminateNamedFirestoreInstance) { App* app = App::GetInstance(); - InitResult init_result; - Firestore* db = Firestore::GetInstance(app, "foo", &init_result); - ASSERT_EQ(kInitResultSuccess, init_result); + InitResult init_result1; + auto db1 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result1)); + ASSERT_EQ(kInitResultSuccess, init_result1); - EXPECT_THAT(db->Terminate(), FutureSucceeds()); - delete db; + EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + + InitResult init_result2; + auto db2 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result2)); + ASSERT_EQ(kInitResultSuccess, init_result2); + + EXPECT_NE(db1, db2); + db1.reset(); + db2.reset(); } TEST_F(FirestoreTest, MaintainsPersistenceAfterRestarting) { @@ -1394,8 +1412,7 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { } App* app = App::GetInstance(); - Firestore* db1 = Firestore::GetInstance(app, "test-db"); - firestore::LocateEmulator(db1); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "test-db"); // Create a document that we can use for verification later. DocumentReference doc1 = db1->Collection("abc").Document(); @@ -1404,24 +1421,21 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { // Terminate `db1` so that it will be removed from the instance cache. EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + DeleteFirestore(db1); - // Verify that GetInstance() returns a new instance since the old instance has - // been terminated. - Firestore* db2 = Firestore::GetInstance(app, "test-db"); - firestore::LocateEmulator(db2); + // Verify that GetInstance() returns a new instance since the old instance + // has been terminated. + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "test-db"); EXPECT_NE(db1, db2); - // Verify that the new instance points to the same database by verifying that - // the document created with the old instance exists in the new instance. + // Verify that the new instance points to the same database by verifying + // that the document created with the old instance exists in the new instance. DocumentReference doc2 = db2->Document(doc_path); const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); ASSERT_NE(snapshot2, nullptr); EXPECT_TRUE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); - - delete db2; - delete db1; } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { @@ -1432,11 +1446,9 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { // Create two DB instances in the same app. App* app = App::GetInstance(); - Firestore* db1 = Firestore::GetInstance(app, "db1"); - firestore::LocateEmulator(db1); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); - Firestore* db2 = Firestore::GetInstance(app, "db2"); - firestore::LocateEmulator(db2); EXPECT_NE(db1, db2); // Create a document in the first DB instance. @@ -1455,9 +1467,6 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { const DocumentSnapshot* snapshot2 = Await(doc2.Get()); EXPECT_FALSE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); - - delete db2; - delete db1; } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { @@ -1468,11 +1477,10 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { // Create two DB instances in the same app. App* app = App::GetInstance(); - Firestore* db1 = Firestore::GetInstance(app, "db1"); - firestore::LocateEmulator(db1); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "db1"); + + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "db2"); - Firestore* db2 = Firestore::GetInstance(app, "db2"); - firestore::LocateEmulator(db2); EXPECT_NE(db1, db2); DisableNetwork(); @@ -1493,9 +1501,6 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); EXPECT_FALSE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{})); - - delete db2; - delete db1; } TEST_F(FirestoreTest, CanStopListeningAfterTerminate) { diff --git a/firestore/integration_test_internal/src/util/integration_test_util.cc b/firestore/integration_test_internal/src/util/integration_test_util.cc index b8e6b7fe23..fc7cecbc67 100644 --- a/firestore/integration_test_internal/src/util/integration_test_util.cc +++ b/firestore/integration_test_internal/src/util/integration_test_util.cc @@ -15,6 +15,7 @@ */ #include // NOLINT(build/c++11) +#include #include // NOLINT(build/c++11) #include "absl/memory/memory.h" @@ -34,11 +35,11 @@ namespace firebase { namespace firestore { struct TestFriend { - static FirestoreInternal* CreateTestFirestoreInternal(App* app) { + static FirestoreInternal* CreateTestFirestoreInternal( + App* app, const std::string& database_id) { #if !defined(__ANDROID__) return new FirestoreInternal( - app, - /*database_id=*/"(default)", // TODO(Mila): use dynamic database ID + app, database_id.c_str(), absl::make_unique(), absl::make_unique()); #else @@ -80,8 +81,9 @@ App* GetApp(const char* name, const std::string& override_project_id) { App* GetApp() { return GetApp(/*name=*/nullptr, /*project_id=*/""); } -FirestoreInternal* CreateTestFirestoreInternal(App* app) { - return TestFriend::CreateTestFirestoreInternal(app); +FirestoreInternal* CreateTestFirestoreInternal(App* app, + const std::string& database_id) { + return TestFriend::CreateTestFirestoreInternal(app, database_id); } } // namespace firestore From 1ca16a165c29932bf415be4f7604a0b252d6f434 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 3 May 2023 11:31:26 -0400 Subject: [PATCH 19/24] format --- .../src/firestore_integration_test.cc | 19 +++++++------ .../src/firestore_integration_test.h | 27 ++++++++++++------- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index e018028653..e95db25518 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -117,16 +117,18 @@ Firestore* FirestoreIntegrationTest::TestFirestore( } Firestore* FirestoreIntegrationTest::TestFirestoreWithDatabaseId( - const std::string& name, const std::string& database_id) const { - return TestFirestoreWithProjectId(name, /*project_id=*/"", database_id); + const std::string& name, const std::string& database_id) const { + return TestFirestoreWithProjectId(name, /*project_id=*/"", database_id); } Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( - const std::string& name, const std::string& project_id, const std::string& database_id) const { - - for (const auto& entry : firestores_) { + const std::string& name, + const std::string& project_id, + const std::string& database_id) const { + for (const auto& entry : firestores_) { const FirestoreInfo& firestore_info = entry.second; - if (firestore_info.name() == name && firestore_info.database_id() == database_id) { + if (firestore_info.name() == name && + firestore_info.database_id() == database_id) { return firestore_info.firestore(); } } @@ -135,8 +137,9 @@ Firestore* FirestoreIntegrationTest::TestFirestoreWithProjectId( if (apps_.find(app) == apps_.end()) { apps_[app] = std::unique_ptr(app); } - Firestore* db = new Firestore(CreateTestFirestoreInternal(app,database_id)); - firestores_[db] = FirestoreInfo(name, database_id,std::unique_ptr(db)); + Firestore* db = new Firestore(CreateTestFirestoreInternal(app, database_id)); + firestores_[db] = + FirestoreInfo(name, database_id, std::unique_ptr(db)); firestore::LocateEmulator(db); return db; diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 3ed02cd5dc..b2338d7bdd 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -49,7 +49,8 @@ const int kCheckIntervalMillis = 100; // The timeout of waiting for a Future or a listener. const int kTimeOutMillis = 15000; -FirestoreInternal* CreateTestFirestoreInternal(App* app, const std::string& database_id = kDefaultDatabase); +FirestoreInternal* CreateTestFirestoreInternal( + App* app, const std::string& database_id = kDefaultDatabase); App* GetApp(); App* GetApp(const char* name, const std::string& override_project_id); @@ -253,13 +254,15 @@ class FirestoreIntegrationTest : public testing::Test { // Returns a Firestore instance for an app with the given `name`, associated // with the database with the given `project_id`. - Firestore* TestFirestoreWithProjectId(const std::string& name, - const std::string& project_id, - const std::string& database_id = kDefaultDatabase) const; + Firestore* TestFirestoreWithProjectId( + const std::string& name, + const std::string& project_id, + const std::string& database_id = kDefaultDatabase) const; // Returns a Firestore instance for an app with the given `name`, associated // with the database with the given `database_id`. - Firestore* TestFirestoreWithDatabaseId(const std::string& name, const std::string& database_id) const; + Firestore* TestFirestoreWithDatabaseId(const std::string& name, + const std::string& database_id) const; // Deletes the given `Firestore` instance, which must have been returned by a // previous invocation of `TestFirestore()`, and removes it from the cache of @@ -424,19 +427,23 @@ class FirestoreIntegrationTest : public testing::Test { class FirestoreInfo { public: FirestoreInfo() = default; - FirestoreInfo(const std::string& name, const std::string& database_id, std::unique_ptr firestore) - : name_(name), database_id_(database_id) ,firestore_(std::move(firestore)) {} + FirestoreInfo(const std::string& name, + const std::string& database_id, + std::unique_ptr firestore) + : name_(name), + database_id_(database_id), + firestore_(std::move(firestore)) {} const std::string& name() const { return name_; } Firestore* firestore() const { return firestore_.get(); } const std::string& database_id() const { return database_id_; } - void ReleaseFirestore() { firestore_.release(); } + void ReleaseFirestore() { firestore_.release(); } private: std::string name_; - std::string database_id_; - std::unique_ptr firestore_; + std::string database_id_; + std::unique_ptr firestore_; }; // The Firestore and App instance caches. From aad6845f62e31f1945b772ba77befa72b86e76eb Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Wed, 3 May 2023 11:36:00 -0400 Subject: [PATCH 20/24] add as required by lint --- .../integration_test_internal/src/firestore_integration_test.h | 1 + 1 file changed, 1 insertion(+) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index b2338d7bdd..0ddf50523c 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include "app/meta/move.h" From 8ee1871ab5c3ebc5c08b8c6eda0aa3f3a494a95d Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Thu, 4 May 2023 14:01:08 -0400 Subject: [PATCH 21/24] resolve comments --- .../src/firestore_integration_test.h | 2 +- .../src/firestore_test.cc | 36 ++++++++----------- .../src/validation_test.cc | 6 ++-- firestore/src/common/firestore.cc | 7 ++-- firestore/src/include/firebase/firestore.h | 1 - 5 files changed, 23 insertions(+), 29 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 0ddf50523c..71010583fc 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -254,7 +254,7 @@ class FirestoreIntegrationTest : public testing::Test { Firestore* TestFirestore(const std::string& name = kDefaultAppName) const; // Returns a Firestore instance for an app with the given `name`, associated - // with the database with the given `project_id`. + // with the database with the given `project_id` and default `database_id`. Firestore* TestFirestoreWithProjectId( const std::string& name, const std::string& project_id, diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index a206f5a9d3..bbbc1aa6c7 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -16,6 +16,7 @@ #include "firebase/firestore.h" +#include #include #include #include @@ -65,7 +66,7 @@ using ::testing::HasSubstr; class FirestoreTest : public FirestoreIntegrationTest { protected: const std::string GetFirestoreDatabaseId(Firestore* firestore) { - return firestore->internal_->database_id().database_id(); + return GetInternal(firestore)->database_id().database_id(); } }; @@ -100,14 +101,14 @@ TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { App* app = this->app(); EXPECT_NE(nullptr, app); - InitResult result; - Firestore* instance = Firestore::GetInstance(app, "foo", &result); - EXPECT_EQ(kInitResultSuccess, result); + InitResult init_result; + auto instance = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result)); + ASSERT_EQ(kInitResultSuccess, init_result); + EXPECT_NE(nullptr, instance); EXPECT_EQ(app, instance->app()); - EXPECT_EQ(GetFirestoreDatabaseId(instance), "foo"); - - delete instance; + EXPECT_EQ(GetFirestoreDatabaseId(instance.get()), "foo"); } // Sanity test for stubs. @@ -1330,21 +1331,13 @@ TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { TEST_F(FirestoreTest, CanTerminateNamedFirestoreInstance) { App* app = App::GetInstance(); - InitResult init_result1; - auto db1 = std::unique_ptr( - Firestore::GetInstance(app, "foo", &init_result1)); - ASSERT_EQ(kInitResultSuccess, init_result1); + Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "foo"); EXPECT_THAT(db1->Terminate(), FutureSucceeds()); + DeleteFirestore(db1); - InitResult init_result2; - auto db2 = std::unique_ptr( - Firestore::GetInstance(app, "foo", &init_result2)); - ASSERT_EQ(kInitResultSuccess, init_result2); - + Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "foo"); EXPECT_NE(db1, db2); - db1.reset(); - db2.reset(); } TEST_F(FirestoreTest, MaintainsPersistenceAfterRestarting) { @@ -1407,7 +1400,7 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. - if (!firestore::IsUsingFirestoreEmulator()) { + if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1440,7 +1433,7 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. - if (!firestore::IsUsingFirestoreEmulator()) { + if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1471,7 +1464,7 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { // TODO(Mila): Remove the emulator env check after prod supports multiDB. - if (!firestore::IsUsingFirestoreEmulator()) { + if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } @@ -1483,7 +1476,6 @@ TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOffline) { EXPECT_NE(db1, db2); - DisableNetwork(); DisableNetwork(); // Create a document in the first DB instance. diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index 5768375b6e..32ec0000a5 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -497,7 +497,7 @@ TEST_F(ValidationTest, } TEST_F(ValidationTest, - differentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { + DifferentFirestoreGetInstanceCanGetSameDefaultFirestoreInstance) { Firestore* instance1 = Firestore::GetInstance(); Firestore* instance2 = Firestore::GetInstance(app()); Firestore* instance3 = Firestore::GetInstance("(default)"); @@ -513,7 +513,7 @@ TEST_F(ValidationTest, TEST_F( ValidationTest, - differentFirestoreGetInstanceWithSameDatabaseNameShouldGetSameFirestoreInstance) { + DifferentFirestoreGetInstanceWithSameDatabaseNameShouldGetSameFirestoreInstance) { Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance(app(), "foo"); EXPECT_EQ(instance1, instance2); @@ -521,7 +521,7 @@ TEST_F( TEST_F( ValidationTest, - differentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { + DifferentFirestoreGetInstanceWithDifferentDatabaseNameShouldGetDifferentFirestoreInstance) { { Firestore* instance1 = Firestore::GetInstance(); Firestore* instance2 = Firestore::GetInstance("bar"); diff --git a/firestore/src/common/firestore.cc b/firestore/src/common/firestore.cc index 85768cef31..cbf469922a 100644 --- a/firestore/src/common/firestore.cc +++ b/firestore/src/common/firestore.cc @@ -180,11 +180,14 @@ Firestore* Firestore::CreateFirestore(App* app, MutexLock lock(*g_firestores_lock); - Firestore* from_cache = FindFirestoreInCache(app, nullptr, init_result_out); + const char* database_id = internal->database_id().database_id().c_str(); + Firestore* from_cache = + FindFirestoreInCache(app, database_id, init_result_out); SIMPLE_HARD_ASSERT(from_cache == nullptr, "Firestore must not be created already"); - return AddFirestoreToCache(new Firestore(internal), nullptr, init_result_out); + return AddFirestoreToCache(new Firestore(internal), database_id, + init_result_out); } Firestore* Firestore::AddFirestoreToCache(Firestore* firestore, diff --git a/firestore/src/include/firebase/firestore.h b/firestore/src/include/firebase/firestore.h index 3f9558789a..9c745a2127 100644 --- a/firestore/src/include/firebase/firestore.h +++ b/firestore/src/include/firebase/firestore.h @@ -491,7 +491,6 @@ class Firestore { friend class IncludesTest; template friend struct CleanupFn; - friend class FirestoreTest; friend class csharp::ApiHeaders; friend class csharp::TransactionManager; From 3d7b9867ed6403e06525b3ae0b5ee7e1f473abca Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 5 May 2023 12:43:33 -0400 Subject: [PATCH 22/24] resolve comments --- .../src/firestore_test.cc | 25 ++++++++++++------- .../src/validation_test.cc | 25 ++++++++++++++++--- 2 files changed, 38 insertions(+), 12 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index bbbc1aa6c7..082209fd74 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -109,6 +109,7 @@ TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { EXPECT_NE(nullptr, instance); EXPECT_EQ(app, instance->app()); EXPECT_EQ(GetFirestoreDatabaseId(instance.get()), "foo"); + instance.reset(); } // Sanity test for stubs. @@ -1398,14 +1399,17 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewInstance) { delete db1; } -TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { - // TODO(Mila): Remove the emulator env check after prod supports multiDB. +TEST_F(FirestoreTest, + CanReadDocsAfterRestartFirestoreAndCreateNewNamedDatabaseInstance) { + // TODO(Mila): Remove the emulator env check and LocateEmulator call after + // prod supports multiDB. if (!IsUsingFirestoreEmulator()) { GTEST_SKIP(); } App* app = App::GetInstance(); - Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "test-db"); + auto db1 = std::unique_ptr(Firestore::GetInstance(app, "test-db")); + firestore::LocateEmulator(db1.get()); // Create a document that we can use for verification later. DocumentReference doc1 = db1->Collection("abc").Document(); @@ -1414,21 +1418,24 @@ TEST_F(FirestoreTest, RestartFirestoreLeadsToNewNamedDatabaseInstance) { // Terminate `db1` so that it will be removed from the instance cache. EXPECT_THAT(db1->Terminate(), FutureSucceeds()); - DeleteFirestore(db1); - // Verify that GetInstance() returns a new instance since the old instance - // has been terminated. - Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "test-db"); + // Verify that GetInstance() returns a new instance since the old instance has + // been terminated. + auto db2 = std::unique_ptr(Firestore::GetInstance(app, "test-db")); + firestore::LocateEmulator(db2.get()); EXPECT_NE(db1, db2); - // Verify that the new instance points to the same database by verifying - // that the document created with the old instance exists in the new instance. + // Verify that the new instance points to the same database by verifying that + // the document created with the old instance exists in the new instance. DocumentReference doc2 = db2->Document(doc_path); const DocumentSnapshot* snapshot2 = Await(doc2.Get(Source::kCache)); ASSERT_NE(snapshot2, nullptr); EXPECT_TRUE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); + + db1.reset(); + db2.reset(); } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) { diff --git a/firestore/integration_test_internal/src/validation_test.cc b/firestore/integration_test_internal/src/validation_test.cc index 32ec0000a5..3bd0f43ef5 100644 --- a/firestore/integration_test_internal/src/validation_test.cc +++ b/firestore/integration_test_internal/src/validation_test.cc @@ -478,21 +478,29 @@ TEST_F(ValidationTest, Firestore* instance1 = Firestore::GetInstance(); Firestore* instance2 = Firestore::GetInstance(); EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance(app()); Firestore* instance2 = Firestore::GetInstance(app()); EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance("foo"); EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance(app(), "foo"); Firestore* instance2 = Firestore::GetInstance(app(), "foo"); EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; } } @@ -506,9 +514,10 @@ TEST_F(ValidationTest, EXPECT_EQ(instance1, instance2); EXPECT_EQ(instance1, instance3); EXPECT_EQ(instance1, instance4); - EXPECT_EQ(instance2, instance3); - EXPECT_EQ(instance2, instance4); - EXPECT_EQ(instance3, instance4); + delete instance1; + delete instance2; + delete instance3; + delete instance4; } TEST_F( @@ -517,6 +526,8 @@ TEST_F( Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance(app(), "foo"); EXPECT_EQ(instance1, instance2); + delete instance1; + delete instance2; } TEST_F( @@ -526,21 +537,29 @@ TEST_F( Firestore* instance1 = Firestore::GetInstance(); Firestore* instance2 = Firestore::GetInstance("bar"); EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance("bar"); EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance(app(), "foo"); Firestore* instance2 = Firestore::GetInstance(app(), "bar"); EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; } { Firestore* instance1 = Firestore::GetInstance("foo"); Firestore* instance2 = Firestore::GetInstance(app(), "bar"); EXPECT_NE(instance1, instance2); + delete instance1; + delete instance2; } } TEST_F(ValidationTest, CollectionPathsMustBeOddLength) { From d53fbc7628e322d106757808ce4e7396bf559fe8 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 5 May 2023 12:51:26 -0400 Subject: [PATCH 23/24] remove Integration_Test friend from firestore_main.h --- firestore/src/main/firestore_main.h | 1 - 1 file changed, 1 deletion(-) diff --git a/firestore/src/main/firestore_main.h b/firestore/src/main/firestore_main.h index 06593a7d03..4e7f164a58 100644 --- a/firestore/src/main/firestore_main.h +++ b/firestore/src/main/firestore_main.h @@ -135,7 +135,6 @@ class FirestoreInternal { private: friend class TestFriend; - friend class FirestoreTest; enum class AsyncApi { kEnableNetwork = 0, From 6f7d3eac9e415420c135e877a716c7453a5cb859 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 5 May 2023 13:54:43 -0400 Subject: [PATCH 24/24] remove `reset()`s --- .../src/firestore_test.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index 082209fd74..24b478f06e 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -109,7 +109,6 @@ TEST_F(FirestoreTest, GetInstanceWithNamedDatabase) { EXPECT_NE(nullptr, instance); EXPECT_EQ(app, instance->app()); EXPECT_EQ(GetFirestoreDatabaseId(instance.get()), "foo"); - instance.reset(); } // Sanity test for stubs. @@ -1326,18 +1325,22 @@ TEST_F(FirestoreTest, CanTerminateFirestoreInstance) { ASSERT_EQ(kInitResultSuccess, init_result2); EXPECT_NE(db1, db2); - db1.reset(); - db2.reset(); } TEST_F(FirestoreTest, CanTerminateNamedFirestoreInstance) { App* app = App::GetInstance(); - Firestore* db1 = TestFirestoreWithDatabaseId(app->name(), "foo"); + InitResult init_result1; + auto db1 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result1)); + ASSERT_EQ(kInitResultSuccess, init_result1); EXPECT_THAT(db1->Terminate(), FutureSucceeds()); - DeleteFirestore(db1); - Firestore* db2 = TestFirestoreWithDatabaseId(app->name(), "foo"); + InitResult init_result2; + auto db2 = std::unique_ptr( + Firestore::GetInstance(app, "foo", &init_result2)); + ASSERT_EQ(kInitResultSuccess, init_result2); + EXPECT_NE(db1, db2); } @@ -1433,9 +1436,6 @@ TEST_F(FirestoreTest, EXPECT_TRUE(snapshot2->exists()); EXPECT_THAT(snapshot2->GetData(), ContainerEq(MapFieldValue{{"foo", FieldValue::String("bar")}})); - - db1.reset(); - db2.reset(); } TEST_F(FirestoreTest, CanKeepDocsSeparateWithMultiDBWhenOnline) {