From e4f358c986ae780a2daf7c27770dbe4d1d975c77 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 00:46:17 -0400 Subject: [PATCH 01/26] FIRQueryTests.mm: add a test for resuming a query with existence filter --- .../Tests/Integration/API/FIRQueryTests.mm | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index f311a140de0..e4470b92232 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -22,6 +22,16 @@ #import "Firestore/Example/Tests/Util/FSTHelpers.h" #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h" +namespace { + +NSArray *SortedStringsNotIn(NSSet *set, NSSet *remove) { + NSMutableSet *mutableSet = [NSMutableSet setWithSet:set]; + [mutableSet minusSet:remove]; + return [mutableSet.allObjects sortedArrayUsingSelector:@selector(caseInsensitiveCompare:)]; +} + +} // namespace + @interface FIRQueryTests : FSTIntegrationTestCase @end @@ -1180,4 +1190,101 @@ - (void)testOrderByEquality { matchesResult:@[ @"doc6", @"doc3" ]]; } +- (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { + // Prepare the names and contents of the 100 documents to create. + NSMutableDictionary *> *testData = + [[NSMutableDictionary alloc] init]; + for (int i = 0; i < 100; i++) { + [testData setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(i)]]; + } + + // Create 100 documents in a new collection. + FIRCollectionReference *collRef = [self collectionRefWithDocuments:testData]; + + // Run a query to populate the local cache with the 100 documents and a resume token. + NSArray *createdDocuments; + { + FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + NSMutableArray *createdDocumentsAccumulator = + [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in querySnapshot1.documents) { + [createdDocumentsAccumulator addObject:documentSnapshot.reference]; + } + createdDocuments = [createdDocumentsAccumulator copy]; + } + XCTAssertEqual(createdDocuments.count, 100, @"createdDocuments has the wrong size"); + + // Delete 50 of the 100 documents. Do this in a transaction, rather than + // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. + NSSet *deletedDocumentIds; + { + NSMutableArray *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; + XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; + [collRef.firestore + runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) { + for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { + FIRDocumentReference *documentToDelete = createdDocuments[i]; + [transaction deleteDocument:documentToDelete]; + [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID]; + } + return @"document deletion successful"; + } + completion:^(id, NSError *) { + [expectation fulfill]; + }]; + [self awaitExpectation:expectation]; + deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator]; + } + XCTAssertEqual(deletedDocumentIds.count, 50, @"deletedDocumentIds has the wrong size"); + + // Wait for 10 seconds, during which Watch will stop tracking the query and will send an existence + // filter rather than "delete" events when the query is resumed. + [NSThread sleepForTimeInterval:10.0f]; + + // Resume the query and save the resulting snapshot for verification. + FIRQuerySnapshot *querySnapshot2 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + + // Verify that the snapshot from the resumed query contains the expected documents; that is, + // that it contains the 50 documents that were _not_ deleted. + // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to + // send an existence filter. At the time of writing, the Firestore emulator fails to send an + // existence filter, resulting in the client including the deleted documents in the snapshot + // of the resumed query. + if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) { + NSSet *actualDocumentIds; + { + NSMutableArray *actualDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in querySnapshot2.documents) { + [actualDocumentIdsAccumulator addObject:documentSnapshot.documentID]; + } + actualDocumentIds = [NSSet setWithArray:actualDocumentIdsAccumulator]; + } + NSSet *expectedDocumentIds; + { + NSMutableArray *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + for (FIRDocumentReference *documentRef in createdDocuments) { + if (![deletedDocumentIds containsObject:documentRef.documentID]) { + [expectedDocumentIdsAccumulator addObject:documentRef.documentID]; + } + } + expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator]; + } + if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) { + NSArray *unexpectedDocumentIds = + SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); + NSArray *missingDocumentIds = + SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); + XCTFail(@"The snapshot contained %@ documents (expected %@): %@ unexpected and %@ missing; " + @"unexpected documents: %@; missing documents: %@", + @(actualDocumentIds.count), @(expectedDocumentIds.count), + @(unexpectedDocumentIds.count), @(missingDocumentIds.count), + [unexpectedDocumentIds componentsJoinedByString:@", "], + [missingDocumentIds componentsJoinedByString:@", "]); + } + } +} + @end From b8a9c80909cc3d4e1ba2a2dba948e4eff206d710 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 00:52:36 -0400 Subject: [PATCH 02/26] FSTIntegrationTestCase.mm: use a WriteBatch to create documents, for efficiency --- .../Tests/Util/FSTIntegrationTestCase.mm | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm index bbff7d14abe..439eee6a833 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm @@ -25,6 +25,7 @@ #import #import #import +#import #include #include @@ -366,11 +367,48 @@ - (FIRCollectionReference *)collectionRefWithDocuments: - (void)writeAllDocuments:(NSDictionary *> *)documents toCollection:(FIRCollectionReference *)collection { + NSMutableArray *commits = [[NSMutableArray alloc] init]; + __block FIRWriteBatch *writeBatch = nil; + __block int writeBatchSize = 0; + [documents enumerateKeysAndObjectsUsingBlock:^(NSString *key, NSDictionary *value, BOOL *) { - FIRDocumentReference *ref = [collection documentWithPath:key]; - [self writeDocumentRef:ref data:value]; + if (writeBatch == nil) { + writeBatch = [collection.firestore batch]; + } + + [writeBatch setData:value forDocument:[collection documentWithPath:key]]; + writeBatchSize++; + + // Write batches are capped at 500 writes. Use 400 just to be safe. + if (writeBatchSize == 400) { + XCTestExpectation *commitExpectation = [self expectationWithDescription:@"WriteBatch commit"]; + [writeBatch commitWithCompletion:^(NSError *_Nullable error) { + [commitExpectation fulfill]; + if (error != nil) { + XCTFail(@"WriteBatch commit failed: %@", error); + } + }]; + [commits addObject:commitExpectation]; + writeBatch = nil; + writeBatchSize = 0; + } }]; + + if (writeBatch != nil) { + XCTestExpectation *commitExpectation = [self expectationWithDescription:@"WriteBatch commit"]; + [writeBatch commitWithCompletion:^(NSError *_Nullable error) { + [commitExpectation fulfill]; + if (error != nil) { + XCTFail(@"WriteBatch commit failed: %@", error); + } + }]; + [commits addObject:commitExpectation]; + } + + for (XCTestExpectation *commitExpectation in commits) { + [self awaitExpectation:commitExpectation]; + } } - (void)readerAndWriterOnDocumentRef:(void (^)(FIRDocumentReference *readerRef, From aed88292444ab3a7c5023527b669136e19f34dd1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 10:46:11 -0400 Subject: [PATCH 03/26] FIRQueryTests.mm: fix signed/unsigned integer conversion warnings --- .../Example/Tests/Integration/API/FIRQueryTests.mm | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index e4470b92232..786ad0d155a 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1213,7 +1213,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { } createdDocuments = [createdDocumentsAccumulator copy]; } - XCTAssertEqual(createdDocuments.count, 100, @"createdDocuments has the wrong size"); + XCTAssertEqual(createdDocuments.count, 100u, @"createdDocuments has the wrong size"); // Delete 50 of the 100 documents. Do this in a transaction, rather than // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. @@ -1237,7 +1237,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { [self awaitExpectation:expectation]; deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator]; } - XCTAssertEqual(deletedDocumentIds.count, 50, @"deletedDocumentIds has the wrong size"); + XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size"); // Wait for 10 seconds, during which Watch will stop tracking the query and will send an existence // filter rather than "delete" events when the query is resumed. @@ -1277,10 +1277,11 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); NSArray *missingDocumentIds = SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); - XCTFail(@"The snapshot contained %@ documents (expected %@): %@ unexpected and %@ missing; " + XCTFail(@"The snapshot contained %lu documents (expected %lu): " + @"%lu unexpected and %lu missing; " @"unexpected documents: %@; missing documents: %@", - @(actualDocumentIds.count), @(expectedDocumentIds.count), - @(unexpectedDocumentIds.count), @(missingDocumentIds.count), + (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count, + (unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count, [unexpectedDocumentIds componentsJoinedByString:@", "], [missingDocumentIds componentsJoinedByString:@", "]); } From 1d5bed4413b615ade95105bbfd1cfe30b5a77a0d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 12:03:14 -0400 Subject: [PATCH 04/26] FIRQueryTests.mm: fix unused variable warning of NSError** --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 786ad0d155a..fab5db32ba9 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1223,7 +1223,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; [collRef.firestore - runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) { + runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) { for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { FIRDocumentReference *documentToDelete = createdDocuments[i]; [transaction deleteDocument:documentToDelete]; From b4fd14ec38c45e18dee927f2018c0f3413422b9e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 10:54:34 -0400 Subject: [PATCH 05/26] some small tweaks --- .../Tests/Integration/API/FIRQueryTests.mm | 38 ++++++------------- .../Tests/Util/FSTIntegrationTestCase.h | 3 ++ .../Tests/Util/FSTIntegrationTestCase.mm | 10 +++++ 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index fab5db32ba9..93791bae165 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1192,39 +1192,31 @@ - (void)testOrderByEquality { - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { // Prepare the names and contents of the 100 documents to create. - NSMutableDictionary *> *testData = + NSMutableDictionary *> *testDocs = [[NSMutableDictionary alloc] init]; for (int i = 0; i < 100; i++) { - [testData setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(i)]]; + [testDocs setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(1000 + i)]]; } // Create 100 documents in a new collection. - FIRCollectionReference *collRef = [self collectionRefWithDocuments:testData]; + FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs]; // Run a query to populate the local cache with the 100 documents and a resume token. - NSArray *createdDocuments; - { - FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef - source:FIRFirestoreSourceDefault]; - NSMutableArray *createdDocumentsAccumulator = - [[NSMutableArray alloc] init]; - for (FIRDocumentSnapshot *documentSnapshot in querySnapshot1.documents) { - [createdDocumentsAccumulator addObject:documentSnapshot.reference]; - } - createdDocuments = [createdDocumentsAccumulator copy]; - } - XCTAssertEqual(createdDocuments.count, 100u, @"createdDocuments has the wrong size"); + FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value"); + NSArray *createdDocuments = + FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1); // Delete 50 of the 100 documents. Do this in a transaction, rather than // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. NSSet *deletedDocumentIds; { NSMutableArray *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; - deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; [collRef.firestore runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) { - for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { + for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) { FIRDocumentReference *documentToDelete = createdDocuments[i]; [transaction deleteDocument:documentToDelete]; [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID]; @@ -1254,14 +1246,8 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { // existence filter, resulting in the client including the deleted documents in the snapshot // of the resumed query. if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) { - NSSet *actualDocumentIds; - { - NSMutableArray *actualDocumentIdsAccumulator = [[NSMutableArray alloc] init]; - for (FIRDocumentSnapshot *documentSnapshot in querySnapshot2.documents) { - [actualDocumentIdsAccumulator addObject:documentSnapshot.documentID]; - } - actualDocumentIds = [NSSet setWithArray:actualDocumentIdsAccumulator]; - } + NSSet *actualDocumentIds = + [NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)]; NSSet *expectedDocumentIds; { NSMutableArray *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; @@ -1277,7 +1263,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); NSArray *missingDocumentIds = SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); - XCTFail(@"The snapshot contained %lu documents (expected %lu): " + XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): " @"%lu unexpected and %lu missing; " @"unexpected documents: %@; missing documents: %@", (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count, diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h index 6b4ee5fb356..c33f3f5bc97 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h @@ -142,6 +142,9 @@ NSArray *FIRQuerySnapshotGetIDs(FIRQuerySnapshot *docs); * in order of { type, doc title, doc data }. */ NSArray *> *FIRQuerySnapshotGetDocChangesData(FIRQuerySnapshot *docs); +/** Gets the FIRDocumentReference objects from a FIRQuerySnapshot and returns them. */ +NSArray *FIRDocumentReferenceArrayFromQuerySnapshot(FIRQuerySnapshot *); + #if __cplusplus } // extern "C" #endif diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm index d89e6881ce6..d2db81f869b 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm @@ -600,6 +600,16 @@ - (void)waitUntil:(BOOL (^)())predicate { return result; } +extern "C" NSArray *FIRDocumentReferenceArrayFromQuerySnapshot( + FIRQuerySnapshot *docs) { + NSMutableArray *documentReferenceAccumulator = + [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in docs.documents) { + [documentReferenceAccumulator addObject:documentSnapshot.reference]; + } + return [documentReferenceAccumulator copy]; +} + @end NS_ASSUME_NONNULL_END From 91960418696f69e7b0fa2b434b2e60c6c9413fe5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 13:57:16 -0400 Subject: [PATCH 06/26] testing_hooks.h skeleton added --- Firestore/core/src/util/testing_hooks.cc | 26 +++++++ Firestore/core/src/util/testing_hooks.h | 92 ++++++++++++++++++++++++ 2 files changed, 118 insertions(+) create mode 100644 Firestore/core/src/util/testing_hooks.cc create mode 100644 Firestore/core/src/util/testing_hooks.h diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc new file mode 100644 index 00000000000..1ea276ec0a2 --- /dev/null +++ b/Firestore/core/src/util/testing_hooks.cc @@ -0,0 +1,26 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/util/testing_hooks.h" + +namespace firebase { +namespace firestore { +namespace util { + + +} // namespace util +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h new file mode 100644 index 00000000000..eb06f2b4ba3 --- /dev/null +++ b/Firestore/core/src/util/testing_hooks.h @@ -0,0 +1,92 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIRESTORE_CORE_SRC_UTIL_TESTING_HOOKS_H_ +#define FIRESTORE_CORE_SRC_UTIL_TESTING_HOOKS_H_ + +#include +#include +#include + +#include "Firestore/core/src/api/listener_registration.h" + +namespace firebase { +namespace firestore { +namespace util { + +/** + * Manages "testing hooks", hooks into the internals of the SDK to verify + * internal state and events during integration tests. Do not use this class + * except for testing purposes. + */ +class TestingHooks final { + public: + + /** + * Information about an existence filter mismatch, as specified to callbacks + * registered with `OnExistenceFilterMismatch()`. + */ + struct ExistenceFilterMismatchInfo { + int32_t localCacheCount = -1; + int32_t existenceFilterCount = -1; + }; + + /** + * Registers a callback to be invoked when an existence filter mismatch occurs + * in the Watch listen stream. + * + * The relative order in which callbacks are notified is unspecified; do not + * rely on any particular ordering. If a given callback is registered multiple + * times then it will be notified multiple times, once per registration. + * + * The thread on which the callback occurs is unspecified; listeners should + * perform their work as quickly as possible and return to avoid blocking any + * critical work. In particular, the listener callbacks should *not* block or + * perform long-running operations. Listener callbacks can occur concurrently + * with other callbacks on the same and other listeners. + * + * The `ExistenceFilterMismatchInfo` reference specified to the callback is + * only valid during the lifetime of the callback. Once the callback returns + * then it must not use the given `ExistenceFilterMismatchInfo` reference + * again. + * + * @param callback the callback to invoke upon existence filter mismatch. + * + * @return an object whose `Remove()` member function unregisters the given + * callback; only the first invocation of `Remove()` does anything; all + * subsequent invocations do nothing. + */ + static std::shared_ptr OnExistenceFilterMismatch(std::function); + + /** + * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks. + * @param info Information about the existence filter mismatch. + */ + static void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&); + + private: + TestingHooks() = delete; + TestingHooks(const TestingHooks&) = delete; + TestingHooks(TestingHooks&&) = delete; + TestingHooks& operator=(const TestingHooks&) = delete; + TestingHooks& operator=(TestingHooks&&) = delete; +}; + +} // namespace util +} // namespace firestore +} // namespace firebase + +#endif // FIRESTORE_CORE_SRC_UTIL_TESTING_HOOKS_H_ From 3e5df3792d3890c6b950de0260bbf7d50a7f59b9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 16:06:01 -0400 Subject: [PATCH 07/26] add a stub implementation of testing hooks --- Firestore/core/src/remote/remote_event.cc | 4 +++ Firestore/core/src/util/testing_hooks.cc | 31 +++++++++++++++++++++++ Firestore/core/src/util/testing_hooks.h | 12 ++++----- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/Firestore/core/src/remote/remote_event.cc b/Firestore/core/src/remote/remote_event.cc index f6f5d86ccca..022f386b748 100644 --- a/Firestore/core/src/remote/remote_event.cc +++ b/Firestore/core/src/remote/remote_event.cc @@ -19,6 +19,7 @@ #include #include "Firestore/core/src/local/target_data.h" +#include "Firestore/core/src/util/testing_hooks.h" namespace firebase { namespace firestore { @@ -34,6 +35,7 @@ using model::MutableDocument; using model::SnapshotVersion; using model::TargetId; using nanopb::ByteString; +using util::TestingHooks; // TargetChange @@ -239,6 +241,8 @@ void WatchChangeAggregator::HandleExistenceFilter( // snapshot with `isFromCache:true`. ResetTarget(target_id); pending_target_resets_.insert(target_id); + TestingHooks::NotifyOnExistenceFilterMismatch( + {current_size, expected_count}); } } } diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index 1ea276ec0a2..c3dd142e38b 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -14,12 +14,43 @@ * limitations under the License. */ +#include +#include + #include "Firestore/core/src/util/testing_hooks.h" +#include "absl/base/attributes.h" + namespace firebase { namespace firestore { namespace util { +namespace { + +using ExistenceFilterMismatchCallback = + std::function; + +class OnExistenceFilterMismatchListenerRegistration final + : public api::ListenerRegistration { + public: + void Remove() override { + } +}; + +} // namespace + +ABSL_ATTRIBUTE_UNUSED // This function is only used in integration tests + std::shared_ptr + TestingHooks::OnExistenceFilterMismatch( + ExistenceFilterMismatchCallback callback) { + (void)callback; + return std::make_shared(); +} + +void TestingHooks::NotifyOnExistenceFilterMismatch( + const ExistenceFilterMismatchInfo& info) { + (void)info; +} } // namespace util } // namespace firestore diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index eb06f2b4ba3..bd9621c1412 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -17,7 +17,6 @@ #ifndef FIRESTORE_CORE_SRC_UTIL_TESTING_HOOKS_H_ #define FIRESTORE_CORE_SRC_UTIL_TESTING_HOOKS_H_ -#include #include #include @@ -34,14 +33,13 @@ namespace util { */ class TestingHooks final { public: - /** * Information about an existence filter mismatch, as specified to callbacks * registered with `OnExistenceFilterMismatch()`. */ struct ExistenceFilterMismatchInfo { - int32_t localCacheCount = -1; - int32_t existenceFilterCount = -1; + int localCacheCount = -1; + int existenceFilterCount = -1; }; /** @@ -69,13 +67,15 @@ class TestingHooks final { * callback; only the first invocation of `Remove()` does anything; all * subsequent invocations do nothing. */ - static std::shared_ptr OnExistenceFilterMismatch(std::function); + static std::shared_ptr OnExistenceFilterMismatch( + std::function); /** * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks. * @param info Information about the existence filter mismatch. */ - static void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&); + static void NotifyOnExistenceFilterMismatch( + const ExistenceFilterMismatchInfo&); private: TestingHooks() = delete; From 1aff483d5c904d638835373e94c7a6fbb18e4384 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 16:36:43 -0400 Subject: [PATCH 08/26] wip --- .../core/test/unit/util/testing_hooks_test.cc | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 Firestore/core/test/unit/util/testing_hooks_test.cc diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc new file mode 100644 index 00000000000..a1a0c9c1b50 --- /dev/null +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -0,0 +1,87 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/src/util/testing_hooks.h" + +#include "absl/types/optional.h" + +#include +#include +#include +#include +#include +#include + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace { + +using namespace std::chrono_literals; +using firebase::firestore::util::TestingHooks; + +template +class MessageAccumulator : public std::enable_shared_from_this> { + public: + static std::shared_ptr NewInstance() { + return std::shared_ptr(new MessageAccumulator); + } + + std::function MakeListener() { + auto shared_this = this->shared_from_this(); + return [shared_this](const T& message) { + shared_this->OnMessage(message); + }; + } + + void OnMessage(T&& message) { + std::lock_guard lock(mutex_); + messages_.push_back(std::move(message)); + condition_variable_.notify_all(); + } + + absl::optional WaitForMessage() { + std::unique_lock lock(mutex_); + condition_variable_.wait_for(lock, 1000ms, [&](){return !messages_.empty();}); + auto iter = messages_.begin(); + if (iter == messages_.end()) { + return absl::nullopt; + } + T message = std::move(*iter); + messages_.erase(iter); + return std::move(message); + } + + private: + MessageAccumulator() = default; + std::mutex mutex_; + std::condition_variable condition_variable_; + std::vector messages_; +}; + +TEST(TestingHooks, OnExistenceFilterMismatchShouldCompleteSuccessfully) { + TestingHooks::OnExistenceFilterMismatch([](const TestingHooks::ExistenceFilterMismatchInfo&) {}); +} + +TEST(TestingHooks, OnExistenceFilterMismatchRegisteredCallbackShouldGetNotified) { + auto accumulator = MessageAccumulator::NewInstance(); + TestingHooks::OnExistenceFilterMismatch(accumulator->MakeListener()); + absl::optional message_optional = accumulator->WaitForMessage(); + ASSERT_TRUE(message_optional.has_value()); + TestingHooks::ExistenceFilterMismatchInfo message = std::move(message_optional).value(); +} + +} // namespace From 48583473e050cd9ca6c7af3602ffa7443f4e5cbb Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 12:18:27 -0400 Subject: [PATCH 09/26] implement a working TestingHooks --- Firestore/core/src/remote/remote_event.cc | 2 +- Firestore/core/src/util/testing_hooks.cc | 64 +++++++--- Firestore/core/src/util/testing_hooks.h | 27 ++++- .../core/test/unit/testutil/async_testing.h | 111 ++++++++++++++++++ .../core/test/unit/util/testing_hooks_test.cc | 81 +++++-------- 5 files changed, 212 insertions(+), 73 deletions(-) diff --git a/Firestore/core/src/remote/remote_event.cc b/Firestore/core/src/remote/remote_event.cc index 022f386b748..0c08d983dbf 100644 --- a/Firestore/core/src/remote/remote_event.cc +++ b/Firestore/core/src/remote/remote_event.cc @@ -241,7 +241,7 @@ void WatchChangeAggregator::HandleExistenceFilter( // snapshot with `isFromCache:true`. ResetTarget(target_id); pending_target_resets_.insert(target_id); - TestingHooks::NotifyOnExistenceFilterMismatch( + TestingHooks::GetInstance().NotifyOnExistenceFilterMismatch( {current_size, expected_count}); } } diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index c3dd142e38b..03deb3f95d7 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -14,42 +14,76 @@ * limitations under the License. */ +#include #include #include +#include +#include "Firestore/core/src/util/no_destructor.h" #include "Firestore/core/src/util/testing_hooks.h" -#include "absl/base/attributes.h" - namespace firebase { namespace firestore { namespace util { namespace { -using ExistenceFilterMismatchCallback = - std::function; - -class OnExistenceFilterMismatchListenerRegistration final - : public api::ListenerRegistration { +class RemoveDelegateListenerRegistration final : public api::ListenerRegistration { public: + RemoveDelegateListenerRegistration(std::function delegate) : delegate_(std::move(delegate)) { + } + void Remove() override { + delegate_(); } + + private: + std::function delegate_; }; -} // namespace -ABSL_ATTRIBUTE_UNUSED // This function is only used in integration tests - std::shared_ptr - TestingHooks::OnExistenceFilterMismatch( - ExistenceFilterMismatchCallback callback) { - (void)callback; - return std::make_shared(); +} + +std::shared_ptr TestingHooks::OnExistenceFilterMismatch( ExistenceFilterMismatchCallback callback) { + std::lock_guard lock(mutex_); + const int id = next_id_++; + existence_filter_mismatch_callbacks_.insert({id, std::move(callback)}); + + return std::make_shared([this, id]() { + std::lock_guard lock(mutex_); + auto iter = existence_filter_mismatch_callbacks_.find(id); + if (iter != existence_filter_mismatch_callbacks_.end()) { + existence_filter_mismatch_callbacks_.erase(iter); + } + }); } void TestingHooks::NotifyOnExistenceFilterMismatch( const ExistenceFilterMismatchInfo& info) { - (void)info; + std::unique_lock lock(mutex_); + + // Short-circuit if there is nothing to do. + if (existence_filter_mismatch_callbacks_.empty()) { + return; + } + + // Copy the callbacks into a vector so that we don't need to hold the mutex + // while making the callbacks. This "copy" is somewhat inefficient; however, + // it will only ever happen during integration testing so performance is not + // a concern. + std::vector callbacks; + for (auto&& entry : existence_filter_mismatch_callbacks_) { + callbacks.push_back(entry.second); + } + + // Release the lock so that it is released while calling the callbacks, to + // avoid any potential deadlock. + lock.unlock(); + + // Notify the callbacks. + for (auto&& callback : callbacks) { + callback(info); + } } } // namespace util diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index bd9621c1412..f073cf45377 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -19,6 +19,8 @@ #include #include +#include +#include #include "Firestore/core/src/api/listener_registration.h" @@ -33,6 +35,13 @@ namespace util { */ class TestingHooks final { public: + + /** Returns the singleton instance of this class. */ + static TestingHooks& GetInstance() { + TestingHooks* instance = new TestingHooks; + return *instance; + } + /** * Information about an existence filter mismatch, as specified to callbacks * registered with `OnExistenceFilterMismatch()`. @@ -42,6 +51,8 @@ class TestingHooks final { int existenceFilterCount = -1; }; + using ExistenceFilterMismatchCallback = std::function; + /** * Registers a callback to be invoked when an existence filter mismatch occurs * in the Watch listen stream. @@ -67,22 +78,28 @@ class TestingHooks final { * callback; only the first invocation of `Remove()` does anything; all * subsequent invocations do nothing. */ - static std::shared_ptr OnExistenceFilterMismatch( - std::function); + std::shared_ptr OnExistenceFilterMismatch(ExistenceFilterMismatchCallback); /** * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks. * @param info Information about the existence filter mismatch. */ - static void NotifyOnExistenceFilterMismatch( - const ExistenceFilterMismatchInfo&); + void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&); private: - TestingHooks() = delete; + TestingHooks() = default; + TestingHooks(const TestingHooks&) = delete; TestingHooks(TestingHooks&&) = delete; TestingHooks& operator=(const TestingHooks&) = delete; TestingHooks& operator=(TestingHooks&&) = delete; + + friend class TestingHooksTestHelper; + + mutable std::mutex mutex_; + int next_id_ = 0; + std::unordered_map existence_filter_mismatch_callbacks_; + }; } // namespace util diff --git a/Firestore/core/test/unit/testutil/async_testing.h b/Firestore/core/test/unit/testutil/async_testing.h index efb44f13086..1a501e6c7b7 100644 --- a/Firestore/core/test/unit/testutil/async_testing.h +++ b/Firestore/core/test/unit/testutil/async_testing.h @@ -18,10 +18,13 @@ #define FIRESTORE_CORE_TEST_UNIT_TESTUTIL_ASYNC_TESTING_H_ #include // NOLINT(build/c++11) +#include #include #include // NOLINT(build/c++11) #include +#include #include // NOLINT(build/c++11) +#include #include "gtest/gtest.h" @@ -136,6 +139,114 @@ class AsyncTest { << testing::UnitTest::GetInstance()->current_test_info()->name()}; }; +/** + * A class that can be used to "accumulate" objects that is completely thread + * safe. + * + * When testing "listeners" it is common in tests to just create a std::vector, + * register a "listener", then add objects into the vector when the listener is + * notified. This, however, is not thread safe because there is typically no + * synchronization in place, such as via a mutex. Moreover, if the listener + * receives a notification after the test method completes then the vector, + * which was allocated on the stack, is deleted. Both of these problems result + * in undefined behavior, which is bad. + * + * Using `AsyncAccumulator` solves both of these problems. First, it protects + * the std::vector instance with a mutex to eliminate race conditions. Second, + * instances can only be created as shared_ptr, which can be copied into the + * listener and will keep the vector alive until the test completes or the + * listener is deleted, whichever comes last. + * + * The constructor of `AsyncAccumulator` is private, in order to force + * instances to be created with a shared_ptr via the `NewInstance()` method. + */ +template +class AsyncAccumulator final : public std::enable_shared_from_this> { + public: + + /** + * Creates and returns a std::shared_ptr to a new instance of this class. + */ + static std::shared_ptr NewInstance() { + return std::shared_ptr(new AsyncAccumulator); + } + + /** + * Adds a copy of the given object to this object's encapsulated vector and + * resolves any outstanding std::future objects returned from + * `WaitForObject()`. + */ + void AccumulateObject(const T& object) { + std::lock_guard lock(mutex_); + objects_.push_back(object); + for (auto&& promise : promises_) { + promise.set_value(); + } + promises_.clear(); + } + + /** + * Creates and returns a std::future that resolves when an object is + * accumulated via a call to `AccumulateObject()`. If there is an object + * already accumulated in this object's encapsulated vector then the returned + * future will be resolved immediately. + */ + std::future WaitForObject() { + std::lock_guard lock(mutex_); + std::promise promise; + std::future future = promise.get_future(); + + if (objects_.empty()) { + promises_.push_back(std::move(promise)); + } else { + promise.set_value(); + } + + return future; + } + + /** + * Returns whether the encapsulated vector of accumulated objects is empty. + */ + bool IsEmpty() const { + std::lock_guard lock(mutex_); + return objects_.empty(); + } + + /** + * Removes the first element from the encapsulated vector and returns it. + * + * This function exhibits undefined behavior if the encapsulated vector is + * empty. + */ + T Shift() { + std::lock_guard lock(mutex_); + auto iter = objects_.begin(); + T result = std::move(*iter); + objects_.erase(iter); + return result; + } + + /** + * Creates and returns a function that, when invoked, calls + * `AccumulateObject()`. + */ + std::function AsCallback() { + auto shared_this = this->shared_from_this(); + return [shared_this](const T& object) { + shared_this->AccumulateObject(object); + }; + } + + private: + // Private constructor to force instances to be created via NewInstance(). + AsyncAccumulator() = default; + + mutable std::mutex mutex_; + std::vector objects_; + std::vector> promises_; +}; + } // namespace testutil } // namespace firestore } // namespace firebase diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index a1a0c9c1b50..3f21ac389e1 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -16,72 +16,49 @@ #include "Firestore/core/src/util/testing_hooks.h" -#include "absl/types/optional.h" +#include "Firestore/core/test/unit/testutil/async_testing.h" -#include -#include -#include -#include -#include -#include +#include "absl/types/optional.h" #include "gmock/gmock.h" #include "gtest/gtest.h" -namespace { - -using namespace std::chrono_literals; -using firebase::firestore::util::TestingHooks; +namespace firebase { +namespace firestore { +namespace util { -template -class MessageAccumulator : public std::enable_shared_from_this> { +// A "friend" class of TestingHooks to call its private members. +class TestingHooksTestHelper { public: - static std::shared_ptr NewInstance() { - return std::shared_ptr(new MessageAccumulator); - } + TestingHooksTestHelper() : testing_hooks_(new TestingHooks) {} + std::shared_ptr testing_hooks_; +}; - std::function MakeListener() { - auto shared_this = this->shared_from_this(); - return [shared_this](const T& message) { - shared_this->OnMessage(message); - }; - } +} // namespace util +} // namespace firestore +} // namespace firebase - void OnMessage(T&& message) { - std::lock_guard lock(mutex_); - messages_.push_back(std::move(message)); - condition_variable_.notify_all(); - } +namespace { - absl::optional WaitForMessage() { - std::unique_lock lock(mutex_); - condition_variable_.wait_for(lock, 1000ms, [&](){return !messages_.empty();}); - auto iter = messages_.begin(); - if (iter == messages_.end()) { - return absl::nullopt; - } - T message = std::move(*iter); - messages_.erase(iter); - return std::move(message); - } +using firebase::firestore::util::TestingHooks; +using firebase::firestore::util::TestingHooksTestHelper; +using firebase::firestore::testutil::AsyncTest; +using ExistenceFilterMismatchInfoAccumulator = firebase::firestore::testutil::AsyncAccumulator; - private: - MessageAccumulator() = default; - std::mutex mutex_; - std::condition_variable condition_variable_; - std::vector messages_; +class TestingHooksTest : public ::testing::Test, public AsyncTest, public TestingHooksTestHelper { }; -TEST(TestingHooks, OnExistenceFilterMismatchShouldCompleteSuccessfully) { - TestingHooks::OnExistenceFilterMismatch([](const TestingHooks::ExistenceFilterMismatchInfo&) {}); -} +TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { + auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + + Async([testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch({123, 456}); }); -TEST(TestingHooks, OnExistenceFilterMismatchRegisteredCallbackShouldGetNotified) { - auto accumulator = MessageAccumulator::NewInstance(); - TestingHooks::OnExistenceFilterMismatch(accumulator->MakeListener()); - absl::optional message_optional = accumulator->WaitForMessage(); - ASSERT_TRUE(message_optional.has_value()); - TestingHooks::ExistenceFilterMismatchInfo message = std::move(message_optional).value(); + Await(accumulator->WaitForObject()); + ASSERT_FALSE(accumulator->IsEmpty()); + TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); + EXPECT_EQ(info.localCacheCount, 123); + EXPECT_EQ(info.existenceFilterCount, 456); } } // namespace From 1a10005e804b1be29f9a3e9da5c44c94a1c975f8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 12:49:10 -0400 Subject: [PATCH 10/26] finish testing hooks --- Firestore/core/src/util/testing_hooks.cc | 53 ++++++------- Firestore/core/src/util/testing_hooks.h | 2 +- .../core/test/unit/util/testing_hooks_test.cc | 78 +++++++++++++++++-- 3 files changed, 95 insertions(+), 38 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index 03deb3f95d7..ade2c56f670 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -14,13 +14,13 @@ * limitations under the License. */ -#include +#include "Firestore/core/src/util/testing_hooks.h" + +#include #include -#include #include #include "Firestore/core/src/util/no_destructor.h" -#include "Firestore/core/src/util/testing_hooks.h" namespace firebase { namespace firestore { @@ -28,9 +28,14 @@ namespace util { namespace { -class RemoveDelegateListenerRegistration final : public api::ListenerRegistration { +/** + * An implementation of `ListenerRegistration` whose `Remove()` method simply + * invokes the function specified to the constructor. This allows easily + * creating `ListenerRegistration` objects that call a lambda. + */ +class RemoveDelegatingListenerRegistration final : public api::ListenerRegistration { public: - RemoveDelegateListenerRegistration(std::function delegate) : delegate_(std::move(delegate)) { + RemoveDelegatingListenerRegistration(std::function delegate) : delegate_(std::move(delegate)) { } void Remove() override { @@ -44,12 +49,16 @@ class RemoveDelegateListenerRegistration final : public api::ListenerRegistratio } -std::shared_ptr TestingHooks::OnExistenceFilterMismatch( ExistenceFilterMismatchCallback callback) { - std::lock_guard lock(mutex_); +std::shared_ptr TestingHooks::OnExistenceFilterMismatch(ExistenceFilterMismatchCallback callback) { + // Register the callback. + std::unique_lock lock(mutex_); const int id = next_id_++; - existence_filter_mismatch_callbacks_.insert({id, std::move(callback)}); + existence_filter_mismatch_callbacks_.insert({id, std::make_shared(std::move(callback))}); + lock.unlock(); - return std::make_shared([this, id]() { + // Create a ListenerRegistration that the caller can use to unregister the + // callback. + return std::make_shared([this, id]() { std::lock_guard lock(mutex_); auto iter = existence_filter_mismatch_callbacks_.find(id); if (iter != existence_filter_mismatch_callbacks_.end()) { @@ -60,29 +69,11 @@ std::shared_ptr TestingHooks::OnExistenceFilterMismat void TestingHooks::NotifyOnExistenceFilterMismatch( const ExistenceFilterMismatchInfo& info) { - std::unique_lock lock(mutex_); - - // Short-circuit if there is nothing to do. - if (existence_filter_mismatch_callbacks_.empty()) { - return; - } - - // Copy the callbacks into a vector so that we don't need to hold the mutex - // while making the callbacks. This "copy" is somewhat inefficient; however, - // it will only ever happen during integration testing so performance is not - // a concern. - std::vector callbacks; + std::lock_guard lock(mutex_); for (auto&& entry : existence_filter_mismatch_callbacks_) { - callbacks.push_back(entry.second); - } - - // Release the lock so that it is released while calling the callbacks, to - // avoid any potential deadlock. - lock.unlock(); - - // Notify the callbacks. - for (auto&& callback : callbacks) { - callback(info); + std::async([info, callback = entry.second]() { + callback->operator()(info); + }); } } diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index f073cf45377..033eaa83109 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -98,7 +98,7 @@ class TestingHooks final { mutable std::mutex mutex_; int next_id_ = 0; - std::unordered_map existence_filter_mismatch_callbacks_; + std::unordered_map> existence_filter_mismatch_callbacks_; }; diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index 3f21ac389e1..d80f284d8aa 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -16,6 +16,9 @@ #include "Firestore/core/src/util/testing_hooks.h" +#include + +#include "Firestore/core/src/api/listener_registration.h" #include "Firestore/core/test/unit/testutil/async_testing.h" #include "absl/types/optional.h" @@ -40,25 +43,88 @@ class TestingHooksTestHelper { namespace { +using namespace std::chrono_literals; +using firebase::firestore::api::ListenerRegistration; using firebase::firestore::util::TestingHooks; using firebase::firestore::util::TestingHooksTestHelper; using firebase::firestore::testutil::AsyncTest; using ExistenceFilterMismatchInfoAccumulator = firebase::firestore::testutil::AsyncAccumulator; class TestingHooksTest : public ::testing::Test, public AsyncTest, public TestingHooksTestHelper { + public: + void AssertInvokedWith(const std::shared_ptr& accumulator, TestingHooks::ExistenceFilterMismatchInfo expected) { + Await(accumulator->WaitForObject()); + ASSERT_FALSE(accumulator->IsEmpty()); + TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); + EXPECT_EQ(info.localCacheCount, expected.localCacheCount); + EXPECT_EQ(info.existenceFilterCount, expected.existenceFilterCount); + } + + void NotifyOnExistenceFilterMismatchAsync(TestingHooks::ExistenceFilterMismatchInfo info) { + Async([info, testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch(info); }); + } }; TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); - Async([testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch({123, 456}); }); + NotifyOnExistenceFilterMismatchAsync({123, 456}); + + AssertInvokedWith(accumulator, {123, 456}); +} + +TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) { + auto accumulator1 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); + testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); + + NotifyOnExistenceFilterMismatchAsync({123, 456}); + + AssertInvokedWith(accumulator1, {123, 456}); + AssertInvokedWith(accumulator2, {123, 456}); +} + +TEST_F(TestingHooksTest, OnExistenceFilterMismatchShouldNotBeNotifiedAfterRemove) { + auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + std::shared_ptr registration = testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + registration->Remove(); + + NotifyOnExistenceFilterMismatchAsync({123, 456}); + + std::this_thread::sleep_for(250ms); + EXPECT_TRUE(accumulator->IsEmpty()); +} + +TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { + auto accumulator1 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + auto accumulator3 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); + std::shared_ptr registration2 = testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); + testing_hooks_->OnExistenceFilterMismatch(accumulator3->AsCallback()); + registration2->Remove(); + + NotifyOnExistenceFilterMismatchAsync({123, 456}); + + AssertInvokedWith(accumulator1, {123, 456}); + AssertInvokedWith(accumulator3, {123, 456}); + std::this_thread::sleep_for(250ms); + EXPECT_TRUE(accumulator2->IsEmpty()); +} + +TEST_F(TestingHooksTest, OnExistenceFilterMismatchMultipleRemovesHaveNoEffect) { + auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + std::shared_ptr registration = testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + registration->Remove(); + registration->Remove(); + registration->Remove(); + + NotifyOnExistenceFilterMismatchAsync({123, 456}); - Await(accumulator->WaitForObject()); - ASSERT_FALSE(accumulator->IsEmpty()); - TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); - EXPECT_EQ(info.localCacheCount, 123); - EXPECT_EQ(info.existenceFilterCount, 456); + std::this_thread::sleep_for(250ms); + EXPECT_TRUE(accumulator->IsEmpty()); } } // namespace From 50ed8a30dcf927444c2f787b4c64ff091c7d1c11 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 16:03:02 -0400 Subject: [PATCH 11/26] clean up tests --- .../core/test/unit/util/testing_hooks_test.cc | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index d80f284d8aa..02052394ce3 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -17,13 +17,13 @@ #include "Firestore/core/src/util/testing_hooks.h" #include +#include +#include +#include #include "Firestore/core/src/api/listener_registration.h" #include "Firestore/core/test/unit/testutil/async_testing.h" -#include "absl/types/optional.h" - -#include "gmock/gmock.h" #include "gtest/gtest.h" namespace firebase { @@ -52,7 +52,7 @@ using ExistenceFilterMismatchInfoAccumulator = firebase::firestore::testutil::As class TestingHooksTest : public ::testing::Test, public AsyncTest, public TestingHooksTestHelper { public: - void AssertInvokedWith(const std::shared_ptr& accumulator, TestingHooks::ExistenceFilterMismatchInfo expected) { + void AssertAccumulatedObject(const std::shared_ptr& accumulator, TestingHooks::ExistenceFilterMismatchInfo expected) { Await(accumulator->WaitForObject()); ASSERT_FALSE(accumulator->IsEmpty()); TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); @@ -60,8 +60,8 @@ class TestingHooksTest : public ::testing::Test, public AsyncTest, public Testin EXPECT_EQ(info.existenceFilterCount, expected.existenceFilterCount); } - void NotifyOnExistenceFilterMismatchAsync(TestingHooks::ExistenceFilterMismatchInfo info) { - Async([info, testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch(info); }); + std::future NotifyOnExistenceFilterMismatchAsync(TestingHooks::ExistenceFilterMismatchInfo info) { + return Async([info, testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch(info); }); } }; @@ -71,7 +71,19 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { NotifyOnExistenceFilterMismatchAsync({123, 456}); - AssertInvokedWith(accumulator, {123, 456}); + AssertAccumulatedObject(accumulator, {123, 456}); +} + +TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotifiedMultipleTimes) { + auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + + NotifyOnExistenceFilterMismatchAsync({111, 222}); + AssertAccumulatedObject(accumulator, {111, 222}); + NotifyOnExistenceFilterMismatchAsync({333, 444}); + AssertAccumulatedObject(accumulator, {333, 444}); + NotifyOnExistenceFilterMismatchAsync({555, 666}); + AssertAccumulatedObject(accumulator, {555, 666}); } TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) { @@ -82,8 +94,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) NotifyOnExistenceFilterMismatchAsync({123, 456}); - AssertInvokedWith(accumulator1, {123, 456}); - AssertInvokedWith(accumulator2, {123, 456}); + AssertAccumulatedObject(accumulator1, {123, 456}); + AssertAccumulatedObject(accumulator2, {123, 456}); } TEST_F(TestingHooksTest, OnExistenceFilterMismatchShouldNotBeNotifiedAfterRemove) { @@ -108,8 +120,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { NotifyOnExistenceFilterMismatchAsync({123, 456}); - AssertInvokedWith(accumulator1, {123, 456}); - AssertInvokedWith(accumulator3, {123, 456}); + AssertAccumulatedObject(accumulator1, {123, 456}); + AssertAccumulatedObject(accumulator3, {123, 456}); std::this_thread::sleep_for(250ms); EXPECT_TRUE(accumulator2->IsEmpty()); } From 886dfbafa5b19ba0c48ed496528a5e635ac6e4b1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 16:04:42 -0400 Subject: [PATCH 12/26] ./scripts/style.sh --- Firestore/core/src/util/testing_hooks.cc | 22 +++++---- Firestore/core/src/util/testing_hooks.h | 11 +++-- .../core/test/unit/testutil/async_testing.h | 4 +- .../core/test/unit/util/testing_hooks_test.cc | 49 +++++++++++++------ 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index ade2c56f670..51d0bc3729b 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -33,9 +33,11 @@ namespace { * invokes the function specified to the constructor. This allows easily * creating `ListenerRegistration` objects that call a lambda. */ -class RemoveDelegatingListenerRegistration final : public api::ListenerRegistration { +class RemoveDelegatingListenerRegistration final + : public api::ListenerRegistration { public: - RemoveDelegatingListenerRegistration(std::function delegate) : delegate_(std::move(delegate)) { + RemoveDelegatingListenerRegistration(std::function delegate) + : delegate_(std::move(delegate)) { } void Remove() override { @@ -46,14 +48,17 @@ class RemoveDelegatingListenerRegistration final : public api::ListenerRegistrat std::function delegate_; }; +} // namespace -} - -std::shared_ptr TestingHooks::OnExistenceFilterMismatch(ExistenceFilterMismatchCallback callback) { +std::shared_ptr +TestingHooks::OnExistenceFilterMismatch( + ExistenceFilterMismatchCallback callback) { // Register the callback. std::unique_lock lock(mutex_); const int id = next_id_++; - existence_filter_mismatch_callbacks_.insert({id, std::make_shared(std::move(callback))}); + existence_filter_mismatch_callbacks_.insert( + {id, + std::make_shared(std::move(callback))}); lock.unlock(); // Create a ListenerRegistration that the caller can use to unregister the @@ -71,9 +76,8 @@ void TestingHooks::NotifyOnExistenceFilterMismatch( const ExistenceFilterMismatchInfo& info) { std::lock_guard lock(mutex_); for (auto&& entry : existence_filter_mismatch_callbacks_) { - std::async([info, callback = entry.second]() { - callback->operator()(info); - }); + std::async( + [info, callback = entry.second]() { callback->operator()(info); }); } } diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index 033eaa83109..66866b5f202 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -35,7 +35,6 @@ namespace util { */ class TestingHooks final { public: - /** Returns the singleton instance of this class. */ static TestingHooks& GetInstance() { TestingHooks* instance = new TestingHooks; @@ -51,7 +50,8 @@ class TestingHooks final { int existenceFilterCount = -1; }; - using ExistenceFilterMismatchCallback = std::function; + using ExistenceFilterMismatchCallback = + std::function; /** * Registers a callback to be invoked when an existence filter mismatch occurs @@ -78,7 +78,8 @@ class TestingHooks final { * callback; only the first invocation of `Remove()` does anything; all * subsequent invocations do nothing. */ - std::shared_ptr OnExistenceFilterMismatch(ExistenceFilterMismatchCallback); + std::shared_ptr OnExistenceFilterMismatch( + ExistenceFilterMismatchCallback); /** * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks. @@ -98,8 +99,8 @@ class TestingHooks final { mutable std::mutex mutex_; int next_id_ = 0; - std::unordered_map> existence_filter_mismatch_callbacks_; - + std::unordered_map> + existence_filter_mismatch_callbacks_; }; } // namespace util diff --git a/Firestore/core/test/unit/testutil/async_testing.h b/Firestore/core/test/unit/testutil/async_testing.h index 1a501e6c7b7..59e34938512 100644 --- a/Firestore/core/test/unit/testutil/async_testing.h +++ b/Firestore/core/test/unit/testutil/async_testing.h @@ -161,9 +161,9 @@ class AsyncTest { * instances to be created with a shared_ptr via the `NewInstance()` method. */ template -class AsyncAccumulator final : public std::enable_shared_from_this> { +class AsyncAccumulator final + : public std::enable_shared_from_this> { public: - /** * Creates and returns a std::shared_ptr to a new instance of this class. */ diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index 02052394ce3..eaa43a28374 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -33,26 +33,34 @@ namespace util { // A "friend" class of TestingHooks to call its private members. class TestingHooksTestHelper { public: - TestingHooksTestHelper() : testing_hooks_(new TestingHooks) {} + TestingHooksTestHelper() : testing_hooks_(new TestingHooks) { + } std::shared_ptr testing_hooks_; }; -} // namespace util -} // namespace firestore -} // namespace firebase +} // namespace util +} // namespace firestore +} // namespace firebase namespace { using namespace std::chrono_literals; using firebase::firestore::api::ListenerRegistration; +using firebase::firestore::testutil::AsyncTest; using firebase::firestore::util::TestingHooks; using firebase::firestore::util::TestingHooksTestHelper; -using firebase::firestore::testutil::AsyncTest; -using ExistenceFilterMismatchInfoAccumulator = firebase::firestore::testutil::AsyncAccumulator; +using ExistenceFilterMismatchInfoAccumulator = + firebase::firestore::testutil::AsyncAccumulator< + TestingHooks::ExistenceFilterMismatchInfo>; -class TestingHooksTest : public ::testing::Test, public AsyncTest, public TestingHooksTestHelper { +class TestingHooksTest : public ::testing::Test, + public AsyncTest, + public TestingHooksTestHelper { public: - void AssertAccumulatedObject(const std::shared_ptr& accumulator, TestingHooks::ExistenceFilterMismatchInfo expected) { + void AssertAccumulatedObject( + const std::shared_ptr& + accumulator, + TestingHooks::ExistenceFilterMismatchInfo expected) { Await(accumulator->WaitForObject()); ASSERT_FALSE(accumulator->IsEmpty()); TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); @@ -60,8 +68,11 @@ class TestingHooksTest : public ::testing::Test, public AsyncTest, public Testin EXPECT_EQ(info.existenceFilterCount, expected.existenceFilterCount); } - std::future NotifyOnExistenceFilterMismatchAsync(TestingHooks::ExistenceFilterMismatchInfo info) { - return Async([info, testing_hooks = testing_hooks_]() { testing_hooks->NotifyOnExistenceFilterMismatch(info); }); + std::future NotifyOnExistenceFilterMismatchAsync( + TestingHooks::ExistenceFilterMismatchInfo info) { + return Async([info, testing_hooks = testing_hooks_]() { + testing_hooks->NotifyOnExistenceFilterMismatch(info); + }); } }; @@ -74,7 +85,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { AssertAccumulatedObject(accumulator, {123, 456}); } -TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotifiedMultipleTimes) { +TEST_F(TestingHooksTest, + OnExistenceFilterMismatchCallbackShouldGetNotifiedMultipleTimes) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); @@ -86,7 +98,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotifiedMulti AssertAccumulatedObject(accumulator, {555, 666}); } -TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) { +TEST_F(TestingHooksTest, + OnExistenceFilterMismatchAllCallbacksShouldGetNotified) { auto accumulator1 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); @@ -98,9 +111,11 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) AssertAccumulatedObject(accumulator2, {123, 456}); } -TEST_F(TestingHooksTest, OnExistenceFilterMismatchShouldNotBeNotifiedAfterRemove) { +TEST_F(TestingHooksTest, + OnExistenceFilterMismatchShouldNotBeNotifiedAfterRemove) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - std::shared_ptr registration = testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + std::shared_ptr registration = + testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); registration->Remove(); NotifyOnExistenceFilterMismatchAsync({123, 456}); @@ -114,7 +129,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); auto accumulator3 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); - std::shared_ptr registration2 = testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); + std::shared_ptr registration2 = + testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); testing_hooks_->OnExistenceFilterMismatch(accumulator3->AsCallback()); registration2->Remove(); @@ -128,7 +144,8 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { TEST_F(TestingHooksTest, OnExistenceFilterMismatchMultipleRemovesHaveNoEffect) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - std::shared_ptr registration = testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + std::shared_ptr registration = + testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); registration->Remove(); registration->Remove(); registration->Remove(); From f57a2c68b5a56ac3daa3fe09eef608f587052e13 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 16:24:19 -0400 Subject: [PATCH 13/26] testing_hooks_util.h/cc added --- .../test/unit/testutil/testing_hooks_util.cc | 58 +++++++++++++++++++ .../test/unit/testutil/testing_hooks_util.h | 43 ++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 Firestore/core/test/unit/testutil/testing_hooks_util.cc create mode 100644 Firestore/core/test/unit/testutil/testing_hooks_util.h diff --git a/Firestore/core/test/unit/testutil/testing_hooks_util.cc b/Firestore/core/test/unit/testutil/testing_hooks_util.cc new file mode 100644 index 00000000000..ae2e458fad5 --- /dev/null +++ b/Firestore/core/test/unit/testutil/testing_hooks_util.cc @@ -0,0 +1,58 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "Firestore/core/test/unit/testutil/testing_hooks_util.h" + +#include +#include +#include +#include + +#include "Firestore/core/src/api/listener_registration.h" +#include "Firestore/core/src/util/defer.h" +#include "Firestore/core/src/util/testing_hooks.h" +#include "Firestore/core/test/unit/testutil/async_testing.h" + +namespace firebase { +namespace firestore { +namespace testutil { + +using util::Defer; +using util::TestingHooks; + +std::vector +CaptureExistenceFilterMismatches(std::function callback) { + auto accumulator = AsyncAccumulator< + TestingHooks::ExistenceFilterMismatchInfo>::NewInstance(); + + TestingHooks& testing_hooks = TestingHooks::GetInstance(); + std::shared_ptr registration = + testing_hooks.OnExistenceFilterMismatch(accumulator->AsCallback()); + Defer unregister_callback([registration]() { registration->Remove(); }); + + callback(); + + std::vector mismatches; + while (!accumulator->IsEmpty()) { + mismatches.push_back(accumulator->Shift()); + } + + return mismatches; +} + +} // namespace testutil +} // namespace firestore +} // namespace firebase diff --git a/Firestore/core/test/unit/testutil/testing_hooks_util.h b/Firestore/core/test/unit/testutil/testing_hooks_util.h new file mode 100644 index 00000000000..b6827636fd4 --- /dev/null +++ b/Firestore/core/test/unit/testutil/testing_hooks_util.h @@ -0,0 +1,43 @@ +/* + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef FIRESTORE_CORE_TEST_UNIT_TESTUTIL_TESTING_HOOKS_UTIL_H_ +#define FIRESTORE_CORE_TEST_UNIT_TESTUTIL_TESTING_HOOKS_UTIL_H_ + +#include +#include + +#include "Firestore/core/src/util/testing_hooks.h" + +namespace firebase { +namespace firestore { +namespace testutil { + +/** + * Captures all existence filter mismatches in the Watch 'Listen' stream that + * occur during the execution of the given callback. + * @param callback The callback to invoke; during the invocation of this + * callback all existence filter mismatches will be captured. + * @return the captured existence filter mismatches. + */ +std::vector +CaptureExistenceFilterMismatches(std::function callback); + +} // namespace testutil +} // namespace firestore +} // namespace firebase + +#endif // FIRESTORE_CORE_TEST_UNIT_TESTUTIL_TESTING_HOOKS_UTIL_H_ From 90f8661c53ea73a43b513984ae96cb946699520b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 1 May 2023 15:42:39 -0400 Subject: [PATCH 14/26] code cleanup --- Firestore/core/src/util/testing_hooks.cc | 8 ++ Firestore/core/src/util/testing_hooks.h | 9 +- .../test/unit/testutil/testing_hooks_util.cc | 1 - .../core/test/unit/util/testing_hooks_test.cc | 109 ++++++++++++------ 4 files changed, 87 insertions(+), 40 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index 51d0bc3729b..5e065f8e7de 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -18,6 +18,7 @@ #include #include +#include #include #include "Firestore/core/src/util/no_destructor.h" @@ -61,6 +62,13 @@ TestingHooks::OnExistenceFilterMismatch( std::make_shared(std::move(callback))}); lock.unlock(); + // NOTE: Capturing `this` in the lambda below is safe because the destructor + // is deleted and, therefore, `this` can never be deleted. The static_assert + // statements below verify this invariant. + using this_type = std::remove_pointer::type; + static_assert(std::is_same::value, ""); + static_assert(!std::is_destructible::value, ""); + // Create a ListenerRegistration that the caller can use to unregister the // callback. return std::make_shared([this, id]() { diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index 66866b5f202..9f6b74934a8 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -23,6 +23,7 @@ #include #include "Firestore/core/src/api/listener_registration.h" +#include "Firestore/core/src/util/no_destructor.h" namespace firebase { namespace firestore { @@ -37,7 +38,7 @@ class TestingHooks final { public: /** Returns the singleton instance of this class. */ static TestingHooks& GetInstance() { - TestingHooks* instance = new TestingHooks; + static NoDestructor instance; return *instance; } @@ -90,12 +91,16 @@ class TestingHooks final { private: TestingHooks() = default; + // Delete the destructor so that the singleton instance of this class can + // never be deleted. + ~TestingHooks() = delete; + TestingHooks(const TestingHooks&) = delete; TestingHooks(TestingHooks&&) = delete; TestingHooks& operator=(const TestingHooks&) = delete; TestingHooks& operator=(TestingHooks&&) = delete; - friend class TestingHooksTestHelper; + friend class NoDestructor; mutable std::mutex mutex_; int next_id_ = 0; diff --git a/Firestore/core/test/unit/testutil/testing_hooks_util.cc b/Firestore/core/test/unit/testutil/testing_hooks_util.cc index ae2e458fad5..e987e23be30 100644 --- a/Firestore/core/test/unit/testutil/testing_hooks_util.cc +++ b/Firestore/core/test/unit/testutil/testing_hooks_util.cc @@ -18,7 +18,6 @@ #include #include -#include #include #include "Firestore/core/src/api/listener_registration.h" diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index eaa43a28374..098fd09969d 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -22,40 +22,25 @@ #include #include "Firestore/core/src/api/listener_registration.h" +#include "Firestore/core/src/util/defer.h" #include "Firestore/core/test/unit/testutil/async_testing.h" #include "gtest/gtest.h" -namespace firebase { -namespace firestore { -namespace util { - -// A "friend" class of TestingHooks to call its private members. -class TestingHooksTestHelper { - public: - TestingHooksTestHelper() : testing_hooks_(new TestingHooks) { - } - std::shared_ptr testing_hooks_; -}; - -} // namespace util -} // namespace firestore -} // namespace firebase - namespace { using namespace std::chrono_literals; + using firebase::firestore::api::ListenerRegistration; using firebase::firestore::testutil::AsyncTest; +using firebase::firestore::util::Defer; using firebase::firestore::util::TestingHooks; -using firebase::firestore::util::TestingHooksTestHelper; + using ExistenceFilterMismatchInfoAccumulator = firebase::firestore::testutil::AsyncAccumulator< TestingHooks::ExistenceFilterMismatchInfo>; -class TestingHooksTest : public ::testing::Test, - public AsyncTest, - public TestingHooksTestHelper { +class TestingHooksTest : public ::testing::Test, public AsyncTest { public: void AssertAccumulatedObject( const std::shared_ptr& @@ -70,15 +55,24 @@ class TestingHooksTest : public ::testing::Test, std::future NotifyOnExistenceFilterMismatchAsync( TestingHooks::ExistenceFilterMismatchInfo info) { - return Async([info, testing_hooks = testing_hooks_]() { - testing_hooks->NotifyOnExistenceFilterMismatch(info); + return Async([info]() { + TestingHooks::GetInstance().NotifyOnExistenceFilterMismatch(info); }); } }; +TEST_F(TestingHooksTest, GetInstanceShouldAlwaysReturnTheSameObject) { + TestingHooks& testing_hooks1 = TestingHooks::GetInstance(); + TestingHooks& testing_hooks2 = TestingHooks::GetInstance(); + EXPECT_EQ(&testing_hooks1, &testing_hooks2); +} + TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + std::shared_ptr listener_registration = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); + Defer unregister_listener([=] { listener_registration->Remove(); }); NotifyOnExistenceFilterMismatchAsync({123, 456}); @@ -88,7 +82,10 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotified) { TEST_F(TestingHooksTest, OnExistenceFilterMismatchCallbackShouldGetNotifiedMultipleTimes) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + std::shared_ptr listener_registration = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); + Defer unregister_listener([=] { listener_registration->Remove(); }); NotifyOnExistenceFilterMismatchAsync({111, 222}); AssertAccumulatedObject(accumulator, {111, 222}); @@ -102,8 +99,14 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchAllCallbacksShouldGetNotified) { auto accumulator1 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); - testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); + std::shared_ptr listener_registration1 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator1->AsCallback()); + Defer unregister_listener1([=] { listener_registration1->Remove(); }); + std::shared_ptr listener_registration2 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator2->AsCallback()); + Defer unregister_listener2([=] { listener_registration2->Remove(); }); NotifyOnExistenceFilterMismatchAsync({123, 456}); @@ -111,11 +114,32 @@ TEST_F(TestingHooksTest, AssertAccumulatedObject(accumulator2, {123, 456}); } +TEST_F(TestingHooksTest, + OnExistenceFilterMismatchCallbackShouldGetNotifiedOncePerRegistration) { + auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); + std::shared_ptr listener_registration1 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); + Defer unregister_listener1([=] { listener_registration1->Remove(); }); + std::shared_ptr listener_registration2 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); + Defer unregister_listener2([=] { listener_registration1->Remove(); }); + + NotifyOnExistenceFilterMismatchAsync({123, 456}); + + AssertAccumulatedObject(accumulator, {123, 456}); + AssertAccumulatedObject(accumulator, {123, 456}); + std::this_thread::sleep_for(250ms); + EXPECT_TRUE(accumulator->IsEmpty()); +} + TEST_F(TestingHooksTest, OnExistenceFilterMismatchShouldNotBeNotifiedAfterRemove) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); std::shared_ptr registration = - testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); registration->Remove(); NotifyOnExistenceFilterMismatchAsync({123, 456}); @@ -128,11 +152,20 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { auto accumulator1 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); auto accumulator2 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); auto accumulator3 = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - testing_hooks_->OnExistenceFilterMismatch(accumulator1->AsCallback()); - std::shared_ptr registration2 = - testing_hooks_->OnExistenceFilterMismatch(accumulator2->AsCallback()); - testing_hooks_->OnExistenceFilterMismatch(accumulator3->AsCallback()); - registration2->Remove(); + std::shared_ptr listener_registration1 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator1->AsCallback()); + Defer unregister_listener1([=] { listener_registration1->Remove(); }); + std::shared_ptr listener_registration2 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator2->AsCallback()); + Defer unregister_listener2([=] { listener_registration1->Remove(); }); + std::shared_ptr listener_registration3 = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator3->AsCallback()); + Defer unregister_listener3([=] { listener_registration3->Remove(); }); + + listener_registration2->Remove(); NotifyOnExistenceFilterMismatchAsync({123, 456}); @@ -144,11 +177,13 @@ TEST_F(TestingHooksTest, OnExistenceFilterMismatchRemoveShouldOnlyRemoveOne) { TEST_F(TestingHooksTest, OnExistenceFilterMismatchMultipleRemovesHaveNoEffect) { auto accumulator = ExistenceFilterMismatchInfoAccumulator::NewInstance(); - std::shared_ptr registration = - testing_hooks_->OnExistenceFilterMismatch(accumulator->AsCallback()); - registration->Remove(); - registration->Remove(); - registration->Remove(); + std::shared_ptr listener_registration = + TestingHooks::GetInstance().OnExistenceFilterMismatch( + accumulator->AsCallback()); + Defer unregister_listener([=] { listener_registration->Remove(); }); + listener_registration->Remove(); + listener_registration->Remove(); + listener_registration->Remove(); NotifyOnExistenceFilterMismatchAsync({123, 456}); From 024a8fefe78a517433cd445c59c16633fcdb0cce Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 1 May 2023 15:57:58 -0400 Subject: [PATCH 15/26] Firestore.xcodeproj/project.pbxproj updated --- .../Firestore.xcodeproj/project.pbxproj | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index 6f6e25e705b..3bf403087f3 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -145,6 +145,7 @@ 1733601ECCEA33E730DEAF45 /* autoid_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54740A521FC913E500713A1A /* autoid_test.cc */; }; 17473086EBACB98CDC3CC65C /* view_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = C7429071B33BDF80A7FA2F8A /* view_test.cc */; }; 17638F813B9B556FE7718C0C /* FIRQuerySnapshotTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04F202154AA00B64F25 /* FIRQuerySnapshotTests.mm */; }; + 179B8752CF4255263B9986FD /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; 17DC97DE15D200932174EC1F /* defer_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 8ABAC2E0402213D837F73DC3 /* defer_test.cc */; }; 17DFF30CF61D87883986E8B6 /* executor_std_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4687208F9B9100554BA2 /* executor_std_test.cc */; }; 17ECB768DA44AE0F49647E22 /* memory_query_engine_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 8EF6A33BC2D84233C355F1D0 /* memory_query_engine_test.cc */; }; @@ -157,6 +158,7 @@ 198F193BD9484E49375A7BE7 /* FSTHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03A2021401F00B64F25 /* FSTHelpers.mm */; }; 199B778D5820495797E0BE02 /* filesystem_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = F51859B394D01C0C507282F1 /* filesystem_test.cc */; }; 1A1299107EFF68DA9DAB19BD /* leveldb_overlay_migration_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = D8A6D52723B1BABE1B7B8D8F /* leveldb_overlay_migration_manager_test.cc */; }; + 1A21C6A5AC8A153E96F91566 /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; 1A3D8028303B45FCBB21CAD3 /* aggregation_result.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = D872D754B8AD88E28AF28B28 /* aggregation_result.pb.cc */; }; 1B4794A51F4266556CD0976B /* view_snapshot_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = CC572A9168BBEF7B83E4BBC5 /* view_snapshot_test.cc */; }; 1B6E74BA33B010D76DB1E2F9 /* FIRGeoPointTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E048202154AA00B64F25 /* FIRGeoPointTests.mm */; }; @@ -210,11 +212,14 @@ 23EFC681986488B033C2B318 /* leveldb_opener_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 75860CD13AF47EB1EA39EC2F /* leveldb_opener_test.cc */; }; 2428E92E063EBAEA44BA5913 /* target_index_matcher_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 63136A2371C0C013EC7A540C /* target_index_matcher_test.cc */; }; 248DE4F56DD938F4DBCCF39B /* bundle_reader_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 6ECAF7DE28A19C69DF386D88 /* bundle_reader_test.cc */; }; + 24B75C63BDCD5551B2F69901 /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; 24CB39421C63CD87242B31DF /* bundle_reader_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 6ECAF7DE28A19C69DF386D88 /* bundle_reader_test.cc */; }; 254CD651CB621D471BC5AC12 /* target_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B5C37696557C81A6C2B7271A /* target_cache_test.cc */; }; 258B372CF33B7E7984BBA659 /* fake_target_metadata_provider.cc in Sources */ = {isa = PBXBuildFile; fileRef = 71140E5D09C6E76F7C71B2FC /* fake_target_metadata_provider.cc */; }; + 258F2B14F7E4C615707E67B1 /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; 25A75DFA730BAD21A5538EC5 /* document.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D821C2DDC800EFB9CC /* document.pb.cc */; }; 25C167BAA4284FC951206E1F /* FIRFirestoreTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5467FAFF203E56F8009C9584 /* FIRFirestoreTests.mm */; }; + 25DCB9BD1C681C6611A17164 /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; 25FE27330996A59F31713A0C /* FIRDocumentReferenceTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E049202154AA00B64F25 /* FIRDocumentReferenceTests.mm */; }; 2620644052E960310DADB298 /* FIRFieldValueTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E04A202154AA00B64F25 /* FIRFieldValueTests.mm */; }; 2634E1C1971C05790B505824 /* resource_path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B686F2B02024FFD70028D6BE /* resource_path_test.cc */; }; @@ -435,6 +440,7 @@ 518BF03D57FBAD7C632D18F8 /* FIRQueryUnitTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = FF73B39D04D1760190E6B84A /* FIRQueryUnitTests.mm */; }; 52967C3DD7896BFA48840488 /* byte_string_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5342CDDB137B4E93E2E85CCA /* byte_string_test.cc */; }; 529AB59F636060FEA21BD4FF /* garbage_collection_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = AAED89D7690E194EF3BA1132 /* garbage_collection_spec_test.json */; }; + 5360D52DCAD1069B1E4B0B9D /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; 53AB47E44D897C81A94031F6 /* write.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D921C2DDC800EFB9CC /* write.pb.cc */; }; 53BBB5CDED453F923ADD08D2 /* stream_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 5B5414D28802BC76FDADABD6 /* stream_test.cc */; }; 53F449F69DF8A3ABC711FD59 /* secure_random_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 54740A531FC913E500713A1A /* secure_random_test.cc */; }; @@ -662,6 +668,7 @@ 6359EA7D5C76D462BD31B5E5 /* watch_change_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 2D7472BC70C024D736FF74D9 /* watch_change_test.cc */; }; 6380CACCF96A9B26900983DC /* leveldb_target_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = E76F0CDF28E5FA62D21DE648 /* leveldb_target_cache_test.cc */; }; 63B91FC476F3915A44F00796 /* query.pb.cc in Sources */ = {isa = PBXBuildFile; fileRef = 544129D621C2DDC800EFB9CC /* query.pb.cc */; }; + 64055C7B3BF2C4106714DD3C /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; 650B31A5EC6F8D2AEA79C350 /* index_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AE4A9E38D65688EE000EE2A1 /* index_manager_test.cc */; }; 65537B22A73E3909666FB5BC /* remote_document_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 7EB299CF85034F09CFD6F3FD /* remote_document_cache_test.cc */; }; 65D54B964A2021E5A36AB21F /* bundle_loader_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A853C81A6A5A51C9D0389EDA /* bundle_loader_test.cc */; }; @@ -913,6 +920,7 @@ 9A8B01AF6F19D248202FBC0A /* FIRQueryUnitTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = FF73B39D04D1760190E6B84A /* FIRQueryUnitTests.mm */; }; 9AC28D928902C6767A11F5FC /* objc_type_traits_apple_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = 2A0CF41BA5AED6049B0BEB2C /* objc_type_traits_apple_test.mm */; }; 9AC604BF7A76CABDF26F8C8E /* cc_compilation_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 1B342370EAE3AA02393E33EB /* cc_compilation_test.cc */; }; + 9B2C6A48A4DBD36080932B4E /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; 9B2CD4CBB1DFE8BC3C81A335 /* async_queue_libdispatch_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = B6FB4680208EA0BE00554BA2 /* async_queue_libdispatch_test.mm */; }; 9B9BFC16E26BDE4AE0CDFF4B /* firebase_auth_credentials_provider_test.mm in Sources */ = {isa = PBXBuildFile; fileRef = F869D85E900E5AF6CD02E2FC /* firebase_auth_credentials_provider_test.mm */; }; 9BEC62D59EB2C68342F493CD /* credentials_provider_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 2F4FA4576525144C5069A7A5 /* credentials_provider_test.cc */; }; @@ -1115,6 +1123,7 @@ C1E35BCE2CFF9B56C28545A2 /* Pods_Firestore_Example_tvOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 62E103B28B48A81D682A0DE9 /* Pods_Firestore_Example_tvOS.framework */; }; C1F196EC5A7C112D2F7C7724 /* view_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = C7429071B33BDF80A7FA2F8A /* view_test.cc */; }; C1F8991BD11FFD705D74244F /* random_access_queue_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 014C60628830D95031574D15 /* random_access_queue_test.cc */; }; + C201BE60B8717CDCBEEDF3F0 /* testing_hooks_util.cc in Sources */ = {isa = PBXBuildFile; fileRef = F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */; }; C21B3A1CCB3AD42E57EA14FC /* Pods_Firestore_Tests_macOS.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = 759E964B6A03E6775C992710 /* Pods_Firestore_Tests_macOS.framework */; }; C23552A6D9FB0557962870C2 /* local_store_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 307FF03D0297024D59348EBD /* local_store_test.cc */; }; C25F321AC9BF8D1CFC8543AF /* reference_set_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 132E32997D781B896672D30A /* reference_set_test.cc */; }; @@ -1172,6 +1181,7 @@ CFF1EBC60A00BA5109893C6E /* memory_index_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = DB5A1E760451189DA36028B3 /* memory_index_manager_test.cc */; }; D00E69F7FDF2BE674115AD3F /* field_path_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = B686F2AD2023DDB20028D6BE /* field_path_test.cc */; }; D04CBBEDB8DC16D8C201AC49 /* leveldb_target_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = E76F0CDF28E5FA62D21DE648 /* leveldb_target_cache_test.cc */; }; + D0DA42DC66C4FE508A63B269 /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; D143FBD057481C1A59B27E5E /* persistence_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 54DA12A31F315EE100DD57A1 /* persistence_spec_test.json */; }; D156B9F19B5B29E77664FDFC /* logic_utils_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 28B45B2104E2DAFBBF86DBB7 /* logic_utils_test.cc */; }; D1690214781198276492442D /* event_manager_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 6F57521E161450FAF89075ED /* event_manager_test.cc */; }; @@ -1331,6 +1341,7 @@ F0C8EB1F4FB56401CFA4F374 /* object_value_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 214877F52A705012D6720CA0 /* object_value_test.cc */; }; F0EA84FB66813F2BC164EF7C /* token_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A082AFDD981B07B5AD78FDE8 /* token_test.cc */; }; F10A3E4E164A5458DFF7EDE6 /* leveldb_remote_document_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 0840319686A223CC4AD3FAB1 /* leveldb_remote_document_cache_test.cc */; }; + F184E5367DF3CA158EDE8532 /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; F19B749671F2552E964422F7 /* FIRListenerRegistrationTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E06B202154D500B64F25 /* FIRListenerRegistrationTests.mm */; }; F1EAEE9DF819C017A9506AEB /* FIRIndexingTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 795AA8FC31D2AF6864B07D39 /* FIRIndexingTests.mm */; }; F272A8C41D2353700A11D1FB /* field_mask_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 549CCA5320A36E1F00BCEB75 /* field_mask_test.cc */; }; @@ -1350,6 +1361,7 @@ F6079BFC9460B190DA85C2E6 /* pretty_printing_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = AB323F9553050F4F6490F9FF /* pretty_printing_test.cc */; }; F609600E9A88A4D44FD1FCEB /* FSTSpecTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03020213FFC00B64F25 /* FSTSpecTests.mm */; }; F660788F69B4336AC6CD2720 /* offline_spec_test.json in Resources */ = {isa = PBXBuildFile; fileRef = 54DA12A11F315EE100DD57A1 /* offline_spec_test.json */; }; + F6738D3B72352BBEFB87172C /* testing_hooks_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */; }; F696B7467E80E370FDB3EAA7 /* remote_document_cache_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 7EB299CF85034F09CFD6F3FD /* remote_document_cache_test.cc */; }; F6BC4D3E336F3CE0782BCC34 /* memory_query_engine_test.cc in Sources */ = {isa = PBXBuildFile; fileRef = 8EF6A33BC2D84233C355F1D0 /* memory_query_engine_test.cc */; }; F72DF72447EA7AB9D100816A /* FSTHelpers.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5492E03A2021401F00B64F25 /* FSTHelpers.mm */; }; @@ -1619,6 +1631,7 @@ 5C7942B6244F4C416B11B86C /* leveldb_mutation_queue_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_mutation_queue_test.cc; sourceTree = ""; }; 5CAE131920FFFED600BE9A4A /* Firestore_Benchmarks_iOS.xctest */ = {isa = PBXFileReference; explicitFileType = wrapper.cfbundle; includeInIndex = 0; path = Firestore_Benchmarks_iOS.xctest; sourceTree = BUILT_PRODUCTS_DIR; }; 5CAE131D20FFFED600BE9A4A /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; + 5CFCF090AC17BC8D36640F88 /* testing_hooks_util.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = testing_hooks_util.h; sourceTree = ""; }; 5E19B9B2105BA618DA9EE99C /* query_engine_test.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = query_engine_test.h; sourceTree = ""; }; 5FF903AEFA7A3284660FA4C5 /* leveldb_local_store_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_local_store_test.cc; sourceTree = ""; }; 6003F58A195388D20070C39A /* Firestore_Example_iOS.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = Firestore_Example_iOS.app; sourceTree = BUILT_PRODUCTS_DIR; }; @@ -1713,12 +1726,12 @@ 9098A0C535096F2EE9C35DE0 /* create_noop_connectivity_monitor.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = create_noop_connectivity_monitor.h; sourceTree = ""; }; 9113B6F513D0473AEABBAF1F /* persistence_testing.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = persistence_testing.cc; sourceTree = ""; }; 9765D47FA12FA283F4EFAD02 /* memory_lru_garbage_collector_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = memory_lru_garbage_collector_test.cc; sourceTree = ""; }; - 97C492D2524E92927C11F425 /* Pods-Firestore_FuzzTests_iOS.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_FuzzTests_iOS.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_FuzzTests_iOS/Pods-Firestore_FuzzTests_iOS.release.xcconfig"; sourceTree = ""; }; 98366480BD1FD44A1FEDD982 /* Pods-Firestore_Example_macOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_macOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_macOS/Pods-Firestore_Example_macOS.debug.xcconfig"; sourceTree = ""; }; 99434327614FEFF7F7DC88EC /* counting_query_engine.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = counting_query_engine.cc; sourceTree = ""; }; 9B0B005A79E765AF02793DCE /* schedule_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = schedule_test.cc; sourceTree = ""; }; 9C1AFCC9E616EC33D6E169CF /* recovery_spec_test.json */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.json; path = recovery_spec_test.json; sourceTree = ""; }; 9CFD366B783AE27B9E79EE7A /* string_format_apple_test.mm */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.objcpp; path = string_format_apple_test.mm; sourceTree = ""; }; + A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; path = testing_hooks_test.cc; sourceTree = ""; }; A082AFDD981B07B5AD78FDE8 /* token_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; name = token_test.cc; path = credentials/token_test.cc; sourceTree = ""; }; A366F6AE1A5A77548485C091 /* bundle.pb.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = bundle.pb.cc; sourceTree = ""; }; A5466E7809AD2871FFDE6C76 /* view_testing.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = view_testing.cc; sourceTree = ""; }; @@ -1823,6 +1836,7 @@ EF83ACD5E1E9F25845A9ACED /* leveldb_migrations_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = leveldb_migrations_test.cc; sourceTree = ""; }; F02F734F272C3C70D1307076 /* filter_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = filter_test.cc; sourceTree = ""; }; F119BDDF2F06B3C0883B8297 /* firebase_app_check_credentials_provider_test.mm */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.objcpp; name = firebase_app_check_credentials_provider_test.mm; path = credentials/firebase_app_check_credentials_provider_test.mm; sourceTree = ""; }; + F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */ = {isa = PBXFileReference; includeInIndex = 1; path = testing_hooks_util.cc; sourceTree = ""; }; F354C0FE92645B56A6C6FD44 /* Pods-Firestore_IntegrationTests_iOS.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_IntegrationTests_iOS.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_IntegrationTests_iOS/Pods-Firestore_IntegrationTests_iOS.release.xcconfig"; sourceTree = ""; }; F51859B394D01C0C507282F1 /* filesystem_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = filesystem_test.cc; sourceTree = ""; }; F694C3CE4B77B3C0FA4BBA53 /* Pods_Firestore_Benchmarks_iOS.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Pods_Firestore_Benchmarks_iOS.framework; sourceTree = BUILT_PRODUCTS_DIR; }; @@ -2026,6 +2040,8 @@ 64AA92CFA356A2360F3C5646 /* filesystem_testing.h */, 3CAA33F964042646FDDAF9F9 /* status_testing.cc */, 4334F87873015E3763954578 /* status_testing.h */, + F1ADF4E1991C352F0ECCE1E7 /* testing_hooks_util.cc */, + 5CFCF090AC17BC8D36640F88 /* testing_hooks_util.h */, 54A0352820A3B3BD003E0143 /* testutil.cc */, 54A0352920A3B3BD003E0143 /* testutil.h */, 5497CB76229DECDE000FB92F /* time_testing.cc */, @@ -2104,6 +2120,7 @@ AB380CFC201A2EE200D97691 /* string_util_test.cc */, 79507DF8378D3C42F5B36268 /* string_win_test.cc */, 899FC22684B0F7BEEAE13527 /* task_test.cc */, + A002425BC4FC4E805F4175B6 /* testing_hooks_test.cc */, B68B1E002213A764008977EF /* to_string_apple_test.mm */, B696858D2214B53900271095 /* to_string_test.cc */, ); @@ -3811,6 +3828,8 @@ B384E0F90D4CCC15C88CAF30 /* target_index_matcher_test.cc in Sources */, 55427A6CFFB22E069DCC0CC4 /* target_test.cc in Sources */, 88929ED628DA8DD9592974ED /* task_test.cc in Sources */, + 9B2C6A48A4DBD36080932B4E /* testing_hooks_test.cc in Sources */, + 64055C7B3BF2C4106714DD3C /* testing_hooks_util.cc in Sources */, 32A95242C56A1A230231DB6A /* testutil.cc in Sources */, 5497CB78229DECDE000FB92F /* time_testing.cc in Sources */, ACC9369843F5ED3BD2284078 /* timestamp_test.cc in Sources */, @@ -4019,6 +4038,8 @@ 2428E92E063EBAEA44BA5913 /* target_index_matcher_test.cc in Sources */, EB2137E6FBB0DDE2DF80E3D0 /* target_test.cc in Sources */, 67CF9FAA890307780731E1DA /* task_test.cc in Sources */, + 24B75C63BDCD5551B2F69901 /* testing_hooks_test.cc in Sources */, + 258F2B14F7E4C615707E67B1 /* testing_hooks_util.cc in Sources */, 8388418F43042605FB9BFB92 /* testutil.cc in Sources */, 5497CB79229DECDE000FB92F /* time_testing.cc in Sources */, 26CB3D7C871BC56456C6021E /* timestamp_test.cc in Sources */, @@ -4245,6 +4266,8 @@ C8722550B56CEB96F84DCE94 /* target_index_matcher_test.cc in Sources */, 35FEB53E165518C0DE155CB0 /* target_test.cc in Sources */, 76A5447D76F060E996555109 /* task_test.cc in Sources */, + D0DA42DC66C4FE508A63B269 /* testing_hooks_test.cc in Sources */, + 179B8752CF4255263B9986FD /* testing_hooks_util.cc in Sources */, 409C0F2BFC2E1BECFFAC4D32 /* testutil.cc in Sources */, 6300709ECDE8E0B5A8645F8D /* time_testing.cc in Sources */, 0CEE93636BA4852D3C5EC428 /* timestamp_test.cc in Sources */, @@ -4471,6 +4494,8 @@ 15A5DEC8430E71D64424CBFD /* target_index_matcher_test.cc in Sources */, 035DE410628A8F804F6F2790 /* target_test.cc in Sources */, 93C8F772F4DC5A985FA3D815 /* task_test.cc in Sources */, + F6738D3B72352BBEFB87172C /* testing_hooks_test.cc in Sources */, + 1A21C6A5AC8A153E96F91566 /* testing_hooks_util.cc in Sources */, A17DBC8F24127DA8A381F865 /* testutil.cc in Sources */, A25FF76DEF542E01A2DF3B0E /* time_testing.cc in Sources */, 1E42CD0F60EB22A5D0C86D1F /* timestamp_test.cc in Sources */, @@ -4689,6 +4714,8 @@ F27347560A963E8162C56FF3 /* target_index_matcher_test.cc in Sources */, 205601D1C6A40A4DD3BBAA04 /* target_test.cc in Sources */, 662793139A36E5CFC935B949 /* task_test.cc in Sources */, + F184E5367DF3CA158EDE8532 /* testing_hooks_test.cc in Sources */, + 25DCB9BD1C681C6611A17164 /* testing_hooks_util.cc in Sources */, 54A0352A20A3B3BD003E0143 /* testutil.cc in Sources */, 5497CB77229DECDE000FB92F /* time_testing.cc in Sources */, ABF6506C201131F8005F2C74 /* timestamp_test.cc in Sources */, @@ -4934,6 +4961,8 @@ 84E75527F3739131C09BEAA5 /* target_index_matcher_test.cc in Sources */, 7D25D41B013BB70ADE526055 /* target_test.cc in Sources */, C57B15CADD8C3E806B154C19 /* task_test.cc in Sources */, + 5360D52DCAD1069B1E4B0B9D /* testing_hooks_test.cc in Sources */, + C201BE60B8717CDCBEEDF3F0 /* testing_hooks_util.cc in Sources */, CA989C0E6020C372A62B7062 /* testutil.cc in Sources */, 2D220B9ABFA36CD7AC43D0A7 /* time_testing.cc in Sources */, D91D86B29B86A60C05879A48 /* timestamp_test.cc in Sources */, From c919bdefda8c69175482c8018b89268bfc891630 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 1 May 2023 16:12:10 -0400 Subject: [PATCH 16/26] start adding logic to verify the existence filter --- .../Tests/Integration/API/FIRQueryTests.mm | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 93791bae165..2c36a34e221 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -22,6 +22,8 @@ #import "Firestore/Example/Tests/Util/FSTHelpers.h" #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h" +#include "Firestore/core/test/unit/testutil/testing_hooks_util.h" + namespace { NSArray *SortedStringsNotIn(NSSet *set, NSSet *remove) { @@ -1236,8 +1238,13 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { [NSThread sleepForTimeInterval:10.0f]; // Resume the query and save the resulting snapshot for verification. - FIRQuerySnapshot *querySnapshot2 = [self readDocumentSetForRef:collRef - source:FIRFirestoreSourceDefault]; + // Use some internal testing hooks to "capture" the existence filter mismatches to verify them. + FIRQuerySnapshot *querySnapshot2; + std::vector + existence_filter_mismatches = + firebase::firestore::testutil::CaptureExistenceFilterMismatches([&] { + querySnapshot2 = [self readDocumentSetForRef:collRef source:FIRFirestoreSourceDefault]; + }); // Verify that the snapshot from the resumed query contains the expected documents; that is, // that it contains the 50 documents that were _not_ deleted. @@ -1272,6 +1279,17 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { [missingDocumentIds componentsJoinedByString:@", "]); } } + + // Skip the verification of the existence filter mismatch when testing against the Firestore + // emulator because the Firestore emulator fails to to send an existence filter at all. + // TODO(b/270731363): Enable the verification of the existence filter mismatch once the Firestore + // emulator is fixed to send an existence filter. + if ([FSTIntegrationTestCase isRunningAgainstEmulator]) { + return; + } + + // Verify that Watch sent an existence filter with the correct counts when the query was resumed. + XCTAssertEqual(static_cast(existence_filter_mismatches.size()), 1); } @end From 945cf847d37e972509ae3d07e1db5c8f0f586148 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 1 May 2023 16:29:25 -0400 Subject: [PATCH 17/26] fix GetInstance() incorrectly inlining, causing different instances to be returned --- Firestore/core/src/util/testing_hooks.cc | 6 ++++++ Firestore/core/src/util/testing_hooks.h | 5 +---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index 5e065f8e7de..e122d4ef5ce 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -51,6 +51,12 @@ class RemoveDelegatingListenerRegistration final } // namespace +/** Returns the singleton instance of this class. */ +TestingHooks& TestingHooks::GetInstance() { + static NoDestructor instance; + return *instance; +} + std::shared_ptr TestingHooks::OnExistenceFilterMismatch( ExistenceFilterMismatchCallback callback) { diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index 9f6b74934a8..2e501e2cab0 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -37,10 +37,7 @@ namespace util { class TestingHooks final { public: /** Returns the singleton instance of this class. */ - static TestingHooks& GetInstance() { - static NoDestructor instance; - return *instance; - } + static TestingHooks& GetInstance(); /** * Information about an existence filter mismatch, as specified to callbacks From e5464211592469611b0bc7a404b86d0de6081000 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 May 2023 11:56:51 -0400 Subject: [PATCH 18/26] testing_hooks.cc: make callbacks synchronous, for simplicity --- Firestore/core/src/util/testing_hooks.cc | 26 ++++++++++++++++++++---- Firestore/core/src/util/testing_hooks.h | 17 +++++++++------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index e122d4ef5ce..e55e46ca3e5 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -16,10 +16,11 @@ #include "Firestore/core/src/util/testing_hooks.h" -#include +#include #include #include #include +#include #include "Firestore/core/src/util/no_destructor.h" @@ -88,10 +89,27 @@ TestingHooks::OnExistenceFilterMismatch( void TestingHooks::NotifyOnExistenceFilterMismatch( const ExistenceFilterMismatchInfo& info) { - std::lock_guard lock(mutex_); + std::unique_lock lock(mutex_); + + // Short-circuit to avoid any unnecessary work if there is nothing to do. + if (existence_filter_mismatch_callbacks_.empty()) { + return; + } + + // Copy the callbacks into a vector so that they can be invoked after + // releasing the lock. + std::vector> callbacks; for (auto&& entry : existence_filter_mismatch_callbacks_) { - std::async( - [info, callback = entry.second]() { callback->operator()(info); }); + callbacks.push_back(entry.second); + } + + // Release the lock so that the callback invocations are done _without_ + // holding the lock. This avoids deadlock in the case that invocations are + // re-entrant. + lock.unlock(); + + for (std::shared_ptr callback : callbacks) { + callback->operator()(info); } } diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index 2e501e2cab0..f2ee5134600 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -59,11 +59,11 @@ class TestingHooks final { * rely on any particular ordering. If a given callback is registered multiple * times then it will be notified multiple times, once per registration. * - * The thread on which the callback occurs is unspecified; listeners should - * perform their work as quickly as possible and return to avoid blocking any - * critical work. In particular, the listener callbacks should *not* block or - * perform long-running operations. Listener callbacks can occur concurrently - * with other callbacks on the same and other listeners. + * The listener callbacks are performed synchronously in + * `NotifyOnExistenceFilterMismatch()`; therefore, listeners should perform + * their work as quickly as possible and return to avoid blocking any critical + * work. In particular, the listener callbacks should *not* block or perform + * long-running operations. * * The `ExistenceFilterMismatchInfo` reference specified to the callback is * only valid during the lifetime of the callback. Once the callback returns @@ -74,13 +74,16 @@ class TestingHooks final { * * @return an object whose `Remove()` member function unregisters the given * callback; only the first invocation of `Remove()` does anything; all - * subsequent invocations do nothing. + * subsequent invocations do nothing. Note that due to inherent race + * conditions it is technically possible, although unlikely, that callbacks + * could still occur _after_ unregistering. */ std::shared_ptr OnExistenceFilterMismatch( ExistenceFilterMismatchCallback); /** - * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks. + * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks + * synchronously. * @param info Information about the existence filter mismatch. */ void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&); From cac906f5616dc2d6b459631eafeffb3d6b858e27 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 May 2023 12:30:34 -0400 Subject: [PATCH 19/26] add // NOLINT(build/c++11) comments to make check_imports.swift happy --- Firestore/core/src/util/testing_hooks.cc | 3 ++- Firestore/core/test/unit/testutil/async_testing.h | 4 ++-- Firestore/core/test/unit/util/testing_hooks_test.cc | 7 ++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.cc b/Firestore/core/src/util/testing_hooks.cc index e55e46ca3e5..1f48fd0a812 100644 --- a/Firestore/core/src/util/testing_hooks.cc +++ b/Firestore/core/src/util/testing_hooks.cc @@ -17,10 +17,11 @@ #include "Firestore/core/src/util/testing_hooks.h" #include -#include +#include // NOLINT(build/c++11) #include #include #include +#include #include "Firestore/core/src/util/no_destructor.h" diff --git a/Firestore/core/test/unit/testutil/async_testing.h b/Firestore/core/test/unit/testutil/async_testing.h index 59e34938512..5e230043d48 100644 --- a/Firestore/core/test/unit/testutil/async_testing.h +++ b/Firestore/core/test/unit/testutil/async_testing.h @@ -18,13 +18,13 @@ #define FIRESTORE_CORE_TEST_UNIT_TESTUTIL_ASYNC_TESTING_H_ #include // NOLINT(build/c++11) -#include #include #include // NOLINT(build/c++11) #include -#include +#include // NOLINT(build/c++11) #include // NOLINT(build/c++11) #include +#include #include "gtest/gtest.h" diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index 098fd09969d..a8df325696a 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -16,10 +16,11 @@ #include "Firestore/core/src/util/testing_hooks.h" -#include -#include +#include // NOLINT(build/c++11) +#include // NOLINT(build/c++11) #include -#include +#include // NOLINT(build/c++11) +#include // NOLINT(build/c++11) #include "Firestore/core/src/api/listener_registration.h" #include "Firestore/core/src/util/defer.h" From bd64a0a7c7fd816eb981092d1c804aff8c21528a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 May 2023 12:37:29 -0400 Subject: [PATCH 20/26] finish integration test --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 4 ++++ Firestore/core/src/util/testing_hooks.h | 4 ++-- Firestore/core/test/unit/testutil/async_testing.h | 4 ++-- Firestore/core/test/unit/util/testing_hooks_test.cc | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 2c36a34e221..bbf86289c93 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1290,6 +1290,10 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { // Verify that Watch sent an existence filter with the correct counts when the query was resumed. XCTAssertEqual(static_cast(existence_filter_mismatches.size()), 1); + firebase::firestore::util::TestingHooks::ExistenceFilterMismatchInfo &info = + existence_filter_mismatches[0]; + XCTAssertEqual(info.local_cache_count, 100); + XCTAssertEqual(info.existence_filter_count, 50); } @end diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index f2ee5134600..9768d251bde 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -44,8 +44,8 @@ class TestingHooks final { * registered with `OnExistenceFilterMismatch()`. */ struct ExistenceFilterMismatchInfo { - int localCacheCount = -1; - int existenceFilterCount = -1; + int local_cache_count = -1; + int existence_filter_count = -1; }; using ExistenceFilterMismatchCallback = diff --git a/Firestore/core/test/unit/testutil/async_testing.h b/Firestore/core/test/unit/testutil/async_testing.h index 5e230043d48..682f4749f62 100644 --- a/Firestore/core/test/unit/testutil/async_testing.h +++ b/Firestore/core/test/unit/testutil/async_testing.h @@ -21,10 +21,10 @@ #include #include // NOLINT(build/c++11) #include -#include // NOLINT(build/c++11) +#include // NOLINT(build/c++11) #include // NOLINT(build/c++11) -#include #include +#include #include "gtest/gtest.h" diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index a8df325696a..0e51403624a 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -50,8 +50,8 @@ class TestingHooksTest : public ::testing::Test, public AsyncTest { Await(accumulator->WaitForObject()); ASSERT_FALSE(accumulator->IsEmpty()); TestingHooks::ExistenceFilterMismatchInfo info = accumulator->Shift(); - EXPECT_EQ(info.localCacheCount, expected.localCacheCount); - EXPECT_EQ(info.existenceFilterCount, expected.existenceFilterCount); + EXPECT_EQ(info.local_cache_count, expected.local_cache_count); + EXPECT_EQ(info.existence_filter_count, expected.existence_filter_count); } std::future NotifyOnExistenceFilterMismatchAsync( From 3ec30d1346fe3096ff5559faef1a74d86f6b25d7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 May 2023 12:45:49 -0400 Subject: [PATCH 21/26] fix lint warnings --- Firestore/core/src/util/testing_hooks.h | 2 +- Firestore/core/test/unit/util/testing_hooks_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index 9768d251bde..e84beaf157c 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -19,7 +19,7 @@ #include #include -#include +#include // NOLINT(build/c++11) #include #include "Firestore/core/src/api/listener_registration.h" diff --git a/Firestore/core/test/unit/util/testing_hooks_test.cc b/Firestore/core/test/unit/util/testing_hooks_test.cc index 0e51403624a..be7b9a2c8e6 100644 --- a/Firestore/core/test/unit/util/testing_hooks_test.cc +++ b/Firestore/core/test/unit/util/testing_hooks_test.cc @@ -30,7 +30,7 @@ namespace { -using namespace std::chrono_literals; +using namespace std::chrono_literals; // NOLINT(build/namespaces) using firebase::firestore::api::ListenerRegistration; using firebase::firestore::testutil::AsyncTest; From 9a69bd1176111cf806047ff5c7e0a252f668d0be Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 2 May 2023 15:37:45 -0400 Subject: [PATCH 22/26] testing_hooks.h: fix @param tags in docs --- Firestore/core/src/util/testing_hooks.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index e84beaf157c..d5818cd7843 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -79,14 +79,14 @@ class TestingHooks final { * could still occur _after_ unregistering. */ std::shared_ptr OnExistenceFilterMismatch( - ExistenceFilterMismatchCallback); + ExistenceFilterMismatchCallback callback); /** * Invokes all currently-registered `OnExistenceFilterMismatch` callbacks * synchronously. * @param info Information about the existence filter mismatch. */ - void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo&); + void NotifyOnExistenceFilterMismatch(const ExistenceFilterMismatchInfo& info); private: TestingHooks() = default; From f53340c1b0c3ff146c433bdc2ced05a28e01d1c1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 May 2023 11:09:55 -0400 Subject: [PATCH 23/26] FIRQueryTests.mm: call [self setContinueAfterFailure:NO] --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index bbf86289c93..fba25c382f7 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1193,6 +1193,10 @@ - (void)testOrderByEquality { } - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { + // Set this test to stop when the first failure occurs because some test assertion failures make + // the rest of the test not applicable or will even crash. + [self setContinueAfterFailure:NO]; + // Prepare the names and contents of the 100 documents to create. NSMutableDictionary *> *testDocs = [[NSMutableDictionary alloc] init]; From a89115ff541faf0d427132d9c186fffde160cc07 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 May 2023 11:11:46 -0400 Subject: [PATCH 24/26] Add docs for ExistenceFilterMismatchInfo member variables --- Firestore/core/src/util/testing_hooks.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Firestore/core/src/util/testing_hooks.h b/Firestore/core/src/util/testing_hooks.h index d5818cd7843..1e1ea0b1788 100644 --- a/Firestore/core/src/util/testing_hooks.h +++ b/Firestore/core/src/util/testing_hooks.h @@ -44,7 +44,13 @@ class TestingHooks final { * registered with `OnExistenceFilterMismatch()`. */ struct ExistenceFilterMismatchInfo { + /** The number of documents that matched the query in the local cache. */ int local_cache_count = -1; + + /** + * The number of documents that matched the query on the server, as + * specified in the `ExistenceFilter` message's `count` field. + */ int existence_filter_count = -1; }; From 2b0cb73ec673b87b66cac13a9bc27868596b79ca Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 May 2023 11:12:59 -0400 Subject: [PATCH 25/26] Firestore.xcodeproj/project.pbxproj: add back missing lines --- Firestore/Example/Firestore.xcodeproj/project.pbxproj | 1 + 1 file changed, 1 insertion(+) diff --git a/Firestore/Example/Firestore.xcodeproj/project.pbxproj b/Firestore/Example/Firestore.xcodeproj/project.pbxproj index 3bf403087f3..2ad7125dc7c 100644 --- a/Firestore/Example/Firestore.xcodeproj/project.pbxproj +++ b/Firestore/Example/Firestore.xcodeproj/project.pbxproj @@ -1726,6 +1726,7 @@ 9098A0C535096F2EE9C35DE0 /* create_noop_connectivity_monitor.h */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.c.h; path = create_noop_connectivity_monitor.h; sourceTree = ""; }; 9113B6F513D0473AEABBAF1F /* persistence_testing.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = persistence_testing.cc; sourceTree = ""; }; 9765D47FA12FA283F4EFAD02 /* memory_lru_garbage_collector_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = memory_lru_garbage_collector_test.cc; sourceTree = ""; }; + 97C492D2524E92927C11F425 /* Pods-Firestore_FuzzTests_iOS.release.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_FuzzTests_iOS.release.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_FuzzTests_iOS/Pods-Firestore_FuzzTests_iOS.release.xcconfig"; sourceTree = ""; }; 98366480BD1FD44A1FEDD982 /* Pods-Firestore_Example_macOS.debug.xcconfig */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = text.xcconfig; name = "Pods-Firestore_Example_macOS.debug.xcconfig"; path = "Pods/Target Support Files/Pods-Firestore_Example_macOS/Pods-Firestore_Example_macOS.debug.xcconfig"; sourceTree = ""; }; 99434327614FEFF7F7DC88EC /* counting_query_engine.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = counting_query_engine.cc; sourceTree = ""; }; 9B0B005A79E765AF02793DCE /* schedule_test.cc */ = {isa = PBXFileReference; includeInIndex = 1; lastKnownFileType = sourcecode.cpp.cpp; path = schedule_test.cc; sourceTree = ""; }; From 73a73c50d6bf7637617c383bd7b141a3a279c8fb Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 3 May 2023 11:30:44 -0400 Subject: [PATCH 26/26] async_testing.h: capture shared_from_this() more efficiently --- Firestore/core/test/unit/testutil/async_testing.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Firestore/core/test/unit/testutil/async_testing.h b/Firestore/core/test/unit/testutil/async_testing.h index 682f4749f62..cd56b16dd1b 100644 --- a/Firestore/core/test/unit/testutil/async_testing.h +++ b/Firestore/core/test/unit/testutil/async_testing.h @@ -232,8 +232,7 @@ class AsyncAccumulator final * `AccumulateObject()`. */ std::function AsCallback() { - auto shared_this = this->shared_from_this(); - return [shared_this](const T& object) { + return [shared_this = this->shared_from_this()](const T& object) { shared_this->AccumulateObject(object); }; }