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

fix: Adds check for .aab format before querying apkDetails #2189

Merged
merged 3 commits into from
Apr 7, 2022

Conversation

Xlopec
Copy link
Contributor

@Xlopec Xlopec commented Nov 30, 2021

Fixes #2188

This PR adds checks for packaging format before querying for APK details. It fixes the following issue #2188

Checklist

  • [] Documented
  • Unit tested
  • [] Integration tests updated

@github-actions
Copy link
Contributor

github-actions bot commented Nov 30, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Xlopec Xlopec force-pushed the fix_aab_bad_request branch from 8da6fa8 to d627471 Compare November 30, 2021 12:26
Copy link
Contributor

@bootstraponline bootstraponline left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a fix for this issue! I've left some comments.

@@ -50,7 +40,10 @@ internal suspend fun AndroidArgs.runAndroidTests(): TestResult = coroutineScope
ignoredTestsShardChunks += context.ignoredTestCases
allTestShardChunks += context.shards
}
context.reportPackageName()

if (args.appApk?.endsWith(".aab") != true) {
Copy link
Contributor

@bootstraponline bootstraponline Nov 30, 2021

Choose a reason for hiding this comment

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

I think we should only report if it ends with .apk as google may add additional extensions in the future

we should probably also call toLowerCase and trim to avoid whitespace.

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, what about null case, should we reportPackageName in this case? I guess we will get FileReference.remote == "" in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about this approach?

If the app path doesn't end in .apk, we'll report an empty string. App path can't be null.

diff --git a/test_runner/src/main/kotlin/ftl/client/google/AppDetails.kt b/test_runner/src/main/kotlin/ftl/client/google/AppDetails.kt
index 0a806b70..118b74cd 100644
--- a/test_runner/src/main/kotlin/ftl/client/google/AppDetails.kt
+++ b/test_runner/src/main/kotlin/ftl/client/google/AppDetails.kt
@@ -3,10 +3,14 @@ package ftl.client.google
 import com.google.testing.model.FileReference
 import ftl.http.executeWithRetry

-fun getAndroidAppDetails(gcsAppPath: String): String =
-    GcTesting.get
+fun getAndroidAppDetails(gcsAppPath: String): String {
+    // getApkDetails errors when sent non-apk files such as aab
+    if (gcsAppPath.trim().lowercase().endsWith(".apk").not()) return ""
+
+    return GcTesting.get
         .ApplicationDetailService()
         .getApkDetails(FileReference().apply { gcsPath = gcsAppPath })
         .apply { requestHeaders.set("X-Server-Timeout", 1800) } // 30 min
         .executeWithRetry()
         ?.apkDetail?.apkManifest?.packageName?.toString().orEmpty()
+}

Copy link
Contributor Author

@Xlopec Xlopec Dec 3, 2021

Choose a reason for hiding this comment

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

Looks good to me, pushed updated code

test_runner/src/test/kotlin/ftl/args/AndroidArgsTest.kt Outdated Show resolved Hide resolved
@@ -1717,6 +1715,34 @@ AndroidArgs
}
}

@Test
fun `should send no package name to mixpanel for aab format`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for adding a test!

test_runner/src/test/kotlin/ftl/util/MatricesMocks.kt Outdated Show resolved Hide resolved
test_runner/src/test/kotlin/ftl/run/TestRunnerTest.kt Outdated Show resolved Hide resolved
@bootstraponline
Copy link
Contributor

bootstraponline commented Dec 2, 2021

@Xlopec it looks like the code is failing lint check. Details in the GitHub actions build jobs.

lintKotlinMain sources failed lint check

$ ./gradlew lintKotlinMain

> Task :test_runner:lintKotlinMain FAILED
/flank/test_runner/src/main/kotlin/ftl/run/platform/RunAndroidTests.kt:16:1: Lint error > [no-wildcard-imports] Wildcard import
/flank/test_runner/src/main/kotlin/ftl/run/platform/RunAndroidTests.kt:17:1: Lint error > [no-unused-imports] Unnecessary import
/flank/test_runner/src/main/kotlin/ftl/run/platform/RunAndroidTests.kt:17:1: Lint error > [no-wildcard-imports] Wildcard import

FAILURE: Build failed with an exception.

@bootstraponline
Copy link
Contributor

@Xlopec If you could sign the CLA, then the builds should turn green.

image
image

@Xlopec
Copy link
Contributor Author

Xlopec commented Dec 15, 2021

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Dec 15, 2021
@bootstraponline
Copy link
Contributor

@Xlopec It looks like the test you added is failing in CI.

@bootstraponline
Copy link
Contributor

image

@jstewart5000 jstewart5000 merged commit 4a6e2b7 into Flank:master Apr 7, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad Request error on getApkDetails when using .aab instead of .apk
3 participants