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

Add support additional-app-test-apks #83

Merged
merged 1 commit into from
Dec 11, 2019

Conversation

japplin
Copy link
Contributor

@japplin japplin commented Nov 12, 2019

Let me know if this dsl is acceptable if not, let me know how to adjust it.

@runningcode
Copy link
Owner

Hey, thanks for the contribution!
I think we learned that it's best not to use custom classes in the API of fladle. We removed Device from the API recently.
#79

I'd like to not use a custom class here. I like where this is going though. Perhaps if this were a Map<String, String> I think it would work.

@japplin
Copy link
Contributor Author

japplin commented Nov 13, 2019

Hey thanks for the update, what do you think of using Map<String, List<String>>? It would help avoid redeclaring the same debugApk.

@runningcode
Copy link
Owner

Sounds good!

@japplin
Copy link
Contributor Author

japplin commented Nov 14, 2019

Updated

@runningcode
Copy link
Owner

Hey, sorry, I must have missed this in the first review. I was looking at this from mobile. I would like to be able to keep the current api with debugApk and instrumentationApk. Would it be possible to use this map only for the additional-test-apks?

@japplin
Copy link
Contributor Author

japplin commented Nov 18, 2019

I've updated again with restored debugApk and instrumentationApk fields

Copy link
Owner

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

Looks great! I just need to think over if I like the explicit way you specified the debug apks for each instrumentation apk. I think it can make it easier to debug the flank configs. Thoughts?

@@ -56,6 +56,17 @@ internal class YamlWriter {
appendln(" - $file")
}
}
val testApks = config.additionalTestApks.flatMap { (debugApk, instrumentationApks) ->
Copy link
Owner

Choose a reason for hiding this comment

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

This code can be somewhat simplified a bit since the instrumentation apk will take whatever value is above is as the debug apk. See the readme. https://github.com/TestArmada/flank/blob/master/README.md

if (config.additionalTestApks.isNotEmpty()) {
  appendln("  additional-app-test-apks:")
  config.additionalTestApks.entries.forEach { debugApk, instrumentationApks ->
    appendln("    - app: ${debugApk}")
    instrumentationApks.forEach {
        appendln("    test: ${it}")
    }
  }

On the other hand, I do like the more explicit output format that you created here. Let me think this one over overnight. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change this?

@@ -104,6 +104,9 @@ class FlankGradlePlugin : Plugin<Project> {
}
checkNotNull(base.debugApk!!) { "debugApk file cannot be null ${base.debugApk}" }
checkNotNull(base.instrumentationApk!!) { "instrumentationApk file cannot be null ${base.instrumentationApk}" }
base.additionalTestApks.forEach {
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@@ -121,6 +122,9 @@ This is the path to the app's debug apk.
### instrumentationApk
This is the path to the app's instrumentation apk.

### additionalTestApks
Copy link
Owner

Choose a reason for hiding this comment

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

can you add a short sample? and that it is a map of debug apks to a list of instrumentation apks? thanks :)

Copy link
Owner

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

Let's do this! Sorry for not responding earlier.

@runningcode runningcode merged commit e13f219 into runningcode:master Dec 11, 2019
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.

2 participants