Omit missing app apk if additional-app-test-apks specified #801
Conversation
jan-goral
changed the title
jan-goral
force-pushed
the
800-app-test-apks
branch
from
1d32163
to
0d94350
Compare
jan-goral
requested review from
adamfilipow92,
bootstraponline and
piotradamczyk5
Codecov Report
@@ Coverage Diff @@
## master #801 +/- ##
============================================
+ Coverage 78.77% 78.82% +0.04%
+ Complexity 696 669 -27
============================================
Files 143 148 +5
Lines 2950 2966 +16
Branches 423 431 +8
============================================
+ Hits 2324 2338 +14
Misses 354 354
- Partials 272 274 +2 |
|
Should we also omit missing test apk when using additiona-app-test-apks? I think Fladle wanted to specify all the app/test apks via additional-app-test-apks. The PR title and release note only mention the app apk. Maybe the test apk is already optional? |
|
Hey, thank you very much for doing this. Yes @bootstraponline is correct that we want to specify all app/test apks via additional app test apks. That being said, and I really appreciate this change, after having implemented the plugin I think I am unlikely to take advantage of this API for now. Many of the largers companies who require automatically specifying their additional test apks will be stuck on flank 8.1.0 for the foreseeable future until firebase test lab supports more than 50 shards. If Fladle were to take advantage of this new API, it would not be backward compatible anymore. That being said, I really hope google will change their mind about the 50 shard limitation and this api will greatly simplify this part of the code: https://github.com/runningcode/fladle/blob/master/buildSrc/src/main/java/com/osacky/flank/gradle/FulladlePlugin.kt#L42 |
|
@runningcode I already persuaded FTL to increase the limit, it will be raised to 250 in a few weeks. We will probably be breaking backwards compatibility in the next release. #792 |
Yes, test apk was already optional because robo tests requires only app apk |
| ).plus( | ||
| elements = additionalAppTestApks.map { | ||
| ResolvedApks( | ||
| app = it.app ?: appApk, | ||
| app = it.app ?: appApk!!, |
There was a problem hiding this comment.
I think should throw here custom exception, not nasty NPE (which should not happen in kotlin app)
There was a problem hiding this comment.
agreed, I don't like force unwraps in the production code.
| ) | ||
| } | ||
|
|
||
| @Test(expected = Exception::class) |
There was a problem hiding this comment.
In addition to previous comment about NPE. We should check here for this particular custom exception.
| @@ -104,13 +104,17 @@ class AndroidArgs( | |||
| override val hasMultipleExecutions: Boolean get() = super.hasMultipleExecutions || additionalAppTestApks.isNotEmpty() | |||
|
|
|||
| init { | |||
| if (appApk == null) additionalAppTestApks.filter { (app, _) -> app == null }.let { missingApks -> | |||
| if (missingApks.isNotEmpty()) throw FlankFatalError("Cannot resolve app apk pair for ${missingApks.map { it.test }}") | |||
There was a problem hiding this comment.
I am wondering if it wasn't be better to print just apk file name not full path to file. For many (potential) incorrect pairs it would be hard to read something.
There was a problem hiding this comment.
I agree a file name is easier to read. I think the full path will be useful when debugging problems in CI where the paths aren't obvious though. Maybe we could print each missing path on a new line to improve readability?
jan-goral
force-pushed
the
800-app-test-apks
branch
from
0d94350
to
a13f6d2
Compare
| @@ -243,7 +243,7 @@ class AndroidRunCommand : CommonRunCommand(), Runnable { | |||
| // AndroidFlankYml | |||
|
|
|||
| @Option( | |||
| names = ["--additional-app-test-apks"], | |||
| names = ["--app-test-apks", "--additional-app-test-apks"], | |||
There was a problem hiding this comment.
we should probably keep the flags 1:1 mapping with the yaml. --app-test-apks isn't a valid key name in the yaml.
Fixes #800
Checklist