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.

Sign up for GitHub

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

feat: plumb SDK test data branch through to URL used for test config #137

Merged
merged 4 commits into from
Nov 28, 2024
Merged

feat: plumb SDK test data branch through to URL used for test config #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1c6a6b
feat: plumb SDK test data branch through to URL used for test config
typotter Nov 26, 2024
d059abd
clean up temp
typotter Nov 26, 2024
277da84
one long line
typotter Nov 26, 2024
6d45d17
Merge branch 'main' into tp/testdatabranch
typotter Nov 28, 2024
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: 7 additions & 7 deletions .github/workflows/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ on:
required: false

env:
SDK_BRANCH_NAME: ${{ inputs.sdk_branch || github.head_ref || github.ref_name }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaning up names for consistency with other repos

TEST_DATA_BRANCH_NAME: ${{ inputs.test_data_branch || 'main' }}
SDK_BRANCH: ${{ inputs.sdk_branch || github.head_ref || github.ref_name }}
TEST_DATA_BRANCH: ${{ inputs.test_data_branch || 'main' }}

ORG_GRADLE_PROJECT_ossrhUsername: ${{ secrets.OSSRH_USERNAME }}
ORG_GRADLE_PROJECT_ossrhPassword: ${{ secrets.OSSRH_PASSWORD }}
Expand All @@ -43,15 +43,15 @@ jobs:
- name: Display Testing Details
run: |
echo "Running SDK Test using"
echo "Test Data: sdk-test-data@${TEST_DATA_BRANCH_NAME}"
echo "SDK Branch: android-sdk@${SDK_BRANCH_NAME}"
echo "Test Data: sdk-test-data@${TEST_DATA_BRANCH}"
echo "SDK Branch: android-sdk@${SDK_BRANCH}"
echo "API Level: ${{ matrix.api-level }}"

- name: Check out Java SDK
uses: actions/checkout@v4
with:
repository: Eppo-exp/android-sdk
ref: ${{ env.SDK_BRANCH_NAME}}
ref: ${{ env.SDK_BRANCH}}

- name: Set up JDK 17
uses: actions/setup-java@v3
Expand All @@ -66,7 +66,7 @@ jobs:
echo "GRADLE_USER_HOME=${HOME}/.gradle" >> $GITHUB_ENV

- name: Set up test data
run: make test-data branchName=${{env.TEST_DATA_BRANCH_NAME}}
run: make test-data branchName=${{env.TEST_DATA_BRANCH}}

- name: Wait for mock UFC DNS to resolve
run: |
Expand Down Expand Up @@ -131,7 +131,7 @@ jobs:
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
./gradlew connectedCheck --no-daemon # run tests
./gradlew connectedCheck --no-daemon -Pandroid.testInstrumentationRunnerArguments.TEST_DATA_BRANCH=${{ env.TEST_DATA_BRANCH }} # run tests

- name: Upload Emulator Logs
if: always()
Expand Down
12 changes: 10 additions & 2 deletions eppo/src/androidTest/java/cloud/eppo/android/EppoClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import android.util.Log;
import androidx.annotation.Nullable;
import androidx.test.core.app.ApplicationProvider;
import androidx.test.platform.app.InstrumentationRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

import cloud.eppo.BaseEppoClient;
import cloud.eppo.EppoHttpClient;
import cloud.eppo.android.cache.LRUAssignmentCache;
Expand Down Expand Up @@ -68,8 +69,15 @@ public class EppoClientTest {
private static final String TAG = logTag(EppoClient.class);
private static final String DUMMY_API_KEY = "mock-api-key";
private static final String DUMMY_OTHER_API_KEY = "another-mock-api-key";
private static final String TEST_HOST =

// Use branch if specified by env variable `TEST_DATA_BRANCH`.
private static final String TEST_BRANCH =
InstrumentationRegistry.getArguments().getString("TEST_DATA_BRANCH");
Comment on lines +74 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool did not know about InstrumentRegistry

private static final String TEST_HOST_BASE =
"https://us-central1-eppo-qa.cloudfunctions.net/serveGitHubRacTestFile";
private static final String TEST_HOST =
TEST_HOST_BASE + (TEST_BRANCH != null ? "/b/" + TEST_BRANCH : "");

private static final String INVALID_HOST = "https://thisisabaddomainforthistest.com";
private final ObjectMapper mapper = new ObjectMapper().registerModule(module());
@Mock AssignmentLogger mockAssignmentLogger;
Expand Down Expand Up @@ -781,7 +789,7 @@ private void waitForPopulatedCache() {
while (!cachePopulated) {
if (System.currentTimeMillis() > waitEnd) {
throw new InterruptedException(
"Cache file never populated; assuming configuration error");
"Cache file never populated or smaller than expected 8000 bytes; assuming configuration error");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wasted a bunch of time trying to figure this out (my test data branch had a very small config).

Copy link
Member

Choose a reason for hiding this comment

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

where is this "expected 8000 bytes" coming from? first time I'm hearing about it I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a check below that checks for a minimum cache file size of 8k

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

line 794/995 cc: @felipecsl

}
long expectedMinimumSizeInBytes =
8000; // Last time this test was updated, cache size was 11,506 bytes
Expand Down
Loading