From 66f36f048c0348f2423c40987443089a3cf0fe32 Mon Sep 17 00:00:00 2001 From: varconst Date: Fri, 25 Jun 2021 21:42:18 -0400 Subject: [PATCH 01/15] Initial commit --- .../firestore_integration_test_android.cc | 12 +--- .../src/android/promise_android_test.cc | 34 +++------ .../src/firestore_integration_test.cc | 23 ++----- .../src/firestore_integration_test.h | 69 +++++++++++-------- .../src/util/future_test_util.cc | 2 +- 5 files changed, 58 insertions(+), 82 deletions(-) diff --git a/firestore/integration_test_internal/src/android/firestore_integration_test_android.cc b/firestore/integration_test_internal/src/android/firestore_integration_test_android.cc index bae2f206ef..9ac52a2234 100644 --- a/firestore/integration_test_internal/src/android/firestore_integration_test_android.cc +++ b/firestore/integration_test_internal/src/android/firestore_integration_test_android.cc @@ -71,17 +71,9 @@ Local FirestoreAndroidIntegrationTest::CreateException( } void FirestoreAndroidIntegrationTest::Await(Env& env, const Task& task) { - int cycles = kTimeOutMillis / kCheckIntervalMillis; - while (env.ok() && !task.IsComplete(env)) { - if (ProcessEvents(kCheckIntervalMillis)) { - std::cout << "WARNING: app receives an event requesting exit." - << std::endl; - break; - } - --cycles; - } + bool success = WaitUntil([&] { return env.ok() && task.IsComplete(env); }); if (env.ok()) { - EXPECT_GT(cycles, 0) << "Waiting for Task timed out."; + EXPECT_TRUE(success) << "Waiting for Task timed out."; } } diff --git a/firestore/integration_test_internal/src/android/promise_android_test.cc b/firestore/integration_test_internal/src/android/promise_android_test.cc index 1e9ac52728..e976454616 100644 --- a/firestore/integration_test_internal/src/android/promise_android_test.cc +++ b/firestore/integration_test_internal/src/android/promise_android_test.cc @@ -119,24 +119,10 @@ class TestCompletionBase : public Promise 0) { - return true; - } - } - - if (ProcessEvents(kCheckIntervalMillis)) { - return false; - } - - cycles_remaining--; - if (cycles_remaining == 0) { - return false; - } - } + return WaitFor([&] { + MutexLock lock(mutex_); + return invocation_count_ > 0; + }); } // Returns the number of times that `CompleteWith()` has been invoked. @@ -226,7 +212,7 @@ TEST_F(PromiseTest, FutureVoidShouldSucceedWhenTaskSucceeds) { SetTaskResult(0); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), 0); EXPECT_EQ(future.result(), nullptr); @@ -240,7 +226,7 @@ TEST_F(PromiseTest, FutureNonVoidShouldSucceedWhenTaskSucceeds) { SetTaskResult(42); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), 0); EXPECT_EQ(*future.result(), "42"); @@ -253,7 +239,7 @@ TEST_F(PromiseTest, FutureVoidShouldFailWhenTaskFails) { SetTaskException(Error::kErrorFailedPrecondition, "Simulated failure"); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), Error::kErrorFailedPrecondition); EXPECT_EQ(future.error_message(), std::string("Simulated failure")); @@ -268,7 +254,7 @@ TEST_F(PromiseTest, FutureNonVoidShouldFailWhenTaskFails) { SetTaskException(Error::kErrorFailedPrecondition, "Simulated failure"); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), Error::kErrorFailedPrecondition); EXPECT_EQ(future.error_message(), std::string("Simulated failure")); @@ -282,7 +268,7 @@ TEST_F(PromiseTest, FutureVoidShouldCancelWhenTaskCancels) { CancelTask(); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), Error::kErrorCancelled); EXPECT_EQ(future.error_message(), std::string("cancelled")); @@ -297,7 +283,7 @@ TEST_F(PromiseTest, FutureNonVoidShouldCancelWhenTaskCancels) { CancelTask(); - EXPECT_GT(WaitFor(future), 0); + EXPECT_TRUE(WaitUntilFutureCompletes(future)); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); EXPECT_EQ(future.error(), Error::kErrorCancelled); EXPECT_EQ(future.error_message(), std::string("cancelled")); diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index ec56570def..9d7af3fe4c 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -91,18 +91,9 @@ std::string ToFirestoreErrorCodeName(int error_code) { } } -int WaitFor(const FutureBase& future) { - // Instead of getting a clock, we count the cycles instead. - int cycles = kTimeOutMillis / kCheckIntervalMillis; - while (future.status() == FutureStatus::kFutureStatusPending && cycles > 0) { - if (ProcessEvents(kCheckIntervalMillis)) { - std::cout << "WARNING: app receives an event requesting exit." - << std::endl; - break; - } - --cycles; - } - return cycles; +bool WaitUntilFutureCompletes(const FutureBase& future) { + return WaitUntil( + [&] { return future.status() != FutureStatus::kFutureStatusPending; }); } FirestoreIntegrationTest::FirestoreIntegrationTest() { @@ -276,13 +267,7 @@ FirestoreIntegrationTest::QuerySnapshotToMap( /* static */ void FirestoreIntegrationTest::Await(const Future& future) { - while (future.status() == FutureStatus::kFutureStatusPending) { - if (ProcessEvents(kCheckIntervalMillis)) { - std::cout << "WARNING: app received an event requesting exit." - << std::endl; - break; - } - } + EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; } /* static */ diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index b801f80202..00b2cdf246 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -1,6 +1,7 @@ #ifndef FIREBASE_FIRESTORE_CLIENT_CPP_SRC_TESTS_FIRESTORE_INTEGRATION_TEST_H_ #define FIREBASE_FIRESTORE_CLIENT_CPP_SRC_TESTS_FIRESTORE_INTEGRATION_TEST_H_ +#include // NOLINT(build/c++11) #include #include #include @@ -43,11 +44,15 @@ bool ProcessEvents(int msec); // enum, but this function will gracefully handle the case where it is not. std::string ToFirestoreErrorCodeName(int error_code); -// Waits for a Future to complete. If a timeout is reached then this method -// returns as if successful; therefore, the caller should verify the status of -// the given Future after this function returns. Returns the number of cycles -// that were left before a timeout would have occurred. -int WaitFor(const FutureBase& future); +// Blocks until either the given predicate `pred` returns `true` or timeout +// occurs. Returns `true` on success, `false` on timeout or if exit signal was +// received. +template +bool WaitUntil(const PredT& pred, int timeout_ms = kTimeOutMillis); + +// Blocks until either the future completes or timeout occurs. Returns `true` +// on success, `false` on timeout or if exit signal was received. +bool WaitUntilFutureCompletes(const FutureBase& future); template class EventAccumulator; @@ -280,37 +285,27 @@ class FirestoreIntegrationTest : public testing::Test { // TODO(zxu): add a helper function to block on signal. - // A helper function to block until the future completes. + // Blocks until the future completes and returns the future result. template static const T* Await(const Future& future) { - int cycles = WaitFor(future); - EXPECT_GT(cycles, 0) << "Waiting future timed out."; - if (future.status() == FutureStatus::kFutureStatusComplete) { - if (future.result() == nullptr) { - std::cout << "WARNING: " << DescribeFailedFuture(future) << std::endl; - } - } else { - std::cout << "WARNING: Future is not completed." << std::endl; - } + ASSERT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; + + ASSERT_EQ(future.status(), FutureStatus::kFutureStatusComplete) + << DescribeFailedFuture(); + ASSERT_NE(future.result(), nullptr) << DescribeFailedFuture(); + return future.result(); } + // Blocks until the future completes. static void Await(const Future& future); - // A helper function to block until there is at least n event. + // Blocks until there is at least `event_count` events. template - static void Await(const TestEventListener& listener, int n = 1) { - // Instead of getting a clock, we count the cycles instead. - int cycles = kTimeOutMillis / kCheckIntervalMillis; - while (listener.event_count() < n && cycles > 0) { - if (ProcessEvents(kCheckIntervalMillis)) { - std::cout << "WARNING: app receives an event requesting exit." - << std::endl; - return; - } - --cycles; - } - EXPECT_GT(cycles, 0) << "Waiting listener timed out."; + static void Await(const TestEventListener& listener, int event_count = 1) { + bool success = + WaitUntil([&] { return listener.event_count() >= event_count; }); + EXPECT_TRUE(success) << "Waiting listener timed out."; } // Fails the current test if the given future did not complete or contained an @@ -355,6 +350,24 @@ class FirestoreIntegrationTest : public testing::Test { mutable std::unordered_map firestores_; }; +template +bool WaitUntil(const PredT& pred, int timeout_ms) { + auto now = std::chrono::steady_clock::now(); + auto timeout_time = now + std::chrono::milliseconds(timeout_ms); + + while (!pred() && now < timeout_time) { + if (ProcessEvents(kCheckIntervalMillis)) { + std::cout << "WARNING: app received an event requesting exit." + << std::endl; + return false; + } + + now = std::chrono::steady_clock::now(); + } + + return now < timeout_time; +} + } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/future_test_util.cc b/firestore/integration_test_internal/src/util/future_test_util.cc index 23b7721843..a8c7ad6c9b 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.cc +++ b/firestore/integration_test_internal/src/util/future_test_util.cc @@ -31,7 +31,7 @@ class FutureSucceedsImpl bool MatchAndExplain(const Future& future, testing::MatchResultListener*) const override { - firestore::WaitFor(future); + firestore::WaitUntilFutureCompletes(future); return future.status() == FutureStatus::kFutureStatusComplete && future.error() == firestore::Error::kErrorOk; } From 560d56dd1312bdccc573ef1343234edb252c4b1a Mon Sep 17 00:00:00 2001 From: varconst Date: Fri, 25 Jun 2021 21:55:19 -0400 Subject: [PATCH 02/15] style --- .../integration_test_internal/src/firestore_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index 9d7af3fe4c..2089a1a0b5 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -93,7 +93,7 @@ std::string ToFirestoreErrorCodeName(int error_code) { bool WaitUntilFutureCompletes(const FutureBase& future) { return WaitUntil( - [&] { return future.status() != FutureStatus::kFutureStatusPending; }); + [&] { return future.status() != FutureStatus::kFutureStatusPending; }); } FirestoreIntegrationTest::FirestoreIntegrationTest() { From 79985435bd8cc5f259b253b773c6d59f48d3cba7 Mon Sep 17 00:00:00 2001 From: varconst Date: Mon, 28 Jun 2021 20:12:23 -0400 Subject: [PATCH 03/15] Fixes --- .../src/firestore_integration_test.cc | 3 --- .../src/firestore_integration_test.h | 8 ++++---- .../src/util/future_test_util.cc | 14 ++++++++++---- .../src/util/future_test_util.h | 6 ++++++ 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index 2089a1a0b5..c88f163fe4 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -265,12 +265,10 @@ FirestoreIntegrationTest::QuerySnapshotToMap( return result; } -/* static */ void FirestoreIntegrationTest::Await(const Future& future) { EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; } -/* static */ bool FirestoreIntegrationTest::FailIfUnsuccessful(const char* operation, const FutureBase& future) { if (future.status() != FutureStatus::kFutureStatusComplete) { @@ -286,7 +284,6 @@ bool FirestoreIntegrationTest::FailIfUnsuccessful(const char* operation, } } -/* static */ std::string FirestoreIntegrationTest::DescribeFailedFuture( const FutureBase& future) { return "Future failed: " + ToFirestoreErrorCodeName(future.error()) + " (" + diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 00b2cdf246..a05eacb72d 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -288,11 +288,11 @@ class FirestoreIntegrationTest : public testing::Test { // Blocks until the future completes and returns the future result. template static const T* Await(const Future& future) { - ASSERT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; + EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; - ASSERT_EQ(future.status(), FutureStatus::kFutureStatusComplete) - << DescribeFailedFuture(); - ASSERT_NE(future.result(), nullptr) << DescribeFailedFuture(); + EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete) + << DescribeFailedFuture(future); + EXPECT_NE(future.result(), nullptr) << DescribeFailedFuture(future); return future.result(); } diff --git a/firestore/integration_test_internal/src/util/future_test_util.cc b/firestore/integration_test_internal/src/util/future_test_util.cc index a8c7ad6c9b..da229b7c49 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.cc +++ b/firestore/integration_test_internal/src/util/future_test_util.cc @@ -13,7 +13,7 @@ void PrintTo(std::ostream* os, FutureStatus future_status, int error, const char* error_message = nullptr) { - *os << "Future{status=" << ToEnumeratorName(future_status) + *os << "Future{status=" << ToEnumeratorName(future_status) << " error=" << firestore::ToFirestoreErrorCodeName(error); if (error_message != nullptr) { *os << " error_message=" << error_message; @@ -21,8 +21,9 @@ void PrintTo(std::ostream* os, *os << "}"; } +template class FutureSucceedsImpl - : public testing::MatcherInterface&> { + : public testing::MatcherInterface&> { public: void DescribeTo(std::ostream* os) const override { PrintTo(os, FutureStatus::kFutureStatusComplete, @@ -52,12 +53,17 @@ std::string ToEnumeratorName(FutureStatus status) { } } -void PrintTo(const Future& future, std::ostream* os) { +void PrintTo(const FutureBase& future, std::ostream* os) { PrintTo(os, future.status(), future.error(), future.error_message()); } +template +testing::Matcher&> FutureSucceeds() { + return testing::Matcher&>(new FutureSucceedsImpl()); +} + testing::Matcher&> FutureSucceeds() { - return testing::Matcher&>(new FutureSucceedsImpl()); + return testing::Matcher&>(new FutureSucceedsImpl()); } } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index 14108349c4..4b8a2626cd 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -20,6 +20,12 @@ void PrintTo(const Future& future, std::ostream* os); // will succeed. // // Here is an example of how this function could be used: +// EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); +template +testing::Matcher&> FutureSucceeds(); + +// Similar to the templated function but works for `Future`. Note there's +// no need to specify template parameters: // EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); testing::Matcher&> FutureSucceeds(); From c86d850708230c83532f988934098593463872bb Mon Sep 17 00:00:00 2001 From: varconst Date: Mon, 28 Jun 2021 20:34:26 -0400 Subject: [PATCH 04/15] style --- .../integration_test_internal/src/util/future_test_util.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.cc b/firestore/integration_test_internal/src/util/future_test_util.cc index da229b7c49..1f0f8eb156 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.cc +++ b/firestore/integration_test_internal/src/util/future_test_util.cc @@ -22,8 +22,7 @@ void PrintTo(std::ostream* os, } template -class FutureSucceedsImpl - : public testing::MatcherInterface&> { +class FutureSucceedsImpl : public testing::MatcherInterface&> { public: void DescribeTo(std::ostream* os) const override { PrintTo(os, FutureStatus::kFutureStatusComplete, From 36296360f44cc2e77e7bed1da314defdfb271ea3 Mon Sep 17 00:00:00 2001 From: varconst Date: Mon, 28 Jun 2021 20:39:28 -0400 Subject: [PATCH 05/15] Simplify --- .../src/util/future_test_util.cc | 14 ++++---------- .../src/util/future_test_util.h | 8 +------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.cc b/firestore/integration_test_internal/src/util/future_test_util.cc index 1f0f8eb156..8cabb604fc 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.cc +++ b/firestore/integration_test_internal/src/util/future_test_util.cc @@ -21,15 +21,14 @@ void PrintTo(std::ostream* os, *os << "}"; } -template -class FutureSucceedsImpl : public testing::MatcherInterface&> { +class FutureSucceedsImpl : public testing::MatcherInterface { public: void DescribeTo(std::ostream* os) const override { PrintTo(os, FutureStatus::kFutureStatusComplete, firestore::Error::kErrorOk); } - bool MatchAndExplain(const Future& future, + bool MatchAndExplain(const FutureBase& future, testing::MatchResultListener*) const override { firestore::WaitUntilFutureCompletes(future); return future.status() == FutureStatus::kFutureStatusComplete && @@ -56,13 +55,8 @@ void PrintTo(const FutureBase& future, std::ostream* os) { PrintTo(os, future.status(), future.error(), future.error_message()); } -template -testing::Matcher&> FutureSucceeds() { - return testing::Matcher&>(new FutureSucceedsImpl()); -} - -testing::Matcher&> FutureSucceeds() { - return testing::Matcher&>(new FutureSucceedsImpl()); +testing::Matcher FutureSucceeds() { + return testing::Matcher(new FutureSucceedsImpl()); } } // namespace firebase diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index 4b8a2626cd..3eccb008f6 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -21,13 +21,7 @@ void PrintTo(const Future& future, std::ostream* os); // // Here is an example of how this function could be used: // EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); -template -testing::Matcher&> FutureSucceeds(); - -// Similar to the templated function but works for `Future`. Note there's -// no need to specify template parameters: -// EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); -testing::Matcher&> FutureSucceeds(); +testing::Matcher FutureSucceeds(); // Converts a `FutureStatus` value to its enumerator name, and returns it. For // example, if `kFutureStatusComplete` is specified then "kFutureStatusComplete" From 4644cb643ba296c2fb5c0fe0e69bb8bce7de5d58 Mon Sep 17 00:00:00 2001 From: varconst Date: Mon, 28 Jun 2021 20:40:29 -0400 Subject: [PATCH 06/15] Fix comment --- firestore/integration_test_internal/src/util/future_test_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index 3eccb008f6..fe98f3cd2f 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -20,7 +20,7 @@ void PrintTo(const Future& future, std::ostream* os); // will succeed. // // Here is an example of how this function could be used: -// EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); +// EXPECT_THAT(TestFirestore()->Terminate(), FutureSucceeds()); testing::Matcher FutureSucceeds(); // Converts a `FutureStatus` value to its enumerator name, and returns it. For From 575904478ed106085c68fa1ce027797e1a120202 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Wed, 30 Jun 2021 19:11:46 -0400 Subject: [PATCH 07/15] Fix linkage --- firestore/integration_test_internal/src/util/future_test_util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index fe98f3cd2f..c2a8855c1b 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -12,7 +12,7 @@ namespace firebase { // Prints a human-friendly representation of a Future to an ostream. // This function is found dynamically by googletest to print a Future object // in a test failure message. -void PrintTo(const Future& future, std::ostream* os); +void PrintTo(const FutureBase& future, std::ostream* os); // Creates and returns a "matcher" for a Future's success. The matcher will wait // for the Future to complete with a timeout. If the timeout is reached or the From da3aba9aa4dbc09f2d731bc99b3938acd9bccf91 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Wed, 30 Jun 2021 20:20:59 -0400 Subject: [PATCH 08/15] Fix --- firestore/integration_test_internal/src/bundle_test.cc | 3 ++- .../src/firestore_integration_test.cc | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/firestore/integration_test_internal/src/bundle_test.cc b/firestore/integration_test_internal/src/bundle_test.cc index a3935a1b76..f3992d5b07 100644 --- a/firestore/integration_test_internal/src/bundle_test.cc +++ b/firestore/integration_test_internal/src/bundle_test.cc @@ -172,7 +172,8 @@ TEST_F(BundleTest, CanDeleteFirestoreFromProgressUpdate) { // This future is not completed, and returns back a nullptr for result when it // times out. - EXPECT_EQ(Await(result), nullptr); + EXPECT_TRUE(WaitUntilFutureCompletes(result)); + EXPECT_EQ(result.result(), nullptr); // 3 progresses will be reported: initial, document 1, document 2. // Final progress update is missing because Firestore is deleted before that. diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index c88f163fe4..a6bbb74fff 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -286,8 +286,11 @@ bool FirestoreIntegrationTest::FailIfUnsuccessful(const char* operation, std::string FirestoreIntegrationTest::DescribeFailedFuture( const FutureBase& future) { - return "Future failed: " + ToFirestoreErrorCodeName(future.error()) + " (" + - std::to_string(future.error()) + "): " + future.error_message(); + std::string error_message = + future.error_message() ? future.error_message() : "[no additional info]"; + return std::string("Future failed: ") + + ToFirestoreErrorCodeName(future.error()) + " (" + + std::to_string(future.error()) + "): " + error_message; } bool ProcessEvents(int msec) { return app_framework::ProcessEvents(msec); } From 477e012b78fc066823ae8252244d2c8649c92e78 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Mon, 12 Jul 2021 22:55:51 -0400 Subject: [PATCH 09/15] Feedback --- .../src/firestore_integration_test.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index a05eacb72d..dc5f7db9e8 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -285,7 +285,7 @@ class FirestoreIntegrationTest : public testing::Test { // TODO(zxu): add a helper function to block on signal. - // Blocks until the future completes and returns the future result. + // Blocks until the future is completed and returns the future result. template static const T* Await(const Future& future) { EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; @@ -305,7 +305,7 @@ class FirestoreIntegrationTest : public testing::Test { static void Await(const TestEventListener& listener, int event_count = 1) { bool success = WaitUntil([&] { return listener.event_count() >= event_count; }); - EXPECT_TRUE(success) << "Waiting listener timed out."; + EXPECT_TRUE(success) << "Waiting for a listener timed out."; } // Fails the current test if the given future did not complete or contained an From 9759b3bf5f2c541f6615cf64e491f80c5f04a302 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Thu, 15 Jul 2021 15:16:16 -0400 Subject: [PATCH 10/15] Fix typo --- .../src/android/promise_android_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firestore/integration_test_internal/src/android/promise_android_test.cc b/firestore/integration_test_internal/src/android/promise_android_test.cc index e976454616..d7edf69081 100644 --- a/firestore/integration_test_internal/src/android/promise_android_test.cc +++ b/firestore/integration_test_internal/src/android/promise_android_test.cc @@ -1,4 +1,4 @@ -#include "firestore/src/android/promise_android.h" +#jenclude "firestore/src/android/promise_android.h" #include @@ -119,7 +119,7 @@ class TestCompletionBase : public Promise 0; }); From 0e98a9c473b1eedb3ca42c1b895c891ac210d1c4 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Wed, 21 Jul 2021 21:27:19 -0400 Subject: [PATCH 11/15] Fix PrintTo --- .../src/util/future_test_util.h | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index e15823ace7..af0721a178 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -12,9 +12,11 @@ namespace firebase { // Prints a human-friendly representation of a Future to an ostream. -// This function is found dynamically by googletest to print a Future object -// in a test failure message. -void PrintTo(const FutureBase& future, std::ostream* os); +// This function is via ADL by Googletest to print a Future object in a test +// failure message. Note that the future type must match exactly, without any +// implicit conversions, for this mechanism to work. +template +void PrintTo(const Future& future, std::ostream* os); // Creates and returns a "matcher" for a Future's success. The matcher will wait // for the Future to complete with a timeout. If the timeout is reached or the @@ -32,6 +34,18 @@ testing::Matcher FutureSucceeds(); // indicate this. std::string ToEnumeratorName(FutureStatus status); +// Implementation details follow. + +// Because this function's signature requires an implicit conversion to the base +// class, it cannot be found by Googletest -- instead, the template functions +// delegate to it. +void PrintTo(const FutureBase& future, std::ostream* os); + +template +void PrintTo(const Future& future, std::ostream* os) { + PrintTo(static_cast(future), os); +} + } // namespace firebase #endif // FIREBASE_FIRESTORE_INTEGRATION_TEST_INTERNAL_SRC_UTIL_FUTURE_TEST_UTIL_H_ From a26cc964952464efb81cab2534fa0e58cfa3acbe Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Thu, 22 Jul 2021 19:57:59 -0400 Subject: [PATCH 12/15] Fix some transaction tests timing out --- external/vcpkg | 2 +- .../src/firestore_integration_test.cc | 11 +++++---- .../src/firestore_integration_test.h | 12 ++++++---- .../src/transaction_extra_test.cc | 6 ++++- .../src/transaction_test.cc | 23 +++++++++++++------ 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/external/vcpkg b/external/vcpkg index c69096659f..1d8728ae1b 160000 --- a/external/vcpkg +++ b/external/vcpkg @@ -1 +1 @@ -Subproject commit c69096659f49e2b1aca532ea5c2f8c135182519b +Subproject commit 1d8728ae1ba66ad94b344708cf8d0ace1a6330b8 diff --git a/firestore/integration_test_internal/src/firestore_integration_test.cc b/firestore/integration_test_internal/src/firestore_integration_test.cc index b76c8e8ead..ee29176662 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.cc +++ b/firestore/integration_test_internal/src/firestore_integration_test.cc @@ -97,9 +97,10 @@ std::string ToFirestoreErrorCodeName(int error_code) { } } -bool WaitUntilFutureCompletes(const FutureBase& future) { +bool WaitUntilFutureCompletes(const FutureBase& future, int timeout_ms) { return WaitUntil( - [&] { return future.status() != FutureStatus::kFutureStatusPending; }); + [&] { return future.status() != FutureStatus::kFutureStatusPending; }, + timeout_ms); } FirestoreIntegrationTest::FirestoreIntegrationTest() { @@ -271,8 +272,10 @@ FirestoreIntegrationTest::QuerySnapshotToMap( return result; } -void FirestoreIntegrationTest::Await(const Future& future) { - EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; +void FirestoreIntegrationTest::Await(const Future& future, + int timeout_ms) { + EXPECT_TRUE(WaitUntilFutureCompletes(future, timeout_ms)) + << "Future timed out."; } bool FirestoreIntegrationTest::FailIfUnsuccessful(const char* operation, diff --git a/firestore/integration_test_internal/src/firestore_integration_test.h b/firestore/integration_test_internal/src/firestore_integration_test.h index 19f2019292..bf45b4573e 100644 --- a/firestore/integration_test_internal/src/firestore_integration_test.h +++ b/firestore/integration_test_internal/src/firestore_integration_test.h @@ -54,7 +54,8 @@ bool WaitUntil(const PredT& pred, int timeout_ms = kTimeOutMillis); // Blocks until either the future completes or timeout occurs. Returns `true` // on success, `false` on timeout or if exit signal was received. -bool WaitUntilFutureCompletes(const FutureBase& future); +bool WaitUntilFutureCompletes(const FutureBase& future, + int timeout_ms = kTimeOutMillis); template class EventAccumulator; @@ -283,8 +284,10 @@ class FirestoreIntegrationTest : public testing::Test { // Blocks until the future is completed and returns the future result. template - static const T* Await(const Future& future) { - EXPECT_TRUE(WaitUntilFutureCompletes(future)) << "Future timed out."; + static const T* Await(const Future& future, + int timeout_ms = kTimeOutMillis) { + EXPECT_TRUE(WaitUntilFutureCompletes(future, timeout_ms)) + << "Future timed out."; EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete) << DescribeFailedFuture(future); @@ -294,7 +297,8 @@ class FirestoreIntegrationTest : public testing::Test { } // Blocks until the future completes. - static void Await(const Future& future); + static void Await(const Future& future, + int timeout_ms = kTimeOutMillis); // Blocks until there is at least `event_count` events. template diff --git a/firestore/integration_test_internal/src/transaction_extra_test.cc b/firestore/integration_test_internal/src/transaction_extra_test.cc index bd74b8f51f..f9fbd6a485 100644 --- a/firestore/integration_test_internal/src/transaction_extra_test.cc +++ b/firestore/integration_test_internal/src/transaction_extra_test.cc @@ -89,7 +89,11 @@ TEST_F(TransactionExtraTest, TestReadingADocTwiceWithDifferentVersions) { transaction.Set(doc, MapFieldValue{{"count", FieldValue::Double(16.0)}}); return error; }); - Await(future); + + // Because the transaction retries a few times with exponential backoff, it + // might time out with the default timeout time (the default timeout value is + // 15 seconds and the transaction can take 11-16 seconds). + Await(future, kTimeOutMillis * 2); EXPECT_EQ(Error::kErrorAborted, future.error()); EXPECT_STREQ("Document version changed between two reads.", future.error_message()); diff --git a/firestore/integration_test_internal/src/transaction_test.cc b/firestore/integration_test_internal/src/transaction_test.cc index 6c2b76b42f..511b39f18f 100644 --- a/firestore/integration_test_internal/src/transaction_test.cc +++ b/firestore/integration_test_internal/src/transaction_test.cc @@ -39,12 +39,13 @@ class TransactionTest : public FirestoreIntegrationTest { void RunTransactionAndExpect( Error error, const char* message, - std::function update) { + std::function update, + int timeout_ms = kTimeOutMillis) { Future future; // Re-try 5 times in case server is unavailable. for (int i = 0; i < 5; ++i) { future = TestFirestore()->RunTransaction(update); - Await(future); + Await(future, timeout_ms); if (future.error() == Error::kErrorUnavailable) { std::cout << "Could not reach backend. Retrying transaction test." << std::endl; @@ -57,10 +58,13 @@ class TransactionTest : public FirestoreIntegrationTest { } void RunTransactionAndExpect( - Error error, std::function update) { + Error error, + std::function update, + int timeout_ms = kTimeOutMillis) { switch (error) { case Error::kErrorOk: - RunTransactionAndExpect(Error::kErrorOk, "", std::move(update)); + RunTransactionAndExpect(Error::kErrorOk, "", std::move(update), + timeout_ms); break; case Error::kErrorAborted: RunTransactionAndExpect( @@ -69,7 +73,7 @@ class TransactionTest : public FirestoreIntegrationTest { #else Error::kErrorAborted, #endif - "Transaction failed all retries.", std::move(update)); + "Transaction failed all retries.", std::move(update), timeout_ms); break; case Error::kErrorFailedPrecondition: // Here specifies error message of the most common cause. There are @@ -77,7 +81,7 @@ class TransactionTest : public FirestoreIntegrationTest { // parameter if the expected error message is different. RunTransactionAndExpect(Error::kErrorFailedPrecondition, "Can't update a document that doesn't exist.", - std::move(update)); + std::move(update), timeout_ms); break; default: FAIL() << "Unexpected error code: " << error; @@ -694,6 +698,10 @@ TEST_F(TransactionTest, TestCancellationOnError) { int count = 0; SCOPED_TRACE("TestCancellationOnError"); + + // Because the transaction retries a few times with exponential backoff, it + // might time out with the default timeout time (the default timeout value is + // 15 seconds and the transaction can take up to 20 seconds). RunTransactionAndExpect( Error::kErrorDeadlineExceeded, "no", [doc, &count_locker, &count](Transaction& transaction, @@ -705,7 +713,8 @@ TEST_F(TransactionTest, TestCancellationOnError) { transaction.Set(doc, MapFieldValue{{"foo", FieldValue::String("bar")}}); error_message = "no"; return Error::kErrorDeadlineExceeded; - }); + }, + kTimeOutMillis * 3); // TODO(varconst): uncomment. Currently, there is no way in C++ to distinguish // user error, so the transaction gets retried, and the counter goes up to 6. From 49add61c3112a56d55d6ebc992a131d8dd0bf858 Mon Sep 17 00:00:00 2001 From: Vimanyu Date: Thu, 22 Jul 2021 18:39:30 -0700 Subject: [PATCH 13/15] updating vcpkg submodule --- external/vcpkg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/vcpkg b/external/vcpkg index 1d8728ae1b..5293c8685a 160000 --- a/external/vcpkg +++ b/external/vcpkg @@ -1 +1 @@ -Subproject commit 1d8728ae1ba66ad94b344708cf8d0ace1a6330b8 +Subproject commit 5293c8685a9d1b880a334b40a40a73ee667a294f From 59c84922db74fcbd6f7e6267e37cf4634361c4de Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Thu, 22 Jul 2021 22:06:34 -0400 Subject: [PATCH 14/15] Updated submodule --- external/vcpkg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/external/vcpkg b/external/vcpkg index 1d8728ae1b..5293c8685a 160000 --- a/external/vcpkg +++ b/external/vcpkg @@ -1 +1 @@ -Subproject commit 1d8728ae1ba66ad94b344708cf8d0ace1a6330b8 +Subproject commit 5293c8685a9d1b880a334b40a40a73ee667a294f From 4684e79accc5180f04349c82be47becf19f1a085 Mon Sep 17 00:00:00 2001 From: Konstantin Varlamov Date: Fri, 23 Jul 2021 14:26:11 -0400 Subject: [PATCH 15/15] Review --- .../integration_test_internal/src/util/future_test_util.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/firestore/integration_test_internal/src/util/future_test_util.h b/firestore/integration_test_internal/src/util/future_test_util.h index af0721a178..6de35006ab 100644 --- a/firestore/integration_test_internal/src/util/future_test_util.h +++ b/firestore/integration_test_internal/src/util/future_test_util.h @@ -12,9 +12,9 @@ namespace firebase { // Prints a human-friendly representation of a Future to an ostream. -// This function is via ADL by Googletest to print a Future object in a test -// failure message. Note that the future type must match exactly, without any -// implicit conversions, for this mechanism to work. +// This function is found via ADL by Googletest to print a Future object in +// a test failure message. Note that the future type must match exactly, without +// any implicit conversions, for this mechanism to work. template void PrintTo(const Future& future, std::ostream* os);