From 83efa263297d6a855a4c044226bbe0d84ff07e9f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Jun 2023 13:26:54 -0400 Subject: [PATCH 1/4] Replace explicit Env creation with the env() helper member function --- .../firestore_integration_test_android.cc | 48 ++-- .../firestore_integration_test_android.h | 18 +- ...firestore_integration_test_android_test.cc | 77 ++---- .../src/android/geo_point_android_test.cc | 13 +- .../src/android/jni_runnable_android_test.cc | 258 ++++++++---------- .../src/android/promise_android_test.cc | 65 ++--- .../android/promise_factory_android_test.cc | 43 ++- .../src/android/settings_android_test.cc | 15 +- .../android/snapshot_metadata_android_test.cc | 16 +- .../src/android/timestamp_android_test.cc | 13 +- .../transaction_options_android_test.cc | 27 +- .../src/firestore_test.cc | 9 +- .../src/jni/task_test.cc | 120 ++++---- 13 files changed, 305 insertions(+), 417 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 8e3453e8ca..c1b80106e9 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 @@ -42,7 +42,6 @@ void PrintTo(const Object& object, std::ostream* os) { } // namespace jni using jni::Constructor; -using jni::Env; using jni::ExceptionClearGuard; using jni::Local; using jni::String; @@ -77,62 +76,61 @@ void FirestoreAndroidIntegrationTest::TearDown() { } void FirestoreAndroidIntegrationTest::FailTestIfPendingException() { - Env env; - Local pending_exception = env.ClearExceptionOccurred(); + Local pending_exception = env().ClearExceptionOccurred(); if (!pending_exception) { return; } // Ignore the exception if it was thrown by the last ThrowException() call. - if (env.IsSameObject(pending_exception, last_thrown_exception_)) { + if (env().IsSameObject(pending_exception, last_thrown_exception_)) { return; } // Fail the test since the test completed with a pending exception. - std::string pending_exception_as_string = pending_exception.ToString(env); - env.ExceptionClear(); + std::string pending_exception_as_string = pending_exception.ToString(env()); + env().ExceptionClear(); FAIL() << "Test completed with a pending Java exception: " << pending_exception_as_string; } -Local FirestoreAndroidIntegrationTest::CreateException(Env& env) { - return CreateException(env, - "Test exception created by " - "FirestoreAndroidIntegrationTest::CreateException()"); +Local FirestoreAndroidIntegrationTest::CreateException() { + return CreateException( + "Test exception created by " + "FirestoreAndroidIntegrationTest::CreateException()"); } Local FirestoreAndroidIntegrationTest::CreateException( - Env& env, const std::string& message) { - ExceptionClearGuard exception_clear_guard(env); - Local java_message = env.NewStringUtf(message); - return env.New(kExceptionConstructor, java_message); + const std::string& message) { + ExceptionClearGuard exception_clear_guard(env()); + Local java_message = env().NewStringUtf(message); + return env().New(kExceptionConstructor, java_message); } -Local FirestoreAndroidIntegrationTest::ThrowException(Env& env) { - return ThrowException(env, - "Test exception thrown by " - "FirestoreAndroidIntegrationTest::ThrowException()"); +Local FirestoreAndroidIntegrationTest::ThrowException() { + return ThrowException( + "Test exception thrown by " + "FirestoreAndroidIntegrationTest::ThrowException()"); } Local FirestoreAndroidIntegrationTest::ThrowException( - Env& env, const std::string& message) { - if (!env.ok()) { + const std::string& message) { + if (!env().ok()) { ADD_FAILURE() << "ThrowException() invoked while there is already a " "pending exception"; return {}; } - Local exception = CreateException(env, message); + Local exception = CreateException(message); // Silently discard this exception if the test ends with it still pending. last_thrown_exception_ = exception; - env.Throw(exception); + env().Throw(exception); return exception; } -void FirestoreAndroidIntegrationTest::Await(Env& env, const Task& task) { +void FirestoreAndroidIntegrationTest::Await(const Task& task) { int cycles = kTimeOutMillis / kCheckIntervalMillis; - while (env.ok() && !task.IsComplete(env)) { + while (env().ok() && !task.IsComplete(env())) { if (ProcessEvents(kCheckIntervalMillis)) { std::cout << "WARNING: app receives an event requesting exit." << std::endl; @@ -140,7 +138,7 @@ void FirestoreAndroidIntegrationTest::Await(Env& env, const Task& task) { } --cycles; } - if (env.ok()) { + if (env().ok()) { EXPECT_GT(cycles, 0) << "Waiting for Task timed out."; } } diff --git a/firestore/integration_test_internal/src/android/firestore_integration_test_android.h b/firestore/integration_test_internal/src/android/firestore_integration_test_android.h index 3021cc089b..3d11b0787b 100644 --- a/firestore/integration_test_internal/src/android/firestore_integration_test_android.h +++ b/firestore/integration_test_internal/src/android/firestore_integration_test_android.h @@ -106,23 +106,27 @@ class FirestoreAndroidIntegrationTest : public FirestoreIntegrationTest { jni::Loader& loader() { return loader_; } /** Creates and returns a new Java `Exception` with a default message. */ - static jni::Local CreateException(jni::Env&); + static jni::Local CreateException(); /** Creates and returns a new Java `Exception` with the given message. */ - static jni::Local CreateException(jni::Env&, - const std::string& message); + static jni::Local CreateException(const std::string& message); /** Throws a Java `Exception` object with a default message. */ - jni::Local ThrowException(jni::Env&); + jni::Local ThrowException(); /** Throws a Java `Exception` object with the given message. */ - jni::Local ThrowException(jni::Env&, - const std::string& message); + jni::Local ThrowException(const std::string& message); // Bring definitions of `Await()` from the superclass into this class so that // the definition below *overloads* instead of *hides* them. using FirestoreIntegrationTest::Await; /** Blocks until the given `Task` has completed or times out. */ - static void Await(jni::Env& env, const jni::Task& task); + static void Await(const jni::Task& task); + + /** Returns an Env object for the calling thread, creating it if necessary. */ + static jni::Env& env() { + thread_local jni::Env env; + return env; + } private: void FailTestIfPendingException(); diff --git a/firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc b/firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc index 97ecbe1e11..496cb78e86 100644 --- a/firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc +++ b/firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc @@ -30,7 +30,6 @@ namespace firestore { namespace { using jni::ArrayList; -using jni::Env; using jni::Global; using jni::Local; using jni::Object; @@ -42,9 +41,7 @@ using ::testing::Not; using ::testing::StrEq; TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithNonNull) { - Env env; - - std::string debug_string = ToDebugString(env.NewStringUtf("Test Value")); + std::string debug_string = ToDebugString(env().NewStringUtf("Test Value")); EXPECT_EQ(debug_string, "Test Value"); } @@ -59,50 +56,45 @@ TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithNull) { TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithPendingExceptionAndNonNullObject) { - Env env; - Local object = env.NewStringUtf("Test Value"); - ThrowException(env); - ASSERT_FALSE(env.ok()); + Local object = env().NewStringUtf("Test Value"); + ThrowException(); + ASSERT_FALSE(env().ok()); std::string debug_string = ToDebugString(object); EXPECT_EQ(debug_string, "Test Value"); - env.ExceptionClear(); + env().ExceptionClear(); } TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithPendingExceptionAndNullObject) { - Env env; Object null_reference; - ThrowException(env); - ASSERT_FALSE(env.ok()); + ThrowException(); + ASSERT_FALSE(env().ok()); std::string debug_string = ToDebugString(null_reference); EXPECT_EQ(debug_string, "null"); - env.ExceptionClear(); + env().ExceptionClear(); } TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnTrueForEqualObjects) { - Env env; - Local object1 = env.NewStringUtf("string"); - Local object2 = env.NewStringUtf("string"); + Local object1 = env().NewStringUtf("string"); + Local object2 = env().NewStringUtf("string"); EXPECT_THAT(object1, JavaEq(object2)); } TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnFalseForUnequalObjects) { - Env env; - Local object1 = env.NewStringUtf("string1"); - Local object2 = env.NewStringUtf("string2"); + Local object1 = env().NewStringUtf("string1"); + Local object2 = env().NewStringUtf("string2"); EXPECT_THAT(object1, Not(JavaEq(object2))); } TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnTrueForTwoNullReferences) { - Env env; Local null_reference1; Local null_reference2; @@ -111,9 +103,8 @@ TEST_F(FirestoreAndroidIntegrationTest, TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnFalseIfExactlyOneObjectIsNull) { - Env env; Local null_reference; - Local non_null_reference = env.NewStringUtf("string2"); + Local non_null_reference = env().NewStringUtf("string2"); EXPECT_THAT(null_reference, Not(JavaEq(non_null_reference))); EXPECT_THAT(non_null_reference, Not(JavaEq(null_reference))); @@ -121,9 +112,8 @@ TEST_F(FirestoreAndroidIntegrationTest, TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnFalseForObjectOfDifferentTypes) { - Env env; - Local string_object = env.NewStringUtf("string2"); - Local list_object = ArrayList::Create(env); + Local string_object = env().NewStringUtf("string2"); + Local list_object = ArrayList::Create(env()); EXPECT_THAT(string_object, Not(JavaEq(list_object))); EXPECT_THAT(list_object, Not(JavaEq(string_object))); @@ -131,8 +121,7 @@ TEST_F(FirestoreAndroidIntegrationTest, TEST_F(FirestoreAndroidIntegrationTest, RefersToSameJavaObjectAsShouldReturnTrueForSameObjects) { - Env env; - Local object1 = env.NewStringUtf("string"); + Local object1 = env().NewStringUtf("string"); Global object2 = object1; EXPECT_THAT(object1, RefersToSameJavaObjectAs(object2)); @@ -148,19 +137,17 @@ TEST_F(FirestoreAndroidIntegrationTest, TEST_F(FirestoreAndroidIntegrationTest, RefersToSameJavaObjectAsShouldReturnFalseForDistinctObjects) { - Env env; - Local object1 = env.NewStringUtf("test string"); - Local object2 = env.NewStringUtf("test string"); - ASSERT_FALSE(env.IsSameObject(object1, object2)); + Local object1 = env().NewStringUtf("test string"); + Local object2 = env().NewStringUtf("test string"); + ASSERT_FALSE(env().IsSameObject(object1, object2)); EXPECT_THAT(object1, Not(RefersToSameJavaObjectAs(object2))); } TEST_F(FirestoreAndroidIntegrationTest, RefersToSameJavaObjectAsShouldReturnFalseIfExactlyOneObjectIsNull) { - Env env; Local null_reference; - Local non_null_reference = env.NewStringUtf("string2"); + Local non_null_reference = env().NewStringUtf("string2"); EXPECT_THAT(null_reference, Not(RefersToSameJavaObjectAs(non_null_reference))); @@ -170,42 +157,38 @@ TEST_F(FirestoreAndroidIntegrationTest, TEST_F(FirestoreAndroidIntegrationTest, ThrowExceptionWithNoMessageShouldSetPendingExceptionWithAMessage) { - Env env; - Local throw_exception_return_value = ThrowException(env); - Local actually_thrown_exception = env.ClearExceptionOccurred(); + Local throw_exception_return_value = ThrowException(); + Local actually_thrown_exception = env().ClearExceptionOccurred(); ASSERT_TRUE(actually_thrown_exception); EXPECT_THAT(actually_thrown_exception, RefersToSameJavaObjectAs(throw_exception_return_value)); - EXPECT_THAT(actually_thrown_exception.GetMessage(env), Not(IsEmpty())); + EXPECT_THAT(actually_thrown_exception.GetMessage(env()), Not(IsEmpty())); } TEST_F(FirestoreAndroidIntegrationTest, ThrowExceptionWithAMessageShouldSetPendingExceptionWithTheGivenMessage) { - Env env; Local throw_exception_return_value = - ThrowException(env, "my test message"); - Local actually_thrown_exception = env.ClearExceptionOccurred(); + ThrowException("my test message"); + Local actually_thrown_exception = env().ClearExceptionOccurred(); ASSERT_TRUE(actually_thrown_exception); EXPECT_THAT(actually_thrown_exception, RefersToSameJavaObjectAs(throw_exception_return_value)); - EXPECT_THAT(actually_thrown_exception.GetMessage(env), + EXPECT_THAT(actually_thrown_exception.GetMessage(env()), StrEq("my test message")); } TEST_F(FirestoreAndroidIntegrationTest, CreateExceptionWithNoMessageShouldReturnAnExceptionWithAMessage) { - Env env; - Local exception = CreateException(env); + Local exception = CreateException(); ASSERT_TRUE(exception); - EXPECT_THAT(exception.GetMessage(env), Not(IsEmpty())); + EXPECT_THAT(exception.GetMessage(env()), Not(IsEmpty())); } TEST_F(FirestoreAndroidIntegrationTest, CreateExceptionWithAMessageShouldReturnAnExceptionWithTheGivenMessage) { - Env env; - Local exception = CreateException(env, "my test message"); + Local exception = CreateException("my test message"); ASSERT_TRUE(exception); - EXPECT_THAT(exception.GetMessage(env), StrEq("my test message")); + EXPECT_THAT(exception.GetMessage(env()), StrEq("my test message")); } } // namespace diff --git a/firestore/integration_test_internal/src/android/geo_point_android_test.cc b/firestore/integration_test_internal/src/android/geo_point_android_test.cc index db5e56bbd4..ea1d9b274a 100644 --- a/firestore/integration_test_internal/src/android/geo_point_android_test.cc +++ b/firestore/integration_test_internal/src/android/geo_point_android_test.cc @@ -16,25 +16,20 @@ #include "firestore/src/android/geo_point_android.h" +#include "android/firestore_integration_test_android.h" #include "firebase/firestore/geo_point.h" -#include "firestore/src/jni/env.h" -#include "firestore_integration_test.h" #include "gtest/gtest.h" namespace firebase { namespace firestore { namespace { -using jni::Env; - -using GeoPointTest = FirestoreIntegrationTest; +using GeoPointTest = FirestoreAndroidIntegrationTest; TEST_F(GeoPointTest, Converts) { - Env env; - GeoPoint point{12.0, 34.0}; - auto java_point = GeoPointInternal::Create(env, point); - EXPECT_EQ(point, java_point.ToPublic(env)); + auto java_point = GeoPointInternal::Create(env(), point); + EXPECT_EQ(point, java_point.ToPublic(env())); } } // namespace diff --git a/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc b/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc index 55e6acc7fe..a61394c374 100644 --- a/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc +++ b/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc @@ -31,7 +31,6 @@ namespace firestore { namespace { -using jni::Env; using jni::Global; using jni::Local; using jni::Method; @@ -60,279 +59,254 @@ class JniRunnableTest : public FirestoreAndroidIntegrationTest { loader().LoadClass("java/lang/Thread$State", kThreadStateBlocked); ASSERT_TRUE(loader().ok()); } -}; -/** - * Returns the ID of the current Java thread. - */ -jlong GetCurrentThreadId(Env& env) { - Local thread = env.Call(kCurrentThread); - return env.Call(thread, kThreadGetId); -} + /** + * Returns the ID of the current Java thread. + */ + static jlong GetCurrentThreadId() { + Local thread = env().Call(kCurrentThread); + return env().Call(thread, kThreadGetId); + } -/** - * Returns the ID of the Java main thread. - */ -jlong GetMainThreadId(Env& env) { - Local main_looper = env.Call(kGetMainLooper); - Local main_thread = env.Call(main_looper, kLooperGetThread); - return env.Call(main_thread, kThreadGetId); -} + /** + * Returns the ID of the Java main thread. + */ + static jlong GetMainThreadId() { + Local main_looper = env().Call(kGetMainLooper); + Local main_thread = env().Call(main_looper, kLooperGetThread); + return env().Call(main_thread, kThreadGetId); + } -/** - * Returns whether or not the given thread is in the "blocked" state. - * See java.lang.Thread.State.BLOCKED. - */ -bool IsThreadBlocked(Env& env, Object& thread) { - Local actual_state = env.Call(thread, kThreadGetState); - Local expected_state = env.Get(kThreadStateBlocked); - return Object::Equals(env, expected_state, actual_state); -} + /** + * Returns whether or not the given thread is in the "blocked" state. + * See java.lang.Thread.State.BLOCKED. + */ + static bool IsThreadBlocked(Object& thread) { + Local actual_state = env().Call(thread, kThreadGetState); + Local expected_state = env().Get(kThreadStateBlocked); + return Object::Equals(env(), expected_state, actual_state); + } +}; TEST_F(JniRunnableTest, JavaRunCallsCppRun) { - Env env; bool invoked = false; - auto runnable = MakeJniRunnable(env, [&invoked] { invoked = true; }); + auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); Local java_runnable = runnable.GetJavaRunnable(); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); EXPECT_TRUE(invoked); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } TEST_F(JniRunnableTest, JavaRunCallsCppRunOncePerInvocation) { - Env env; int invoke_count = 0; - auto runnable = MakeJniRunnable(env, [&invoke_count] { invoke_count++; }); + auto runnable = MakeJniRunnable(env(), [&invoke_count] { invoke_count++; }); Local java_runnable = runnable.GetJavaRunnable(); - env.Call(java_runnable, kRunnableRun); - env.Call(java_runnable, kRunnableRun); - env.Call(java_runnable, kRunnableRun); - env.Call(java_runnable, kRunnableRun); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); EXPECT_EQ(invoke_count, 5); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } TEST_F(JniRunnableTest, JavaRunPropagatesExceptions) { - Env env; - Local exception = CreateException(env); - auto runnable = MakeJniRunnable(env, [exception] { - Env env; - env.Throw(exception); - }); + Local exception = CreateException(); + auto runnable = + MakeJniRunnable(env(), [exception] { env().Throw(exception); }); Local java_runnable = runnable.GetJavaRunnable(); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); - Local thrown_exception = env.ClearExceptionOccurred(); + Local thrown_exception = env().ClearExceptionOccurred(); EXPECT_TRUE(thrown_exception); - EXPECT_TRUE(env.IsSameObject(exception, thrown_exception)); + EXPECT_TRUE(env().IsSameObject(exception, thrown_exception)); } TEST_F(JniRunnableTest, DetachCausesJavaRunToDoNothing) { - Env env; bool invoked = false; - auto runnable = MakeJniRunnable(env, [&invoked] { invoked = true; }); + auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); Local java_runnable = runnable.GetJavaRunnable(); - runnable.Detach(env); + runnable.Detach(env()); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); EXPECT_FALSE(invoked); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } TEST_F(JniRunnableTest, DetachCanBeInvokedMultipleTimes) { - Env env; bool invoked = false; - auto runnable = MakeJniRunnable(env, [&invoked] { invoked = true; }); + auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); Local java_runnable = runnable.GetJavaRunnable(); - runnable.Detach(env); - runnable.Detach(env); - runnable.Detach(env); + runnable.Detach(env()); + runnable.Detach(env()); + runnable.Detach(env()); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); EXPECT_FALSE(invoked); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } TEST_F(JniRunnableTest, DetachDetachesEvenIfAnExceptionIsPending) { - Env env; bool invoked = false; - auto runnable = MakeJniRunnable(env, [&invoked] { invoked = true; }); + auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); Local java_runnable = runnable.GetJavaRunnable(); - Local exception = CreateException(env); - env.Throw(exception); - EXPECT_FALSE(env.ok()); + Local exception = ThrowException(); + EXPECT_FALSE(env().ok()); - runnable.Detach(env); + runnable.Detach(env()); - env.ExceptionClear(); - env.Call(java_runnable, kRunnableRun); + env().ExceptionClear(); + env().Call(java_runnable, kRunnableRun); EXPECT_FALSE(invoked); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } // Verify that b/181129657 does not regress; that is, calling `Detach()` from // `Run()` should not deadlock. TEST_F(JniRunnableTest, DetachCanBeCalledFromRun) { - Env env; int run_count = 0; - auto runnable = MakeJniRunnable(env, [&run_count](JniRunnableBase& runnable) { - ++run_count; - Env env; - runnable.Detach(env); - }); + auto runnable = + MakeJniRunnable(env(), [&run_count](JniRunnableBase& runnable) { + ++run_count; + runnable.Detach(env()); + }); Local java_runnable = runnable.GetJavaRunnable(); // Call `run()` twice to verify that the call to `Detach()` successfully // detaches and the second `run()` invocation does not call C++ `Run()`. - env.Call(java_runnable, kRunnableRun); - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); EXPECT_EQ(run_count, 1); } TEST_F(JniRunnableTest, DestructionCausesJavaRunToDoNothing) { - Env env; bool invoked = false; Local java_runnable; { - auto runnable = MakeJniRunnable(env, [&invoked] { invoked = true; }); + auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); java_runnable = runnable.GetJavaRunnable(); } - env.Call(java_runnable, kRunnableRun); + env().Call(java_runnable, kRunnableRun); EXPECT_FALSE(invoked); - EXPECT_TRUE(env.ok()); + EXPECT_TRUE(env().ok()); } TEST_F(JniRunnableTest, RunOnMainThreadRunsOnTheMainThread) { - Env env; jlong captured_thread_id = 0; - auto runnable = MakeJniRunnable(env, [&captured_thread_id] { - Env env; - captured_thread_id = GetCurrentThreadId(env); + auto runnable = MakeJniRunnable(env(), [&captured_thread_id] { + captured_thread_id = GetCurrentThreadId(); }); - Local task = runnable.RunOnMainThread(env); + Local task = runnable.RunOnMainThread(env()); - Await(env, task); - EXPECT_EQ(captured_thread_id, GetMainThreadId(env)); + Await(task); + EXPECT_EQ(captured_thread_id, GetMainThreadId()); } TEST_F(JniRunnableTest, RunOnMainThreadTaskFailsIfRunThrowsException) { - Env env; - Global exception = CreateException(env); - auto runnable = MakeJniRunnable(env, [exception] { - Env env; - env.Throw(exception); - }); + Global exception = CreateException(); + auto runnable = + MakeJniRunnable(env(), [exception] { env().Throw(exception); }); - Local task = runnable.RunOnMainThread(env); + Local task = runnable.RunOnMainThread(env()); - Await(env, task); - Local thrown_exception = task.GetException(env); + Await(task); + Local thrown_exception = task.GetException(env()); EXPECT_TRUE(thrown_exception); - EXPECT_TRUE(env.IsSameObject(exception, thrown_exception)); + EXPECT_TRUE(env().IsSameObject(exception, thrown_exception)); } TEST_F(JniRunnableTest, RunOnMainThreadRunsSynchronouslyFromMainThread) { - Env env; bool is_recursive_call = false; auto runnable = - MakeJniRunnable(env, [&is_recursive_call](JniRunnableBase& runnable) { - Env env; - EXPECT_EQ(GetCurrentThreadId(env), GetMainThreadId(env)); + MakeJniRunnable(env(), [&is_recursive_call](JniRunnableBase& runnable) { + EXPECT_EQ(GetCurrentThreadId(), GetMainThreadId()); if (is_recursive_call) { return; } is_recursive_call = true; - Local task = runnable.RunOnMainThread(env); - EXPECT_TRUE(task.IsComplete(env)); - EXPECT_TRUE(task.IsSuccessful(env)); + Local task = runnable.RunOnMainThread(env()); + EXPECT_TRUE(task.IsComplete(env())); + EXPECT_TRUE(task.IsSuccessful(env())); is_recursive_call = false; }); - Local task = runnable.RunOnMainThread(env); + Local task = runnable.RunOnMainThread(env()); - Await(env, task); + Await(task); } TEST_F(JniRunnableTest, RunOnNewThreadRunsOnANonMainThread) { - Env env; jlong captured_thread_id = 0; - auto runnable = MakeJniRunnable(env, [&captured_thread_id] { - Env env; - captured_thread_id = GetCurrentThreadId(env); + auto runnable = MakeJniRunnable(env(), [&captured_thread_id] { + captured_thread_id = GetCurrentThreadId(); }); - Local task = runnable.RunOnNewThread(env); + Local task = runnable.RunOnNewThread(env()); - Await(env, task); + Await(task); EXPECT_NE(captured_thread_id, 0); - EXPECT_NE(captured_thread_id, GetMainThreadId(env)); - EXPECT_NE(captured_thread_id, GetCurrentThreadId(env)); + EXPECT_NE(captured_thread_id, GetMainThreadId()); + EXPECT_NE(captured_thread_id, GetCurrentThreadId()); } TEST_F(JniRunnableTest, RunOnNewThreadTaskFailsIfRunThrowsException) { - Env env; - Global exception = CreateException(env); - auto runnable = MakeJniRunnable(env, [exception] { - Env env; - env.Throw(exception); - }); + Global exception = CreateException(); + auto runnable = + MakeJniRunnable(env(), [exception] { env().Throw(exception); }); - Local task = runnable.RunOnNewThread(env); + Local task = runnable.RunOnNewThread(env()); - Await(env, task); - Local thrown_exception = task.GetException(env); + Await(task); + Local thrown_exception = task.GetException(env()); EXPECT_TRUE(thrown_exception); - EXPECT_TRUE(env.IsSameObject(exception, thrown_exception)); + EXPECT_TRUE(env().IsSameObject(exception, thrown_exception)); } TEST_F(JniRunnableTest, DetachReturnsAfterLastRunOnAnotherThreadCompletes) { - Env env; compat::Atomic runnable1_run_invoke_count; runnable1_run_invoke_count.store(0); Mutex detach_thread_mutex; Global detach_thread; auto runnable1 = MakeJniRunnable( - env, [&runnable1_run_invoke_count, &detach_thread, &detach_thread_mutex] { + env(), + [&runnable1_run_invoke_count, &detach_thread, &detach_thread_mutex] { runnable1_run_invoke_count.fetch_add(1); - Env env; // Wait for `detach()` to be called and start blocking; then, return to // allow `detach()` to unblock and do its job. - while (env.ok()) { + while (env().ok()) { MutexLock lock(detach_thread_mutex); - if (detach_thread && IsThreadBlocked(env, detach_thread)) { + if (detach_thread && IsThreadBlocked(detach_thread)) { break; } } - EXPECT_TRUE(env.ok()) << "IsThreadBlocked() failed with an exception"; + EXPECT_TRUE(env().ok()) << "IsThreadBlocked() failed with an exception"; }); - auto runnable2 = - MakeJniRunnable(env, [&runnable1, &detach_thread, &detach_thread_mutex] { - Env env; + auto runnable2 = MakeJniRunnable( + env(), [&runnable1, &detach_thread, &detach_thread_mutex] { { MutexLock lock(detach_thread_mutex); - detach_thread = env.Call(kCurrentThread); + detach_thread = env().Call(kCurrentThread); } - runnable1.Detach(env); - EXPECT_TRUE(env.ok()) << "Detach() failed with an exception"; + runnable1.Detach(env()); + EXPECT_TRUE(env().ok()) << "Detach() failed with an exception"; }); // Wait for the `runnable1.Run()` to start to ensure that the lock is held. - Local task1 = runnable1.RunOnNewThread(env); + Local task1 = runnable1.RunOnNewThread(env()); while (true) { if (runnable1_run_invoke_count.load() != 0) { break; @@ -340,14 +314,14 @@ TEST_F(JniRunnableTest, DetachReturnsAfterLastRunOnAnotherThreadCompletes) { } // Start a new thread to call `runnable1.Detach()`. - Local task2 = runnable2.RunOnNewThread(env); + Local task2 = runnable2.RunOnNewThread(env()); - Await(env, task1); - Await(env, task2); + Await(task1); + Await(task2); // Invoke `run()` again and ensure that `Detach()` successfully did its job; // that is, verify that `Run()` is not invoked. - env.Call(runnable1.GetJavaRunnable(), kRunnableRun); + env().Call(runnable1.GetJavaRunnable(), kRunnableRun); EXPECT_EQ(runnable1_run_invoke_count.load(), 1); } 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 f5503f4e88..88457a7dd2 100644 --- a/firestore/integration_test_internal/src/android/promise_android_test.cc +++ b/firestore/integration_test_internal/src/android/promise_android_test.cc @@ -61,10 +61,9 @@ class PromiseTest : public FirestoreAndroidIntegrationTest { void SetUp() override { FirestoreAndroidIntegrationTest::SetUp(); - jni::Env env = GetEnv(); - cancellation_token_source_ = CancellationTokenSource::Create(env); + cancellation_token_source_ = CancellationTokenSource::Create(env()); task_completion_source_ = TaskCompletionSource::Create( - env, cancellation_token_source_.GetToken(env)); + env(), cancellation_token_source_.GetToken(env())); } // An enum of asynchronous functions to use in tests, as required by @@ -78,27 +77,21 @@ class PromiseTest : public FirestoreAndroidIntegrationTest { PromiseFactory& promises() { return promises_; } jni::Local GetTask() { - jni::Env env = GetEnv(); - return task_completion_source_.GetTask(env); + return task_completion_source_.GetTask(env()); } void SetTaskResult(int result) { - jni::Env env = GetEnv(); - task_completion_source_.SetResult(env, jni::Integer::Create(env, result)); + task_completion_source_.SetResult(env(), + jni::Integer::Create(env(), result)); } void SetTaskException(Error error_code, const std::string& error_message) { - jni::Env env = GetEnv(); task_completion_source_.SetException( - env, ExceptionInternal::Create(env, error_code, error_message.c_str())); + env(), + ExceptionInternal::Create(env(), error_code, error_message.c_str())); } - void CancelTask() { - jni::Env env = GetEnv(); - cancellation_token_source_.Cancel(env); - } - - static jni::Env GetEnv() { return jni::Env(app_framework::GetJniEnv()); } + void CancelTask() { cancellation_token_source_.Cancel(env()); } private: PromiseFactory promises_; @@ -236,8 +229,7 @@ class TestVoidCompletion : public TestCompletionBase { }; TEST_F(PromiseTest, FutureVoidShouldSucceedWhenTaskSucceeds) { - jni::Env env = GetEnv(); - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask()); + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); SetTaskResult(0); @@ -249,9 +241,8 @@ TEST_F(PromiseTest, FutureVoidShouldSucceedWhenTaskSucceeds) { } TEST_F(PromiseTest, FutureNonVoidShouldSucceedWhenTaskSucceeds) { - jni::Env env = GetEnv(); auto future = - promises().NewFuture(env, AsyncFn::kFn, GetTask()); + promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); SetTaskResult(42); @@ -263,8 +254,7 @@ TEST_F(PromiseTest, FutureNonVoidShouldSucceedWhenTaskSucceeds) { } TEST_F(PromiseTest, FutureVoidShouldFailWhenTaskFails) { - jni::Env env = GetEnv(); - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask()); + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); SetTaskException(Error::kErrorFailedPrecondition, "Simulated failure"); @@ -277,9 +267,8 @@ TEST_F(PromiseTest, FutureVoidShouldFailWhenTaskFails) { } TEST_F(PromiseTest, FutureNonVoidShouldFailWhenTaskFails) { - jni::Env env = GetEnv(); auto future = - promises().NewFuture(env, AsyncFn::kFn, GetTask()); + promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); SetTaskException(Error::kErrorFailedPrecondition, "Simulated failure"); @@ -292,8 +281,7 @@ TEST_F(PromiseTest, FutureNonVoidShouldFailWhenTaskFails) { } TEST_F(PromiseTest, FutureVoidShouldCancelWhenTaskCancels) { - jni::Env env = GetEnv(); - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask()); + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); CancelTask(); @@ -306,9 +294,8 @@ TEST_F(PromiseTest, FutureVoidShouldCancelWhenTaskCancels) { } TEST_F(PromiseTest, FutureNonVoidShouldCancelWhenTaskCancels) { - jni::Env env = GetEnv(); auto future = - promises().NewFuture(env, AsyncFn::kFn, GetTask()); + promises().NewFuture(env(), AsyncFn::kFn, GetTask()); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); CancelTask(); @@ -321,9 +308,8 @@ TEST_F(PromiseTest, FutureNonVoidShouldCancelWhenTaskCancels) { } TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskSucceeds) { - jni::Env env = GetEnv(); TestVoidCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask(), + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -337,9 +323,8 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskSucceeds) { } TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskSucceeds) { - jni::Env env = GetEnv(); TestCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -353,9 +338,8 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskSucceeds) { } TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskFails) { - jni::Env env = GetEnv(); TestVoidCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask(), + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -369,9 +353,8 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskFails) { } TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskFails) { - jni::Env env = GetEnv(); TestCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -385,9 +368,8 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskFails) { } TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskCancels) { - jni::Env env = GetEnv(); TestVoidCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, GetTask(), + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -401,9 +383,8 @@ TEST_F(PromiseTest, FutureVoidShouldCallCompletionWhenTaskCancels) { } TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskCancels) { - jni::Env env = GetEnv(); TestCompletion completion; - auto future = promises().NewFuture(env, AsyncFn::kFn, + auto future = promises().NewFuture(env(), AsyncFn::kFn, GetTask(), &completion); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); @@ -417,17 +398,15 @@ TEST_F(PromiseTest, FutureNonVoidShouldCallCompletionWhenTaskCancels) { } TEST_F(PromiseTest, RegisterForTaskShouldNotCrashIfFirestoreWasDeleted) { - jni::Env env = GetEnv(); auto promise = promises().MakePromise(); DeleteFirestore(TestFirestore()); - promise.RegisterForTask(env, AsyncFn::kFn, GetTask()); + promise.RegisterForTask(env(), AsyncFn::kFn, GetTask()); } TEST_F(PromiseTest, GetFutureShouldNotCrashIfFirestoreWasDeleted) { - jni::Env env = GetEnv(); auto promise = promises().MakePromise(); - promise.RegisterForTask(env, AsyncFn::kFn, GetTask()); + promise.RegisterForTask(env(), AsyncFn::kFn, GetTask()); DeleteFirestore(TestFirestore()); auto future = promise.GetFuture(); diff --git a/firestore/integration_test_internal/src/android/promise_factory_android_test.cc b/firestore/integration_test_internal/src/android/promise_factory_android_test.cc index 446fc6bd96..06616838f4 100644 --- a/firestore/integration_test_internal/src/android/promise_factory_android_test.cc +++ b/firestore/integration_test_internal/src/android/promise_factory_android_test.cc @@ -23,7 +23,6 @@ #include "firebase/future.h" #include "firestore/src/android/converter_android.h" #include "firestore/src/android/firestore_android.h" -#include "firestore/src/jni/env.h" #include "firestore/src/jni/integer.h" #include "firestore/src/jni/ownership.h" #include "firestore/src/jni/task.h" @@ -34,7 +33,6 @@ namespace firebase { namespace firestore { namespace { -using jni::Env; using jni::Integer; using jni::Local; using jni::Task; @@ -54,24 +52,24 @@ class PromiseFactoryTest : public FirestoreAndroidIntegrationTest { }; protected: - void AssertCreatesValidFutures(Env& env, - PromiseFactory& promise_factory) { - Local tcs = TaskCompletionSource::Create(env); - Local task = tcs.GetTask(env); + static void AssertCreatesValidFutures( + PromiseFactory& promise_factory) { + Local tcs = TaskCompletionSource::Create(env()); + Local task = tcs.GetTask(env()); Future future = - promise_factory.NewFuture(env, AsyncFn::kFn, task); + promise_factory.NewFuture(env(), AsyncFn::kFn, task); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusPending); - tcs.SetResult(env, jni::Integer::Create(env, 42)); + tcs.SetResult(env(), jni::Integer::Create(env(), 42)); Await(future); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusComplete); } - void AssertCreatesInvalidFutures(Env& env, - PromiseFactory& promise_factory) { - Local tcs = TaskCompletionSource::Create(env); - Local task = tcs.GetTask(env); + static void AssertCreatesInvalidFutures( + PromiseFactory& promise_factory) { + Local tcs = TaskCompletionSource::Create(env()); + Local task = tcs.GetTask(env()); Future future = - promise_factory.NewFuture(env, AsyncFn::kFn, task); + promise_factory.NewFuture(env(), AsyncFn::kFn, task); EXPECT_EQ(future.status(), FutureStatus::kFutureStatusInvalid); } }; @@ -82,14 +80,13 @@ TEST_F(PromiseFactoryTest, CopyConstructor) { PromiseFactory promise_factory2(promise_factory1); - Env env; { SCOPED_TRACE("promise_factory1"); - AssertCreatesValidFutures(env, promise_factory1); + AssertCreatesValidFutures(promise_factory1); } { SCOPED_TRACE("promise_factory2"); - AssertCreatesValidFutures(env, promise_factory2); + AssertCreatesValidFutures(promise_factory2); } } @@ -100,14 +97,13 @@ TEST_F(PromiseFactoryTest, CopyConstructorWithDeletedFirestore) { PromiseFactory promise_factory2(promise_factory1); - Env env; { SCOPED_TRACE("promise_factory1"); - AssertCreatesInvalidFutures(env, promise_factory1); + AssertCreatesInvalidFutures(promise_factory1); } { SCOPED_TRACE("promise_factory2"); - AssertCreatesInvalidFutures(env, promise_factory2); + AssertCreatesInvalidFutures(promise_factory2); } } @@ -117,8 +113,7 @@ TEST_F(PromiseFactoryTest, MoveConstructor) { PromiseFactory promise_factory2(std::move(promise_factory1)); - Env env; - AssertCreatesValidFutures(env, promise_factory2); + AssertCreatesValidFutures(promise_factory2); } TEST_F(PromiseFactoryTest, MoveConstructorWithDeletedFirestore) { @@ -128,17 +123,15 @@ TEST_F(PromiseFactoryTest, MoveConstructorWithDeletedFirestore) { PromiseFactory promise_factory2(std::move(promise_factory1)); - Env env; - AssertCreatesInvalidFutures(env, promise_factory2); + AssertCreatesInvalidFutures(promise_factory2); } TEST_F(PromiseFactoryTest, ShouldCreateInvalidPromisesIfFirestoreIsDeleted) { Firestore* firestore = TestFirestore(); PromiseFactory promise_factory(GetFirestoreInternal(firestore)); DeleteFirestore(firestore); - Env env; - AssertCreatesInvalidFutures(env, promise_factory); + AssertCreatesInvalidFutures(promise_factory); } } // namespace diff --git a/firestore/integration_test_internal/src/android/settings_android_test.cc b/firestore/integration_test_internal/src/android/settings_android_test.cc index 1cff815518..f076edcaac 100644 --- a/firestore/integration_test_internal/src/android/settings_android_test.cc +++ b/firestore/integration_test_internal/src/android/settings_android_test.cc @@ -16,9 +16,8 @@ #include "firestore/src/android/settings_android.h" +#include "android/firestore_integration_test_android.h" #include "firestore/src/include/firebase/firestore/settings.h" -#include "firestore/src/jni/env.h" -#include "firestore_integration_test.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -26,13 +25,9 @@ namespace firebase { namespace firestore { namespace { -using jni::Env; - -using SettingsTestAndroid = FirestoreIntegrationTest; +using SettingsTestAndroid = FirestoreAndroidIntegrationTest; TEST_F(SettingsTestAndroid, ConverterBoolsAllTrue) { - Env env; - Settings settings; settings.set_host("foo"); settings.set_ssl_enabled(true); @@ -40,7 +35,7 @@ TEST_F(SettingsTestAndroid, ConverterBoolsAllTrue) { int64_t five_mb = 5 * 1024 * 1024; settings.set_cache_size_bytes(five_mb); - Settings result = SettingsInternal::Create(env, settings).ToPublic(env); + Settings result = SettingsInternal::Create(env(), settings).ToPublic(env()); EXPECT_EQ(result.host(), "foo"); EXPECT_TRUE(result.is_ssl_enabled()); @@ -49,14 +44,12 @@ TEST_F(SettingsTestAndroid, ConverterBoolsAllTrue) { } TEST_F(SettingsTestAndroid, ConverterBoolsAllFalse) { - Env env; - Settings settings; settings.set_host("bar"); settings.set_ssl_enabled(false); settings.set_persistence_enabled(false); - Settings result = SettingsInternal::Create(env, settings).ToPublic(env); + Settings result = SettingsInternal::Create(env(), settings).ToPublic(env()); EXPECT_EQ("bar", result.host()); EXPECT_FALSE(result.is_ssl_enabled()); diff --git a/firestore/integration_test_internal/src/android/snapshot_metadata_android_test.cc b/firestore/integration_test_internal/src/android/snapshot_metadata_android_test.cc index a9965f2383..4fabe4a2a0 100644 --- a/firestore/integration_test_internal/src/android/snapshot_metadata_android_test.cc +++ b/firestore/integration_test_internal/src/android/snapshot_metadata_android_test.cc @@ -20,8 +20,6 @@ #include "firebase_test_framework.h" #include "firestore/src/include/firebase/firestore/snapshot_metadata.h" #include "firestore/src/jni/declaration.h" -#include "firestore/src/jni/env.h" -#include "firestore_integration_test.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -32,29 +30,27 @@ using SnapshotMetadataAndroidTest = FirestoreAndroidIntegrationTest; using jni::Class; using jni::Constructor; -using jni::Env; using jni::Local; TEST_F(SnapshotMetadataAndroidTest, Converts) { - Env env; Constructor ctor("(ZZ)V"); loader().LoadClass("com/google/firebase/firestore/SnapshotMetadata", ctor); ASSERT_TRUE(loader().ok()); { auto java_metadata = - env.New(ctor, /*has_pending_writes=*/true, /*is_from_cache=*/false); - SnapshotMetadata result = java_metadata.ToPublic(env); - EXPECT_TRUE(env.ok()); + env().New(ctor, /*has_pending_writes=*/true, /*is_from_cache=*/false); + SnapshotMetadata result = java_metadata.ToPublic(env()); + EXPECT_TRUE(env().ok()); EXPECT_TRUE(result.has_pending_writes()); EXPECT_FALSE(result.is_from_cache()); } { auto java_metadata = - env.New(ctor, /*has_pending_writes=*/false, /*is_from_cache=*/true); - SnapshotMetadata result = java_metadata.ToPublic(env); - EXPECT_TRUE(env.ok()); + env().New(ctor, /*has_pending_writes=*/false, /*is_from_cache=*/true); + SnapshotMetadata result = java_metadata.ToPublic(env()); + EXPECT_TRUE(env().ok()); EXPECT_FALSE(result.has_pending_writes()); EXPECT_TRUE(result.is_from_cache()); } diff --git a/firestore/integration_test_internal/src/android/timestamp_android_test.cc b/firestore/integration_test_internal/src/android/timestamp_android_test.cc index b0c14dcc72..fb461f6a2e 100644 --- a/firestore/integration_test_internal/src/android/timestamp_android_test.cc +++ b/firestore/integration_test_internal/src/android/timestamp_android_test.cc @@ -16,25 +16,20 @@ #include "firestore/src/android/timestamp_android.h" +#include "android/firestore_integration_test_android.h" #include "firebase/firestore/timestamp.h" -#include "firestore/src/jni/env.h" -#include "firestore_integration_test.h" #include "gtest/gtest.h" namespace firebase { namespace firestore { namespace { -using jni::Env; - -using TimestampTest = FirestoreIntegrationTest; +using TimestampTest = FirestoreAndroidIntegrationTest; TEST_F(TimestampTest, Converts) { - Env env; - Timestamp timestamp{1234, 5678}; - auto java_timestamp = TimestampInternal::Create(env, timestamp); - EXPECT_EQ(timestamp, java_timestamp.ToPublic(env)); + auto java_timestamp = TimestampInternal::Create(env(), timestamp); + EXPECT_EQ(timestamp, java_timestamp.ToPublic(env())); } } // namespace diff --git a/firestore/integration_test_internal/src/android/transaction_options_android_test.cc b/firestore/integration_test_internal/src/android/transaction_options_android_test.cc index aa12fe54f7..893c24bdcb 100644 --- a/firestore/integration_test_internal/src/android/transaction_options_android_test.cc +++ b/firestore/integration_test_internal/src/android/transaction_options_android_test.cc @@ -17,8 +17,7 @@ #include "firestore/src/android/transaction_options_android.h" #include "firestore/src/android/transaction_options_builder_android.h" -#include "firestore/src/jni/env.h" -#include "firestore_integration_test.h" +#include "android/firestore_integration_test_android.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -26,40 +25,36 @@ namespace firebase { namespace firestore { namespace { -using jni::Env; using jni::Local; -using TransactionOptionsTestAndroid = FirestoreIntegrationTest; +using TransactionOptionsTestAndroid = FirestoreAndroidIntegrationTest; TEST_F(TransactionOptionsTestAndroid, DefaultTransactionOptions) { - Env env; Local builder = - TransactionOptionsBuilderInternal::Create(env); + TransactionOptionsBuilderInternal::Create(env()); - Local options = builder.Build(env); + Local options = builder.Build(env()); - EXPECT_EQ(options.GetMaxAttempts(env), 5); + EXPECT_EQ(options.GetMaxAttempts(env()), 5); } TEST_F(TransactionOptionsTestAndroid, SetMaxAttemptsReturnsSameInstance) { - Env env; Local builder = - TransactionOptionsBuilderInternal::Create(env); + TransactionOptionsBuilderInternal::Create(env()); Local retval = - builder.SetMaxAttempts(env, 42); + builder.SetMaxAttempts(env(), 42); - EXPECT_TRUE(env.IsSameObject(builder, retval)); + EXPECT_TRUE(env().IsSameObject(builder, retval)); } TEST_F(TransactionOptionsTestAndroid, SetMaxAttempts) { - Env env; Local builder = - TransactionOptionsBuilderInternal::Create(env); + TransactionOptionsBuilderInternal::Create(env()); - builder.SetMaxAttempts(env, 42); + builder.SetMaxAttempts(env(), 42); - EXPECT_EQ(builder.Build(env).GetMaxAttempts(env), 42); + EXPECT_EQ(builder.Build(env()).GetMaxAttempts(env()), 42); } } // namespace diff --git a/firestore/integration_test_internal/src/firestore_test.cc b/firestore/integration_test_internal/src/firestore_test.cc index b82c3aa638..70d5974ef8 100644 --- a/firestore/integration_test_internal/src/firestore_test.cc +++ b/firestore/integration_test_internal/src/firestore_test.cc @@ -1850,14 +1850,13 @@ TEST_F(FirestoreTest, FirestoreCanBeDeletedFromTransaction) { #if defined(__ANDROID__) TEST_F(FirestoreAndroidIntegrationTest, CanDeleteFirestoreInstanceOnJavaMainThread) { - jni::Env env; Firestore* db = TestFirestore(); - auto runnable = MakeJniRunnable(env, [db] { delete db; }); + auto runnable = MakeJniRunnable(env(), [db] { delete db; }); - jni::Local task = runnable.RunOnMainThread(env); + jni::Local task = runnable.RunOnMainThread(env()); - Await(env, task); - EXPECT_TRUE(task.IsSuccessful(env)); + Await(task); + EXPECT_TRUE(task.IsSuccessful(env())); DisownFirestore(db); // Avoid double-deletion of the `db`. } #endif // defined(__ANDROID__) diff --git a/firestore/integration_test_internal/src/jni/task_test.cc b/firestore/integration_test_internal/src/jni/task_test.cc index 0ec09af81c..c3a0a27dca 100644 --- a/firestore/integration_test_internal/src/jni/task_test.cc +++ b/firestore/integration_test_internal/src/jni/task_test.cc @@ -19,7 +19,6 @@ #include "android/cancellation_token_source.h" #include "android/firestore_integration_test_android.h" #include "android/task_completion_source.h" -#include "firestore/src/jni/env.h" #include "firestore/src/jni/object.h" #include "firestore/src/jni/ownership.h" #include "firestore/src/jni/string.h" @@ -35,61 +34,58 @@ namespace { class TaskTest : public FirestoreAndroidIntegrationTest { protected: - Local CreateIncompleteTask(Env& env) { - return TaskCompletionSource::Create(env).GetTask(env); + static Local CreateIncompleteTask() { + return TaskCompletionSource::Create(env()).GetTask(env()); } template - Local CreateSuccessfulTask(Env& env, T result) { - auto tcs = TaskCompletionSource::Create(env); - tcs.SetResult(env, result); - return tcs.GetTask(env); + static Local CreateSuccessfulTask(T result) { + auto tcs = TaskCompletionSource::Create(env()); + tcs.SetResult(env(), result); + return tcs.GetTask(env()); } - Local CreateSuccessfulTask(Env& env) { - auto result = env.NewStringUtf("Fake Result"); - return CreateSuccessfulTask(env, result); + static Local CreateSuccessfulTask() { + Local result = env().NewStringUtf("Fake Result"); + return CreateSuccessfulTask(result); } - Local CreateFailedTask(Env& env, Throwable exception) { - auto tcs = TaskCompletionSource::Create(env); - tcs.SetException(env, exception); - return tcs.GetTask(env); + static Local CreateFailedTask(const Throwable& exception) { + auto tcs = TaskCompletionSource::Create(env()); + tcs.SetException(env(), exception); + return tcs.GetTask(env()); } - Local CreateFailedTask(Env& env) { - auto exception = CreateException(env); - return CreateFailedTask(env, exception); + static Local CreateFailedTask() { + return CreateFailedTask(CreateException()); } - Local CreateCancelledTask(Env& env) { - auto cts = CancellationTokenSource::Create(env); - auto tcs = TaskCompletionSource::Create(env, cts.GetToken(env)); - cts.Cancel(env); - auto task = tcs.GetTask(env); + static Local CreateCancelledTask() { + auto cts = CancellationTokenSource::Create(env()); + auto tcs = TaskCompletionSource::Create(env(), cts.GetToken(env())); + cts.Cancel(env()); + auto task = tcs.GetTask(env()); // Wait for the `Task` to be "completed" because `Cancel()` marks the `Task` // as "completed" asynchronously. - Await(env, task); + Await(task); return task; } }; TEST_F(TaskTest, GetResultShouldReturnTheResult) { - Env env; - Local result = env.NewStringUtf("Fake Result"); - Local task = CreateSuccessfulTask(env, result); + Local result = env().NewStringUtf("Fake Result"); + Local task = CreateSuccessfulTask(result); - Local actual_result = task.GetResult(env); + Local actual_result = task.GetResult(env()); - ASSERT_THAT(actual_result, JavaEq(result)); + ASSERT_THAT(actual_result, RefersToSameJavaObjectAs(result)); } TEST_F(TaskTest, GetExceptionShouldReturnTheException) { - Env env; - Local exception = CreateException(env); - Local task = CreateFailedTask(env, exception); + Local exception = CreateException(); + Local task = CreateFailedTask(exception); - Local actual_exception = task.GetException(env); + Local actual_exception = task.GetException(env()); ASSERT_THAT(actual_exception, JavaEq(exception)); } @@ -97,91 +93,79 @@ TEST_F(TaskTest, GetExceptionShouldReturnTheException) { // Tests for Task.IsComplete() TEST_F(TaskTest, IsCompleteShouldReturnFalseForIncompleteTask) { - Env env; - Local task = CreateIncompleteTask(env); + Local task = CreateIncompleteTask(); - ASSERT_FALSE(task.IsComplete(env)); + ASSERT_FALSE(task.IsComplete(env())); } TEST_F(TaskTest, IsCompleteShouldReturnTrueForSucceededTask) { - Env env; - Local task = CreateSuccessfulTask(env); + Local task = CreateSuccessfulTask(); - ASSERT_TRUE(task.IsComplete(env)); + ASSERT_TRUE(task.IsComplete(env())); } TEST_F(TaskTest, IsCompleteShouldReturnTrueForFailedTask) { - Env env; - Local task = CreateFailedTask(env); + Local task = CreateFailedTask(); - ASSERT_TRUE(task.IsComplete(env)); + ASSERT_TRUE(task.IsComplete(env())); } TEST_F(TaskTest, IsCompleteShouldReturnTrueForCanceledTask) { - Env env; - Local task = CreateCancelledTask(env); + Local task = CreateCancelledTask(); - ASSERT_TRUE(task.IsComplete(env)); + ASSERT_TRUE(task.IsComplete(env())); } // Tests for Task.IsSuccessful() TEST_F(TaskTest, IsSuccessfulShouldReturnFalseForIncompleteTask) { - Env env; - Local task = CreateIncompleteTask(env); + Local task = CreateIncompleteTask(); - ASSERT_FALSE(task.IsSuccessful(env)); + ASSERT_FALSE(task.IsSuccessful(env())); } TEST_F(TaskTest, IsSuccessfulShouldReturnTrueForSucceededTask) { - Env env; - Local task = CreateSuccessfulTask(env); + Local task = CreateSuccessfulTask(); - ASSERT_TRUE(task.IsSuccessful(env)); + ASSERT_TRUE(task.IsSuccessful(env())); } TEST_F(TaskTest, IsSuccessfulShouldReturnFalseForFailedTask) { - Env env; - Local task = CreateFailedTask(env); + Local task = CreateFailedTask(); - ASSERT_FALSE(task.IsSuccessful(env)); + ASSERT_FALSE(task.IsSuccessful(env())); } TEST_F(TaskTest, IsSuccessfulShouldReturnFalseForCanceledTask) { - Env env; - Local task = CreateCancelledTask(env); + Local task = CreateCancelledTask(); - ASSERT_FALSE(task.IsSuccessful(env)); + ASSERT_FALSE(task.IsSuccessful(env())); } // Tests for Task.IsCanceled() TEST_F(TaskTest, IsCanceledShouldReturnFalseForIncompleteTask) { - Env env; - Local task = CreateIncompleteTask(env); + Local task = CreateIncompleteTask(); - ASSERT_FALSE(task.IsCanceled(env)); + ASSERT_FALSE(task.IsCanceled(env())); } TEST_F(TaskTest, IsCanceledShouldReturnFalseForSucceededTask) { - Env env; - Local task = CreateSuccessfulTask(env); + Local task = CreateSuccessfulTask(); - ASSERT_FALSE(task.IsCanceled(env)); + ASSERT_FALSE(task.IsCanceled(env())); } TEST_F(TaskTest, IsCanceledShouldReturnFalseForFailedTask) { - Env env; - Local task = CreateFailedTask(env); + Local task = CreateFailedTask(); - ASSERT_FALSE(task.IsCanceled(env)); + ASSERT_FALSE(task.IsCanceled(env())); } TEST_F(TaskTest, IsCanceledShouldReturnTrueForCanceledTask) { - Env env; - Local task = CreateCancelledTask(env); + Local task = CreateCancelledTask(); - ASSERT_TRUE(task.IsCanceled(env)); + ASSERT_TRUE(task.IsCanceled(env())); } } // namespace From 148f6d92eff544d33b3545cce98088e2c2f50913 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Jun 2023 22:19:06 -0400 Subject: [PATCH 2/4] firestore_integration_test_android.h: avoid inlining env() so we don't end up with a tonne of thread_local env variables. --- .../src/android/firestore_integration_test_android.cc | 5 +++++ .../src/android/firestore_integration_test_android.h | 5 +---- 2 files changed, 6 insertions(+), 4 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 c1b80106e9..673e43d2de 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 @@ -143,5 +143,10 @@ void FirestoreAndroidIntegrationTest::Await(const Task& task) { } } +jni::Env& FirestoreAndroidIntegrationTest::env() { + thread_local jni::Env env; + return env; +} + } // namespace firestore } // namespace firebase diff --git a/firestore/integration_test_internal/src/android/firestore_integration_test_android.h b/firestore/integration_test_internal/src/android/firestore_integration_test_android.h index 3d11b0787b..9ce2b9b4e7 100644 --- a/firestore/integration_test_internal/src/android/firestore_integration_test_android.h +++ b/firestore/integration_test_internal/src/android/firestore_integration_test_android.h @@ -123,10 +123,7 @@ class FirestoreAndroidIntegrationTest : public FirestoreIntegrationTest { static void Await(const jni::Task& task); /** Returns an Env object for the calling thread, creating it if necessary. */ - static jni::Env& env() { - thread_local jni::Env env; - return env; - } + static jni::Env& env(); private: void FailTestIfPendingException(); From be10689aafa1a00e85f557660c0fc41938b4c37c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Jun 2023 22:28:12 -0400 Subject: [PATCH 3/4] task_test.cc: undo accidental change --- firestore/integration_test_internal/src/jni/task_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/jni/task_test.cc b/firestore/integration_test_internal/src/jni/task_test.cc index c3a0a27dca..f4dfd8ae76 100644 --- a/firestore/integration_test_internal/src/jni/task_test.cc +++ b/firestore/integration_test_internal/src/jni/task_test.cc @@ -78,7 +78,7 @@ TEST_F(TaskTest, GetResultShouldReturnTheResult) { Local actual_result = task.GetResult(env()); - ASSERT_THAT(actual_result, RefersToSameJavaObjectAs(result)); + ASSERT_THAT(actual_result, JavaEq(result)); } TEST_F(TaskTest, GetExceptionShouldReturnTheException) { From f69f5aefc6281ec688ae0ab406dddf89ab1ff12a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Jun 2023 22:29:53 -0400 Subject: [PATCH 4/4] jni_runnable_android_test.cc: undo an accidental change --- .../src/android/jni_runnable_android_test.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc b/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc index a61394c374..f44472e764 100644 --- a/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc +++ b/firestore/integration_test_internal/src/android/jni_runnable_android_test.cc @@ -157,7 +157,8 @@ TEST_F(JniRunnableTest, DetachDetachesEvenIfAnExceptionIsPending) { bool invoked = false; auto runnable = MakeJniRunnable(env(), [&invoked] { invoked = true; }); Local java_runnable = runnable.GetJavaRunnable(); - Local exception = ThrowException(); + Local exception = CreateException(); + env().Throw(exception); EXPECT_FALSE(env().ok()); runnable.Detach(env());