Make it possible to run firestore tests against emulator #488

wu-hui · 2021-06-23T16:42:37Z

Android: via extra adb shell start ... -e USE_FIRESTORE_EMULATOR true.

Desktop/iOS: set env var USE_FIRESTORE_EMULATOR.

github-actions · 2021-06-23T16:43:29Z

❌ Integration test FAILED

Requested by @wu-hui on commit dea10eb
Last updated: Wed Jun 30 20:12 PDT 2021
View integration test log & download artifacts

Failures	Configs
firestore	[TEST] [FAILURE] [iOS] [macos] [ios_target] (1 failed tests) LoadInvalidBundlesShouldFail

alexames · 2021-06-23T21:27:48Z

testing/sample_framework/src/android/android_app_framework.cc

+void SetEnvironmentVariableFromExtra(const char* extra_name, JNIEnv* env,
+ jobject intent,
+ jmethodID get_string_extra) {
+ jstring extra_value_jstring = (jstring)env->CallObjectMethod(


I forget the jni rules for object ownership lifetimes. I see that you called ReleaseStringUTFChars below, but do you also have to call DeleteLocalRef on this as well?

alexames · 2021-06-23T21:28:49Z

testing/sample_framework/src/android/android_app_framework.cc

+ jclass activity_clazz = env->GetObjectClass(activity);
+ jmethodID get_intent = env->GetMethodID(activity_clazz, "getIntent",
+ "()Landroid/content/Intent;");
+ jobject intent = env->CallObjectMethod(activity, get_intent);


Same here, I think you should be calling DeleteLocalRef at the end of the function.

dconeybe · 2021-06-24T14:42:13Z

firestore/integration_test_internal/src/firestore_integration_test.cc

- address = std::getenv("FIRESTORE_EMULATOR_HOST");
+ // Use prod backend as long as this env variable is unset.
+ if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) {
+ LogDebug("Using prod backend for testing...");


nit: The "..." is not really meaningful here. A single period is all that is needed. Also on line 45.

dconeybe · 2021-06-24T14:42:59Z

firestore/integration_test_internal/src/firestore_integration_test.cc

-#endif // !defined(__ANDROID__)
- if (!address.empty()) {
+#if defined(__ANDROID__)
+ std::string local_host = "10.0.2.2";


Where does "10.0.2.2" come from? Could you add an inline comment to document this magic value and/or provide a link to where this number is documented?

dconeybe · 2021-06-24T14:46:25Z

firestore/integration_test_internal/src/firestore_integration_test.cc

+ std::string local_host = "localhost";
+#endif // defined(__ANDROID__)
+
+ std::string port = "8080";


nit: Avoid assign-and-mutate for local variables, port in this case. Instead, assign to the variable exactly once in any given code flow. This improves readability by reducing the cognitive energy required to understand the code and is therefore also less error-prone. This advice comes from Python, but is equally applicable to C++: go/python-tips/026.

dconeybe · 2021-06-24T14:53:03Z

firestore/integration_test_internal/src/firestore_integration_test.cc

 static const char* kBootstrapAppName = "bootstrap";

-// Set Firestore up to use Firestore Emulator if it can be found.
+// Set Firestore up to use Firestore Emulator via USE_FIRESTORE_EMULATOR


It appears that based on the logic in this method, as long as USE_FIRESTORE_EMULATOR is defined, to any value (even 0 or empty), then the emulator will be used, unless FIRESTORE_EMULATOR_PORT is set to the empty string. This logic seems a bit complex.

How do you feel about coding it like this:

If USE_FIRESTORE_EMULATOR is unset, empty, or 0 then use prod; otherwise, use the emulator.

If FIRESTORE_EMULATOR_PORT is unset or empty, then use port 8080; otherwise, use the port specified in the environment variable.

It can also be set to false, no, etc. I think it is reasonable to not stress too much on this.

Done.

dconeybe · 2021-06-24T14:54:09Z

firestore/integration_test_internal/src/firestore_integration_test.cc

+
+ if (!port.empty()) {
+ std::string address = local_host + ":" + port;
+ std::string message = "Using emulator (" + address + ") for testing...";


Instead of creating the temporary variable message, how about just use the string formatting built into LogDebug.

e.g. LogDebug("Using emulator (%s) for testing.", address.c_str());

dconeybe · 2021-06-24T14:55:30Z

testing/sample_framework/src/android/android_app_framework.cc

 return g_text_entry_field_data->ReadText(title, message, placeholder);
 }

+void SetEnvironmentVariableFromExtra(const char* extra_name, JNIEnv* env,


nit: It is conventional for the JNIEnv pointer to be the first argument; please re-order the env argument to be the first argument.

Consider renaming SetEnvironmentVariableFromExtra to SetEnvironmentVariableFromStringExtra, to clearly identify that it is only concerned with string extras (not int extras, long extras, etc).

dconeybe · 2021-06-24T15:00:08Z

testing/sample_framework/src/android/android_app_framework.cc

+ jobject intent,
+ jmethodID get_string_extra) {
+ jstring extra_value_jstring = (jstring)env->CallObjectMethod(
+ intent, get_string_extra, env->NewStringUTF(extra_name));


You should call DeleteLocalRef on the jstring returned from env->NewStringUTF(extra_name).

dconeybe · 2021-06-24T15:02:55Z

testing/sample_framework/src/android/android_app_framework.cc

+
+void SetExtrasAsEnvironmentVariables() {
+ JNIEnv* env = app_framework::GetJniEnv();
+ jobject activity = g_app_state->activity->clazz;


I think that g_app_state->activity->clazz can be replaced by app_framework::GetActivity().

dconeybe · 2021-06-24T15:03:59Z

testing/sample_framework/src/android/android_app_framework.cc

+ JNIEnv* env = app_framework::GetJniEnv();
+ jobject activity = g_app_state->activity->clazz;
+
+ jclass activity_clazz = env->GetObjectClass(activity);


You should call DeleteLocalRef(activity_clazz) once you're done with activity_clazz.

Please rename activity_clazz to activity_class, and intent_clazz to intent_class below. The "ss" -> "zz" is only needed because class is a reserved word, unlike activity_class and intent_class.

dconeybe · 2021-06-24T15:05:10Z

testing/sample_framework/src/android/android_app_framework.cc

+ "()Landroid/content/Intent;");
+ jobject intent = env->CallObjectMethod(activity, get_intent);
+
+ jclass intent_clazz = env->GetObjectClass(intent); // class pointer of Intent


Please remove the comment "// class pointer of Intent"; it is explaining what the code is clearly doing. Moreover, above online 502 you do the same thing without commenting about it.

You should call DeleteLocalRef(intent_clazz) once you're done with intent_clazz.

IMO This logic to get the jmethodID for getStringExtra() belongs in SetEnvironmentVariableFromExtra(), not here. It appears that you're trying to avoid retrieving the jmethodID twice, but the costs of this is inconsequential and leaks implementation details of SetEnvironmentVariableFromExtra() to its callers.

dconeybe

LGTM with a few minor nits. Thanks for doing this!

dconeybe · 2021-06-24T19:37:47Z

testing/sample_framework/src/android/android_app_framework.cc

 #include <cstdlib>
 #include <cstring>
 #include <ctime>
+#include <iostream>


Do you still need #include <iostream>?

dconeybe · 2021-06-24T19:41:10Z

testing/sample_framework/src/android/android_app_framework.cc

+ env->GetStringUTFChars(extra_value_jstring, nullptr);
+ setenv(extra_name, extra_value == nullptr ? "" : extra_value,
+ /*overwrite=*/1);
+ env->ReleaseStringUTFChars(extra_value_jstring, extra_value);


Since extra_value is potentially nullptr, I think you should conditionally invoke ReleaseStringUTFChars() only if it is not null. Alternately, just assume that GetStringUTFChars() returns non-null and get rid of the extra_value == nullptr check above. It would be some incredibly rare situation where it would return null and it would indicate a much larger problem anyways.

* Desktop and Android works with env variable and extra * Format and remove GTEST_FILTER for now * Add deletelocalref * Feedback. * More tweaks.

wu-hui added 2 commits June 22, 2021 20:50

Desktop and Android works with env variable and extra

4fb801a

Format and remove GTEST_FILTER for now

5c5f986

google-cla bot added the cla: yes label Jun 23, 2021

wu-hui added the tests-requested: quick Trigger a quick set of integration tests. label Jun 23, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 23, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Jun 23, 2021

wu-hui requested review from alexames and dconeybe June 23, 2021 18:15

wu-hui assigned alexames and dconeybe Jun 23, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 23, 2021

alexames suggested changes Jun 23, 2021

View reviewed changes

Add deletelocalref

9fd5115

wu-hui added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 24, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 24, 2021

dconeybe requested changes Jun 24, 2021

View reviewed changes

dconeybe assigned wu-hui and unassigned dconeybe Jun 24, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Jun 24, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 24, 2021

Feedback.

fd8ac84

wu-hui assigned dconeybe and unassigned wu-hui Jun 24, 2021

dconeybe previously approved these changes Jun 24, 2021

View reviewed changes

dconeybe assigned wu-hui and unassigned dconeybe Jun 24, 2021

github-actions bot added the tests: failed This PR's integration tests failed. label Jun 26, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 26, 2021

alexames approved these changes Jun 29, 2021

View reviewed changes

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: failed This PR's integration tests failed. labels Jun 29, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 29, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: failed This PR's integration tests failed. labels Jun 29, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 29, 2021

wu-hui enabled auto-merge (squash) June 30, 2021 13:19

wu-hui added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jun 30, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 30, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 30, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: failed This PR's integration tests failed. labels Jun 30, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 30, 2021

wu-hui removed the tests: failed This PR's integration tests failed. label Jul 1, 2021

wu-hui merged commit dea10eb into main Jul 1, 2021

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. labels Jul 1, 2021

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 1, 2021

jonsimantov pushed a commit that referenced this pull request Jul 1, 2021

Make it possible to run firestore tests against emulator (#488)

6e01cb9

* Desktop and Android works with env variable and extra * Format and remove GTEST_FILTER for now * Add deletelocalref * Feedback. * More tweaks.

jonsimantov deleted the wuandy/FirestoreEmulator branch July 12, 2021 22:49

firebase locked and limited conversation to collaborators Jul 31, 2021

Make it possible to run firestore tests against emulator #488

Make it possible to run firestore tests against emulator #488

Uh oh!

Conversation

wu-hui commented Jun 23, 2021

github-actions bot commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dconeybe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

3 participants

github-actions bot commented Jun 23, 2021 •

edited

Loading