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

Get CI/CD Working #33

Closed
wants to merge 31 commits into from
Closed

Get CI/CD Working #33

wants to merge 31 commits into from

Conversation

aarsilv
Copy link
Contributor

@aarsilv aarsilv commented Oct 23, 2023

Fixes: FF-1121 - Get Android SDK CI/CD Working

This gets automated tests for this SDK working by three main changes:

  1. Creating a GitHub action, test.yml, that spins up an emulator with internet access to run the tests
  2. Leveraging a cloud function to serve as a mock RAC server.
  3. Addressing a caching race condition with our client in our unit tests; adjusting instantiation and waiting to be sure caches are deleted and hydrated when we expect

To support the development of the above--and future development--some additional log statements were added and a common log tag prefix was established that we can use to upload filtered logs as an artifact of the run.

Comment on lines +33 to +34
MAVEN_USERNAME: ${{ secrets.MAVEN_USERNAME }}
MAVEN_PASSWORD: ${{ secrets.MAVEN_PASSWORD }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's ok these aren't populated in this repo, they just need to be defined (even if empty string)

api-level: 33
target: google_apis
arch: x86_64
emulator-options: -no-window -gpu swiftshader_indirect -no-snapshot -noaudio -no-boot-anim -dns-server 8.8.8.8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

8.8.8.8 is Google's DNS server.

A bug in Android resulted in newer images not having valid DNS, and thus no internet, so we need to specify this.

mkdir -p app/ # create directory
touch app/emulator.log # create log file
chmod 777 app/emulator.log # allow writing to log file
adb logcat | grep EppoSDK >> app/emulator.log & # pipe all logcat messages into log file as a background process
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 use grep so we get all the logs from our SDK, regardless of the specific class logging.

@@ -63,17 +63,16 @@ ext.versions = [
"androidx_runner": "1.5.2",
"gson": "2.9.1",
"okhttp": "4.10.0",
"wiremock": "2.34.0"
"commonsio": "2.14.0"
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 were using some classes, in apache commons, that came with Wiremock outside of our mock server.

@@ -63,17 +63,16 @@ ext.versions = [
"androidx_runner": "1.5.2",
"gson": "2.9.1",
"okhttp": "4.10.0",
"wiremock": "2.34.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🪓

@@ -4,5 +4,6 @@
<application android:usesCleartextTraffic="true" />
<uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure this is not needed but figure I'd keep it for paranoia, especially in case we start testing old versions of the SDK or something.

private static final String HOST = "http://localhost:" + TEST_PORT;
private static final String INVALID_HOST = "http://localhost:" + (TEST_PORT + 1);
private WireMockServer mockServer;
private static final String TEST_HOST = "http://us-central1-eppo-prod-312905.cloudfunctions.net/serveGithubRacTestFile";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this is some nice DNS thing but not needed

@@ -154,34 +149,18 @@ public void onError(String errorMessage) {
lock.await(2000, TimeUnit.MILLISECONDS);
}

@Before
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting one up once before all tests just caused issues.

Comment on lines -206 to +190
initClient(INVALID_HOST, false, false); // invalid port to force to use cache
initClient(TEST_HOST, false, true); // ensure cache is populated

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

initClient(INVALID_HOST, false, false); // invalid port to force to use cache
Copy link
Member

Choose a reason for hiding this comment

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

HUGE

Comment on lines 14 to 15
env:
ANDROID_API_LEVEL: 33
Copy link
Member

Choose a reason for hiding this comment

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

I learned that this env is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will axe! 🪓

@aarsilv aarsilv marked this pull request as ready for review October 26, 2023 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants