Skip to content
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.

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

Include additional error information in emulator log #34

Merged
merged 7 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ jobs:
- name: Set up test data
run: make test-data

- name: Wait for mock RAC DNS to resolve
run: |
for i in {1..15}
do
ip=$(dig +short us-central1-eppo-prod-312905.cloudfunctions.net)
if [[ -n "$ip" ]]; then
echo "Domain resolved to $ip after $i seconds"
exit 0
fi
sleep 1
done
echo "Failed to resolve after 15 seconds"
exit 1

- name: Spin up emulator and run tests
id: testing
uses: ReactiveCircus/android-emulator-runner@v2
Expand Down
23 changes: 10 additions & 13 deletions eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,8 @@
import static org.junit.Assert.assertTrue;

import static cloud.eppo.android.ConfigCacheFile.CACHE_FILE_NAME;
import static cloud.eppo.android.util.Utils.logTag;

import android.content.res.AssetManager;
import android.util.Log;

import androidx.test.core.app.ApplicationProvider;

Expand Down Expand Up @@ -45,9 +43,8 @@
import cloud.eppo.android.dto.adapters.EppoValueAdapter;

public class EppoClientTest {
private static final String TAG = logTag(EppoClientTest.class);
private static final String TEST_HOST = "http://us-central1-eppo-prod-312905.cloudfunctions.net/serveGithubRacTestFile";
private static final String INVALID_HOST = "http://thisisabaddomainforthistest.com";
private static final String TEST_HOST = "https://us-central1-eppo-prod-312905.cloudfunctions.net/serveGithubRacTestFile";
private static final String INVALID_HOST = "https://thisisabaddomainforthistest.com";
Comment on lines +46 to +47
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made these HTTPS

private Gson gson = new GsonBuilder()
.registerTypeAdapter(EppoValue.class, new EppoValueAdapter())
.registerTypeAdapter(AssignmentValueType.class, new AssignmentValueTypeAdapter(AssignmentValueType.STRING))
Expand Down Expand Up @@ -150,14 +147,16 @@ public void onCompleted() {
@Override
public void onError(String errorMessage) {
if (throwOnCallackError) {
throw new RuntimeException("Unable to initialize");
throw new RuntimeException("Unable to initialize: "+errorMessage);
}
lock.countDown();
}
})
.buildAndInit();

lock.await(2000, TimeUnit.MILLISECONDS);
if(!lock.await(10000, TimeUnit.MILLISECONDS)) {
throw new RuntimeException("Request for RAC did not complete within timeout");
}
Comment on lines +157 to +159
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core change. If CountDownLatch.await() hits it's timeout, it returns false but then continues execution. This can be confusing to the developer, so now we'll throw an error.

First testing this on my bad home wifi, I hit the error 😅 . So, I've extended the timeout to a generous 10 seconds. However, since it's using this await, most of the time we won't actually wait that long.

}

@After
Expand All @@ -179,8 +178,6 @@ public void testAssignments() {
public void testErrorGracefulModeOn() {
try {
initClient(TEST_HOST, false, true, true);
System.out.println("Sleeping for a bit to wait for cache population to complete");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't need these sleeps since we are no formally waiting for the request to complete.

Thread.sleep(1000);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
Expand Down Expand Up @@ -214,8 +211,6 @@ public void testErrorGracefulModeOn() {
public void testErrorGracefulModeOff() {
try {
initClient(TEST_HOST, false, true, false);
Log.d(TAG, "Sleeping for a bit to wait for cache population to complete");
Thread.sleep(1000);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
Expand Down Expand Up @@ -262,12 +257,14 @@ private void runTestCases() {
@Test
public void testCachedAssignments() {
try {
// First initialize successfully
initClient(TEST_HOST, false, true, false); // ensure cache is populated

// wait for a bit since file is loaded asynchronously
// wait for a bit since cache file is loaded asynchronously
System.out.println("Sleeping for a bit to wait for cache population to complete");
Thread.sleep(1000);
Thread.sleep(2000);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do still need this sleep though, for asynchronous cache repopulation, but I'm doubling it given how unimpressed I am with GitHub action performance.


// Then reinitialize with a bad host so we know it's using the cached RAC built from the first initialization
initClient(INVALID_HOST, false, false, false); // invalid port to force to use cache
} catch (InterruptedException e) {
throw new RuntimeException(e);
Expand Down
29 changes: 20 additions & 9 deletions eppo/src/main/java/cloud/eppo/android/EppoHttpClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,21 @@

import android.util.Log;

import org.jetbrains.annotations.NotNull;

import java.io.IOException;
import java.io.Reader;
import java.net.HttpURLConnection;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.TimeUnit;

import okhttp3.Call;
import okhttp3.Callback;
import okhttp3.Dns;
import okhttp3.HttpUrl;
import okhttp3.OkHttpClient;
import okhttp3.Request;
Expand All @@ -19,17 +27,23 @@
public class EppoHttpClient {
private static final String TAG = logTag(EppoHttpClient.class);

private final OkHttpClient client = new OkHttpClient().newBuilder()
.connectTimeout(10, TimeUnit.SECONDS)
.readTimeout(10, TimeUnit.SECONDS)
.build();
private final OkHttpClient client;

private final String baseUrl;
private final String apiKey;

public EppoHttpClient(String baseUrl, String apiKey) {
this.baseUrl = baseUrl;
this.apiKey = apiKey;
this.client = buildOkHttpClient();
}

private static OkHttpClient buildOkHttpClient() {
OkHttpClient.Builder builder = new OkHttpClient().newBuilder()
.connectTimeout(10, TimeUnit.SECONDS)
.readTimeout(10, TimeUnit.SECONDS);

return builder.build();
Comment on lines +38 to +46
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this more obvious it was happening in constructor and set it up for dropping in IpV4-only DNS, which may be needed (but hopefully not 🤞)

}

public void get(String path, RequestCallback callback) {
Expand All @@ -45,7 +59,7 @@ public void get(String path, RequestCallback callback) {
@Override
public void onResponse(Call call, Response response) {
if (response.isSuccessful()) {
Log.d(TAG, "Fetch successfull");
Log.d(TAG, "Fetch successful");
callback.onSuccess(response.body().charStream());
} else {
switch (response.code()) {
Expand All @@ -64,10 +78,7 @@ public void onResponse(Call call, Response response) {

@Override
public void onFailure(Call call, IOException e) {
if (BuildConfig.DEBUG) {
e.printStackTrace();
}
Log.e(TAG, "Http request failure", e);
Log.e(TAG, "Http request failure: "+e.getMessage()+" "+Arrays.toString(e.getStackTrace()), e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More helpful failure log.

callback.onFailure("Unable to fetch from URL "+httpUrl);
}
});
Expand Down
Loading