Skip to content

Commit 99b39b0

Browse files
dconeybedconeybe
authored
Firestore: Improve Android tests to use the new env() helper function rather than creating an Env on the stack (#1357)
1 parent 055e879 commit 99b39b0

13 files changed

+307
-416
lines changed

‎firestore/integration_test_internal/src/android/firestore_integration_test_android.cc‎

Lines changed: 28 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ void PrintTo(const Object& object, std::ostream* os) {
4242
} // namespace jni
4343

4444
using jni::Constructor;
45-
using jni::Env;
4645
using jni::ExceptionClearGuard;
4746
using jni::Local;
4847
using jni::String;
@@ -77,73 +76,77 @@ void FirestoreAndroidIntegrationTest::TearDown() {
7776
}
7877

7978
void FirestoreAndroidIntegrationTest::FailTestIfPendingException() {
80-
Env env;
81-
Local<Throwable> pending_exception = env.ClearExceptionOccurred();
79+
Local<Throwable> pending_exception = env().ClearExceptionOccurred();
8280
if (!pending_exception) {
8381
return;
8482
}
8583

8684
// Ignore the exception if it was thrown by the last ThrowException() call.
87-
if (env.IsSameObject(pending_exception, last_thrown_exception_)) {
85+
if (env().IsSameObject(pending_exception, last_thrown_exception_)) {
8886
return;
8987
}
9088

9189
// Fail the test since the test completed with a pending exception.
92-
std::string pending_exception_as_string = pending_exception.ToString(env);
93-
env.ExceptionClear();
90+
std::string pending_exception_as_string = pending_exception.ToString(env());
91+
env().ExceptionClear();
9492
FAIL() << "Test completed with a pending Java exception: "
9593
<< pending_exception_as_string;
9694
}
9795

98-
Local<Throwable> FirestoreAndroidIntegrationTest::CreateException(Env& env) {
99-
return CreateException(env,
100-
"Test exception created by "
101-
"FirestoreAndroidIntegrationTest::CreateException()");
96+
Local<Throwable> FirestoreAndroidIntegrationTest::CreateException() {
97+
return CreateException(
98+
"Test exception created by "
99+
"FirestoreAndroidIntegrationTest::CreateException()");
102100
}
103101

104102
Local<Throwable> FirestoreAndroidIntegrationTest::CreateException(
105-
Env& env, const std::string& message) {
106-
ExceptionClearGuard exception_clear_guard(env);
107-
Local<String> java_message = env.NewStringUtf(message);
108-
return env.New(kExceptionConstructor, java_message);
103+
const std::string& message) {
104+
ExceptionClearGuard exception_clear_guard(env());
105+
Local<String> java_message = env().NewStringUtf(message);
106+
return env().New(kExceptionConstructor, java_message);
109107
}
110108

111-
Local<Throwable> FirestoreAndroidIntegrationTest::ThrowException(Env& env) {
112-
return ThrowException(env,
113-
"Test exception thrown by "
114-
"FirestoreAndroidIntegrationTest::ThrowException()");
109+
Local<Throwable> FirestoreAndroidIntegrationTest::ThrowException() {
110+
return ThrowException(
111+
"Test exception thrown by "
112+
"FirestoreAndroidIntegrationTest::ThrowException()");
115113
}
116114

117115
Local<Throwable> FirestoreAndroidIntegrationTest::ThrowException(
118-
Env& env, const std::string& message) {
119-
if (!env.ok()) {
116+
const std::string& message) {
117+
if (!env().ok()) {
120118
ADD_FAILURE() << "ThrowException() invoked while there is already a "
121119
"pending exception";
122120
return {};
123121
}
124-
Local<Throwable> exception = CreateException(env, message);
122+
Local<Throwable> exception = CreateException(message);
125123

126124
// Silently discard this exception if the test ends with it still pending.
127125
last_thrown_exception_ = exception;
128126

129-
env.Throw(exception);
127+
env().Throw(exception);
130128
return exception;
131129
}
132130

133-
void FirestoreAndroidIntegrationTest::Await(Env& env, const Task& task) {
131+
void FirestoreAndroidIntegrationTest::Await(const Task& task) {
134132
int cycles = kTimeOutMillis / kCheckIntervalMillis;
135-
while (env.ok() && !task.IsComplete(env)) {
133+
while (env().ok() && !task.IsComplete(env())) {
136134
if (ProcessEvents(kCheckIntervalMillis)) {
137135
std::cout << "WARNING: app receives an event requesting exit."
138136
<< std::endl;
139137
break;
140138
}
141139
--cycles;
142140
}
143-
if (env.ok()) {
141+
if (env().ok()) {
144142
EXPECT_GT(cycles, 0) << "Waiting for Task timed out.";
145143
}
146144
}
147145

146+
jni::Env& FirestoreAndroidIntegrationTest::env() {
147+
thread_local jni::Env env;
148+
return env;
149+
}
150+
148151
} // namespace firestore
149152
} // namespace firebase

‎firestore/integration_test_internal/src/android/firestore_integration_test_android.h‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -106,23 +106,24 @@ class FirestoreAndroidIntegrationTest : public FirestoreIntegrationTest {
106106
jni::Loader& loader() { return loader_; }
107107

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

114113
/** Throws a Java `Exception` object with a default message. */
115-
jni::Local<jni::Throwable> ThrowException(jni::Env&);
114+
jni::Local<jni::Throwable> ThrowException();
116115
/** Throws a Java `Exception` object with the given message. */
117-
jni::Local<jni::Throwable> ThrowException(jni::Env&,
118-
const std::string& message);
116+
jni::Local<jni::Throwable> ThrowException(const std::string& message);
119117

120118
// Bring definitions of `Await()` from the superclass into this class so that
121119
// the definition below *overloads* instead of *hides* them.
122120
using FirestoreIntegrationTest::Await;
123121

124122
/** Blocks until the given `Task` has completed or times out. */
125-
static void Await(jni::Env& env, const jni::Task& task);
123+
static void Await(const jni::Task& task);
124+
125+
/** Returns an Env object for the calling thread, creating it if necessary. */
126+
static jni::Env& env();
126127

127128
private:
128129
void FailTestIfPendingException();

‎firestore/integration_test_internal/src/android/firestore_integration_test_android_test.cc‎

Lines changed: 30 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ namespace firestore {
3030
namespace {
3131

3232
using jni::ArrayList;
33-
using jni::Env;
3433
using jni::Global;
3534
using jni::Local;
3635
using jni::Object;
@@ -42,9 +41,7 @@ using ::testing::Not;
4241
using ::testing::StrEq;
4342

4443
TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithNonNull) {
45-
Env env;
46-
47-
std::string debug_string = ToDebugString(env.NewStringUtf("Test Value"));
44+
std::string debug_string = ToDebugString(env().NewStringUtf("Test Value"));
4845

4946
EXPECT_EQ(debug_string, "Test Value");
5047
}
@@ -59,50 +56,45 @@ TEST_F(FirestoreAndroidIntegrationTest, ToDebugStringWithNull) {
5956

6057
TEST_F(FirestoreAndroidIntegrationTest,
6158
ToDebugStringWithPendingExceptionAndNonNullObject) {
62-
Env env;
63-
Local<String> object = env.NewStringUtf("Test Value");
64-
ThrowException(env);
65-
ASSERT_FALSE(env.ok());
59+
Local<String> object = env().NewStringUtf("Test Value");
60+
ThrowException();
61+
ASSERT_FALSE(env().ok());
6662

6763
std::string debug_string = ToDebugString(object);
6864

6965
EXPECT_EQ(debug_string, "Test Value");
70-
env.ExceptionClear();
66+
env().ExceptionClear();
7167
}
7268

7369
TEST_F(FirestoreAndroidIntegrationTest,
7470
ToDebugStringWithPendingExceptionAndNullObject) {
75-
Env env;
7671
Object null_reference;
77-
ThrowException(env);
78-
ASSERT_FALSE(env.ok());
72+
ThrowException();
73+
ASSERT_FALSE(env().ok());
7974

8075
std::string debug_string = ToDebugString(null_reference);
8176

8277
EXPECT_EQ(debug_string, "null");
83-
env.ExceptionClear();
78+
env().ExceptionClear();
8479
}
8580

8681
TEST_F(FirestoreAndroidIntegrationTest, JavaEqShouldReturnTrueForEqualObjects) {
87-
Env env;
88-
Local<String> object1 = env.NewStringUtf("string");
89-
Local<String> object2 = env.NewStringUtf("string");
82+
Local<String> object1 = env().NewStringUtf("string");
83+
Local<String> object2 = env().NewStringUtf("string");
9084

9185
EXPECT_THAT(object1, JavaEq(object2));
9286
}
9387

9488
TEST_F(FirestoreAndroidIntegrationTest,
9589
JavaEqShouldReturnFalseForUnequalObjects) {
96-
Env env;
97-
Local<String> object1 = env.NewStringUtf("string1");
98-
Local<String> object2 = env.NewStringUtf("string2");
90+
Local<String> object1 = env().NewStringUtf("string1");
91+
Local<String> object2 = env().NewStringUtf("string2");
9992

10093
EXPECT_THAT(object1, Not(JavaEq(object2)));
10194
}
10295

10396
TEST_F(FirestoreAndroidIntegrationTest,
10497
JavaEqShouldReturnTrueForTwoNullReferences) {
105-
Env env;
10698
Local<Object> null_reference1;
10799
Local<Object> null_reference2;
108100

@@ -111,28 +103,25 @@ TEST_F(FirestoreAndroidIntegrationTest,
111103

112104
TEST_F(FirestoreAndroidIntegrationTest,
113105
JavaEqShouldReturnFalseIfExactlyOneObjectIsNull) {
114-
Env env;
115106
Local<String> null_reference;
116-
Local<String> non_null_reference = env.NewStringUtf("string2");
107+
Local<String> non_null_reference = env().NewStringUtf("string2");
117108

118109
EXPECT_THAT(null_reference, Not(JavaEq(non_null_reference)));
119110
EXPECT_THAT(non_null_reference, Not(JavaEq(null_reference)));
120111
}
121112

122113
TEST_F(FirestoreAndroidIntegrationTest,
123114
JavaEqShouldReturnFalseForObjectOfDifferentTypes) {
124-
Env env;
125-
Local<String> string_object = env.NewStringUtf("string2");
126-
Local<ArrayList> list_object = ArrayList::Create(env);
115+
Local<String> string_object = env().NewStringUtf("string2");
116+
Local<ArrayList> list_object = ArrayList::Create(env());
127117

128118
EXPECT_THAT(string_object, Not(JavaEq(list_object)));
129119
EXPECT_THAT(list_object, Not(JavaEq(string_object)));
130120
}
131121

132122
TEST_F(FirestoreAndroidIntegrationTest,
133123
RefersToSameJavaObjectAsShouldReturnTrueForSameObjects) {
134-
Env env;
135-
Local<String> object1 = env.NewStringUtf("string");
124+
Local<String> object1 = env().NewStringUtf("string");
136125
Global<String> object2 = object1;
137126

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

149138
TEST_F(FirestoreAndroidIntegrationTest,
150139
RefersToSameJavaObjectAsShouldReturnFalseForDistinctObjects) {
151-
Env env;
152-
Local<String> object1 = env.NewStringUtf("test string");
153-
Local<String> object2 = env.NewStringUtf("test string");
154-
ASSERT_FALSE(env.IsSameObject(object1, object2));
140+
Local<String> object1 = env().NewStringUtf("test string");
141+
Local<String> object2 = env().NewStringUtf("test string");
142+
ASSERT_FALSE(env().IsSameObject(object1, object2));
155143

156144
EXPECT_THAT(object1, Not(RefersToSameJavaObjectAs(object2)));
157145
}
158146

159147
TEST_F(FirestoreAndroidIntegrationTest,
160148
RefersToSameJavaObjectAsShouldReturnFalseIfExactlyOneObjectIsNull) {
161-
Env env;
162149
Local<String> null_reference;
163-
Local<String> non_null_reference = env.NewStringUtf("string2");
150+
Local<String> non_null_reference = env().NewStringUtf("string2");
164151

165152
EXPECT_THAT(null_reference,
166153
Not(RefersToSameJavaObjectAs(non_null_reference)));
@@ -170,42 +157,38 @@ TEST_F(FirestoreAndroidIntegrationTest,
170157

171158
TEST_F(FirestoreAndroidIntegrationTest,
172159
ThrowExceptionWithNoMessageShouldSetPendingExceptionWithAMessage) {
173-
Env env;
174-
Local<Throwable> throw_exception_return_value = ThrowException(env);
175-
Local<Throwable> actually_thrown_exception = env.ClearExceptionOccurred();
160+
Local<Throwable> throw_exception_return_value = ThrowException();
161+
Local<Throwable> actually_thrown_exception = env().ClearExceptionOccurred();
176162
ASSERT_TRUE(actually_thrown_exception);
177163
EXPECT_THAT(actually_thrown_exception,
178164
RefersToSameJavaObjectAs(throw_exception_return_value));
179-
EXPECT_THAT(actually_thrown_exception.GetMessage(env), Not(IsEmpty()));
165+
EXPECT_THAT(actually_thrown_exception.GetMessage(env()), Not(IsEmpty()));
180166
}
181167

182168
TEST_F(FirestoreAndroidIntegrationTest,
183169
ThrowExceptionWithAMessageShouldSetPendingExceptionWithTheGivenMessage) {
184-
Env env;
185170
Local<Throwable> throw_exception_return_value =
186-
ThrowException(env, "my test message");
187-
Local<Throwable> actually_thrown_exception = env.ClearExceptionOccurred();
171+
ThrowException("my test message");
172+
Local<Throwable> actually_thrown_exception = env().ClearExceptionOccurred();
188173
ASSERT_TRUE(actually_thrown_exception);
189174
EXPECT_THAT(actually_thrown_exception,
190175
RefersToSameJavaObjectAs(throw_exception_return_value));
191-
EXPECT_THAT(actually_thrown_exception.GetMessage(env),
176+
EXPECT_THAT(actually_thrown_exception.GetMessage(env()),
192177
StrEq("my test message"));
193178
}
194179

195180
TEST_F(FirestoreAndroidIntegrationTest,
196181
CreateExceptionWithNoMessageShouldReturnAnExceptionWithAMessage) {
197-
Env env;
198-
Local<Throwable> exception = CreateException(env);
182+
Local<Throwable> exception = CreateException();
199183
ASSERT_TRUE(exception);
200-
EXPECT_THAT(exception.GetMessage(env), Not(IsEmpty()));
184+
EXPECT_THAT(exception.GetMessage(env()), Not(IsEmpty()));
201185
}
202186

203187
TEST_F(FirestoreAndroidIntegrationTest,
204188
CreateExceptionWithAMessageShouldReturnAnExceptionWithTheGivenMessage) {
205-
Env env;
206-
Local<Throwable> exception = CreateException(env, "my test message");
189+
Local<Throwable> exception = CreateException("my test message");
207190
ASSERT_TRUE(exception);
208-
EXPECT_THAT(exception.GetMessage(env), StrEq("my test message"));
191+
EXPECT_THAT(exception.GetMessage(env()), StrEq("my test message"));
209192
}
210193

211194
} // namespace

‎firestore/integration_test_internal/src/android/geo_point_android_test.cc‎

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,20 @@
1616

1717
#include "firestore/src/android/geo_point_android.h"
1818

19+
#include "android/firestore_integration_test_android.h"
1920
#include "firebase/firestore/geo_point.h"
20-
#include "firestore/src/jni/env.h"
21-
#include "firestore_integration_test.h"
2221
#include "gtest/gtest.h"
2322

2423
namespace firebase {
2524
namespace firestore {
2625
namespace {
2726

28-
using jni::Env;
29-
30-
using GeoPointTest = FirestoreIntegrationTest;
27+
using GeoPointTest = FirestoreAndroidIntegrationTest;
3128

3229
TEST_F(GeoPointTest, Converts) {
33-
Env env;
34-
3530
GeoPoint point{12.0, 34.0};
36-
auto java_point = GeoPointInternal::Create(env, point);
37-
EXPECT_EQ(point, java_point.ToPublic(env));
31+
auto java_point = GeoPointInternal::Create(env(), point);
32+
EXPECT_EQ(point, java_point.ToPublic(env()));
3833
}
3934

4035
} // namespace

0 commit comments

Comments
 (0)