-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat: Additional config options for test pairs #2004
Conversation
01647aa
to
527fb5d
Compare
Timestamp: 2021-06-09 07:26:43 |
40e05f0
to
2225f1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks alright for now. Lets get another pair of eyes on it aswell.
@asadsalman Also have a look at the effect it might have for your changes a while back and if it fixes the issues.
@@ -73,18 +70,11 @@ private fun createAndroidTestMatrix( | |||
}.getOrElse { e -> throw FlankGeneralError(e) } | |||
} | |||
|
|||
fun TestMatrixAndroid.Config.clientInfo(matrix: TestMatrixAndroid.Type): ClientInfo { | |||
return if (matrix is TestMatrixAndroid.Type.Instrumentation && matrix.clientDetails.isNotEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pawelpasterz I am sure that there were changes done here for a fix by @asadsalman. Can we confirm that it wont be effected by this change as it seems like a revert to the previous version of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - this will revert those changes. But this is intentional, previous implementation merges client details (from main and additional test pair). All the other, 'additional' configs overwrite 'main' configs. I think we should be consistent here. Saying that merging is not the best option and clientDetails
should be overwritten as well.
test_runner/src/main/kotlin/ftl/run/model/AndroidTestContext.kt
Outdated
Show resolved
Hide resolved
@pawelpasterz this PR looks great! Thanks for adding more fields to additional test APKs! @Sloox, I had added unit tests that that covered the changes I had made previously. They're still passing so I don't think there's issue or regression here. I'll manually verify with an end-to-end run today too but I don't expect any issues. |
4404cfd
to
d4fea83
Compare
I love this! That's one reason why tests are valuable. We're less likely to break business logic when refactoring. |
2f625ee
to
78a4d3d
Compare
@@ -9,8 +9,8 @@ import com.google.testing.model.TestMatrix as GoogleTestMatrix | |||
|
|||
object GoogleTestMatrixAndroid : | |||
TestMatrixAndroid.Execute, | |||
(TestMatrixAndroid.Config, List<TestMatrixAndroid.Type>) -> List<TestMatrix.Data> by { config, types -> | |||
(List<Pair<TestMatrixAndroid.Config, TestMatrixAndroid.Type>>) -> List<TestMatrix.Data> by { configTypePairs -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be better to have a separate data class instead of pair, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree, thanks!
35ee69c
to
5f10825
Compare
Fixes #1815
This PR introduces a couple of changes:
AndroidConfig
With the above enhancements, we can extend
additional-test-app-pairs
easier in the future. For now, only community-requested options are implemented.Test Plan
./gradlew flankFullRun
Checklist