Skip to content

Firestore: Improve Android tests to use the new env() helper function rather than creating an Env on the stack #1357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 23, 2023
Merged

Firestore: Improve Android tests to use the new env() helper function rather than creating an Env on the stack #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
83efa26
Replace explicit Env creation with the env() helper member function
dconeybe Jun 21, 2023
148f6d9
firestore_integration_test_android.h: avoid inlining env() so we don'…
dconeybe Jun 22, 2023
be10689
task_test.cc: undo accidental change
dconeybe Jun 22, 2023
f69f5ae
jni_runnable_android_test.cc: undo an accidental change
dconeybe Jun 22, 2023
d464423
Merge branch 'main' into dconeybe/EnvRawUsagesInTestsRemoved
dconeybe Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 28 additions & 25 deletions firestore/integration_test_internal/src/android/firestore_integration_test_android.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -77,73 +76,77 @@ void FirestoreAndroidIntegrationTest::TearDown() {
}

void FirestoreAndroidIntegrationTest::FailTestIfPendingException() {
Env env;
Local<Throwable> pending_exception = env.ClearExceptionOccurred();
Local<Throwable> 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<Throwable> FirestoreAndroidIntegrationTest::CreateException(Env& env) {
return CreateException(env,
"Test exception created by "
"FirestoreAndroidIntegrationTest::CreateException()");
Local<Throwable> FirestoreAndroidIntegrationTest::CreateException() {
return CreateException(
"Test exception created by "
"FirestoreAndroidIntegrationTest::CreateException()");
}

Local<Throwable> FirestoreAndroidIntegrationTest::CreateException(
Env& env, const std::string& message) {
ExceptionClearGuard exception_clear_guard(env);
Local<String> java_message = env.NewStringUtf(message);
return env.New(kExceptionConstructor, java_message);
const std::string& message) {
ExceptionClearGuard exception_clear_guard(env());
Local<String> java_message = env().NewStringUtf(message);
return env().New(kExceptionConstructor, java_message);
}

Local<Throwable> FirestoreAndroidIntegrationTest::ThrowException(Env& env) {
return ThrowException(env,
"Test exception thrown by "
"FirestoreAndroidIntegrationTest::ThrowException()");
Local<Throwable> FirestoreAndroidIntegrationTest::ThrowException() {
return ThrowException(
"Test exception thrown by "
"FirestoreAndroidIntegrationTest::ThrowException()");
}

Local<Throwable> 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<Throwable> exception = CreateException(env, message);
Local<Throwable> 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;
break;
}
--cycles;
}
if (env.ok()) {
if (env().ok()) {
EXPECT_GT(cycles, 0) << "Waiting for Task timed out.";
}
}

jni::Env& FirestoreAndroidIntegrationTest::env() {
thread_local jni::Env env;
return env;
}

} // namespace firestore
} // namespace firebase
15 changes: 8 additions & 7 deletions firestore/integration_test_internal/src/android/firestore_integration_test_android.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,23 +106,24 @@ class FirestoreAndroidIntegrationTest : public FirestoreIntegrationTest {
jni::Loader& loader() { return loader_; }

/** Creates and returns a new Java `Exception` with a default message. */
static jni::Local<jni::Throwable> CreateException(jni::Env&);
static jni::Local<jni::Throwable> CreateException();
/** Creates and returns a new Java `Exception` with the given message. */
static jni::Local<jni::Throwable> CreateException(jni::Env&,
const std::string& message);
static jni::Local<jni::Throwable> CreateException(const std::string& message);

/** Throws a Java `Exception` object with a default message. */
jni::Local<jni::Throwable> ThrowException(jni::Env&);
jni::Local<jni::Throwable> ThrowException();
/** Throws a Java `Exception` object with the given message. */
jni::Local<jni::Throwable> ThrowException(jni::Env&,
const std::string& message);
jni::Local<jni::Throwable> 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();

private:
void FailTestIfPendingException();
Expand Down
77 changes: 30 additions & 47 deletions firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ namespace firestore {
namespace {

using jni::ArrayList;
using jni::Env;
using jni::Global;
using jni::Local;
using jni::Object;
Expand All @@ -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");
}
Expand All @@ -59,50 +56,45 @@ TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithNull) {

TEST_F(FirestoreAndroidIntegrationTest,
ToDebugStringWithPendingExceptionAndNonNullObject) {
Env env;
Local<String> object = env.NewStringUtf("Test Value");
ThrowException(env);
ASSERT_FALSE(env.ok());
Local<String> 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<String> object1 = env.NewStringUtf("string");
Local<String> object2 = env.NewStringUtf("string");
Local<String> object1 = env().NewStringUtf("string");
Local<String> object2 = env().NewStringUtf("string");

EXPECT_THAT(object1, JavaEq(object2));
}

TEST_F(FirestoreAndroidIntegrationTest,
JavaEqShouldReturnFalseForUnequalObjects) {
Env env;
Local<String> object1 = env.NewStringUtf("string1");
Local<String> object2 = env.NewStringUtf("string2");
Local<String> object1 = env().NewStringUtf("string1");
Local<String> object2 = env().NewStringUtf("string2");

EXPECT_THAT(object1, Not(JavaEq(object2)));
}

TEST_F(FirestoreAndroidIntegrationTest,
JavaEqShouldReturnTrueForTwoNullReferences) {
Env env;
Local<Object> null_reference1;
Local<Object> null_reference2;

Expand All @@ -111,28 +103,25 @@ TEST_F(FirestoreAndroidIntegrationTest,

TEST_F(FirestoreAndroidIntegrationTest,
JavaEqShouldReturnFalseIfExactlyOneObjectIsNull) {
Env env;
Local<String> null_reference;
Local<String> non_null_reference = env.NewStringUtf("string2");
Local<String> non_null_reference = env().NewStringUtf("string2");

EXPECT_THAT(null_reference, Not(JavaEq(non_null_reference)));
EXPECT_THAT(non_null_reference, Not(JavaEq(null_reference)));
}

TEST_F(FirestoreAndroidIntegrationTest,
JavaEqShouldReturnFalseForObjectOfDifferentTypes) {
Env env;
Local<String> string_object = env.NewStringUtf("string2");
Local<ArrayList> list_object = ArrayList::Create(env);
Local<String> string_object = env().NewStringUtf("string2");
Local<ArrayList> list_object = ArrayList::Create(env());

EXPECT_THAT(string_object, Not(JavaEq(list_object)));
EXPECT_THAT(list_object, Not(JavaEq(string_object)));
}

TEST_F(FirestoreAndroidIntegrationTest,
RefersToSameJavaObjectAsShouldReturnTrueForSameObjects) {
Env env;
Local<String> object1 = env.NewStringUtf("string");
Local<String> object1 = env().NewStringUtf("string");
Global<String> object2 = object1;

EXPECT_THAT(object1, RefersToSameJavaObjectAs(object2));
Expand All @@ -148,19 +137,17 @@ TEST_F(FirestoreAndroidIntegrationTest,

TEST_F(FirestoreAndroidIntegrationTest,
RefersToSameJavaObjectAsShouldReturnFalseForDistinctObjects) {
Env env;
Local<String> object1 = env.NewStringUtf("test string");
Local<String> object2 = env.NewStringUtf("test string");
ASSERT_FALSE(env.IsSameObject(object1, object2));
Local<String> object1 = env().NewStringUtf("test string");
Local<String> object2 = env().NewStringUtf("test string");
ASSERT_FALSE(env().IsSameObject(object1, object2));

EXPECT_THAT(object1, Not(RefersToSameJavaObjectAs(object2)));
}

TEST_F(FirestoreAndroidIntegrationTest,
RefersToSameJavaObjectAsShouldReturnFalseIfExactlyOneObjectIsNull) {
Env env;
Local<String> null_reference;
Local<String> non_null_reference = env.NewStringUtf("string2");
Local<String> non_null_reference = env().NewStringUtf("string2");

EXPECT_THAT(null_reference,
Not(RefersToSameJavaObjectAs(non_null_reference)));
Expand All @@ -170,42 +157,38 @@ TEST_F(FirestoreAndroidIntegrationTest,

TEST_F(FirestoreAndroidIntegrationTest,
ThrowExceptionWithNoMessageShouldSetPendingExceptionWithAMessage) {
Env env;
Local<Throwable> throw_exception_return_value = ThrowException(env);
Local<Throwable> actually_thrown_exception = env.ClearExceptionOccurred();
Local<Throwable> throw_exception_return_value = ThrowException();
Local<Throwable> 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<Throwable> throw_exception_return_value =
ThrowException(env, "my test message");
Local<Throwable> actually_thrown_exception = env.ClearExceptionOccurred();
ThrowException("my test message");
Local<Throwable> 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<Throwable> exception = CreateException(env);
Local<Throwable> 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<Throwable> exception = CreateException(env, "my test message");
Local<Throwable> 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
Expand Down
13 changes: 4 additions & 9 deletions firestore/integration_test_internal/src/android/geo_point_android_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading