Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion firestore/integration_test_internal/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
-->
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.google.firebase.cpp.database.testapp"
package="com.google.firebase.cpp.firestore.testapp"
android:versionCode="1"
android:versionName="1.0">
<uses-permission android:name="android.permission.INTERNET" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,32 +20,35 @@ namespace {
// non-default app to avoid data ending up in the cache before tests run.
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. If USE_FIRESTORE_EMULATOR is unset, empty, or 0 then use prod; otherwise, use the emulator.
  2. If FIRESTORE_EMULATOR_PORT is unset or empty, then use port 8080; otherwise, use the port specified in the environment variable.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  2. Done.

void LocateEmulator(Firestore* db) {
// iOS and Android pass emulator address differently, iOS writes it to a
// temp file, but there is no equivalent to `/tmp/` for Android, so it
// uses an environment variable instead.
// TODO(wuandy): See if we can use environment variable for iOS as well?
std::ifstream ifs("/tmp/emulator_address");
std::stringstream buffer;
buffer << ifs.rdbuf();
std::string address;
if (ifs.good()) {
address = buffer.str();
} else if (std::getenv("FIRESTORE_EMULATOR_HOST")) {
address = std::getenv("FIRESTORE_EMULATOR_HOST");
// Use emulator as long as this env variable is set, regardless its value.
if (std::getenv("USE_FIRESTORE_EMULATOR") == nullptr) {
LogDebug("Using Firestore Prod for testing.");
return;
}

#if !defined(__ANDROID__)
absl::StripAsciiWhitespace(&address);
#endif // !defined(__ANDROID__)
if (!address.empty()) {
auto settings = db->settings();
settings.set_host(address);
// Emulator does not support ssl yet.
settings.set_ssl_enabled(false);
db->set_settings(settings);
}
#if defined(__ANDROID__)
// Special IP to access the hosting OS from Android Emulator.
std::string local_host = "10.0.2.2";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

#else
std::string local_host = "localhost";
#endif // defined(__ANDROID__)

// Use FIRESTORE_EMULATOR_PORT if it is set to non empty string,
// otherwise use the default port.
std::string port = std::getenv("FIRESTORE_EMULATOR_PORT")
? std::getenv("FIRESTORE_EMULATOR_PORT")
: "8080";
std::string address =
port.empty() ? (local_host + ":8080") : (local_host + ":" + port);

LogInfo("Using Firestore Emulator (%s) for testing.", address.c_str());
auto settings = db->settings();
settings.set_host(address);
// Emulator does not support ssl yet.
settings.set_ssl_enabled(false);
db->set_settings(settings);
}

} // namespace
Expand Down
39 changes: 39 additions & 0 deletions testing/sample_framework/src/android/android_app_framework.cc
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,43 @@ std::string ReadTextInput(const char* title, const char* message,
return g_text_entry_field_data->ReadText(title, message, placeholder);
}

void SetEnvironmentVariableFromStringExtra(JNIEnv* env, const char* extra_name,
jobject intent) {
jclass intent_class = env->GetObjectClass(intent);
jmethodID get_string_extra = env->GetMethodID(
intent_class, "getStringExtra", "(Ljava/lang/String;)Ljava/lang/String;");
env->DeleteLocalRef(intent_class);

jstring extra_name_jstring = env->NewStringUTF(extra_name);
jstring extra_value_jstring = (jstring)env->CallObjectMethod(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

intent, get_string_extra, extra_name_jstring);
env->DeleteLocalRef(extra_name_jstring);

if (extra_value_jstring != nullptr) {
const char* extra_value =
env->GetStringUTFChars(extra_value_jstring, nullptr);
setenv(extra_name, extra_value, /*overwrite=*/1);
env->ReleaseStringUTFChars(extra_value_jstring, extra_value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

env->DeleteLocalRef(extra_value_jstring);
}
}

void SetExtrasAsEnvironmentVariables() {
JNIEnv* env = app_framework::GetJniEnv();
jobject activity = app_framework::GetActivity();

jclass activity_class = env->GetObjectClass(activity);
jmethodID get_intent = env->GetMethodID(activity_class, "getIntent",
"()Landroid/content/Intent;");
env->DeleteLocalRef(activity_class);

jobject intent = env->CallObjectMethod(activity, get_intent);
SetEnvironmentVariableFromStringExtra(env, "USE_FIRESTORE_EMULATOR", intent);
SetEnvironmentVariableFromStringExtra(env, "FIRESTORE_EMULATOR_PORT", intent);

env->DeleteLocalRef(intent);
}

} // namespace app_framework

// Execute common_main(), flush pending events and finish the activity.
Expand Down Expand Up @@ -520,6 +557,8 @@ extern "C" void android_main(struct android_app* state) {
pthread_create(&thread, nullptr, app_framework::stdout_logger,
reinterpret_cast<void*>(filedes));

app_framework::SetExtrasAsEnvironmentVariables();

// Execute cross platform entry point.
// Copy the app name into a non-const array, as googletest requires that
// main() take non-const char* argv[] so it can modify the arguments.
Expand Down