-
Notifications
You must be signed in to change notification settings - Fork 74
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
Run iOS UIKit instrumented tests on simulator #1396
Conversation
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.
val density = Density(density = UIScreen.mainScreen().scale.toFloat()) | ||
val window = UIWindow(frame = UIScreen.mainScreen().bounds()) | ||
val screenSize = DpSize( | ||
width = UIScreen.mainScreen().bounds().useContents { size.width.dp }, | ||
height = UIScreen.mainScreen().bounds().useContents { size.height.dp } | ||
) |
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.
Let's avoid multiple calls of UIScreen.mainScreen()
function
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.
Done
fun <T> waitToEquals(expected: () -> T, actual: () -> T, deadline: Duration = 1.seconds) { | ||
waitForExpectation(deadline) { | ||
expected() == actual() | ||
} |
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.
- I don't think that we need to redefine asserts here
- Waiting should be based on
isIdle
check but not on comparing exect/actual from assert
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.
As I mentioned, isIdle does not work correctly when we're not bound with manually operating clocks.
I will be working on the proper implementation in the following MRs.
@@ -262,4 +266,30 @@ open class AndroidXComposeMultiplatformExtensionImpl @Inject constructor( | |||
iosSimulatorArm64("uikitSimArm64") { configureFreeCompilerArgs() } | |||
} | |||
} | |||
|
|||
override fun configureTestsRunInIosSimulatorEnvironment(device: String?): Unit = |
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.
We need to be ready to combine AndroidXCompose
copy back into AndroidXExtension
(used in AOSP and in fork for lifecycle modules).
Please do that changes in AndroidXPlugin
instead of AndroidXComposePlugin
(or at least in both places).
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.
After discussion with @eymar , we decided to place it here. Let's discuss one more time to have clear understanding.
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.
We need to be ready to combine AndroidXCompose
copy back into AndroidXExtension
(used in AOSP and in fork for lifecycle modules).
Please do that changes in AndroidXPlugin
instead of AndroidXComposePlugin
(or at least in both places).
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.
Discussed. Keep it in AndroidXCompose for now.
compose/ui/ui/build.gradle
Outdated
@@ -146,6 +146,7 @@ if(AndroidXComposePlugin.isMultiplatformEnabled(project)) { | |||
wasm() | |||
|
|||
configureDarwinFlags() | |||
configureTestsRunInIosSimulatorEnvironment() |
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.
Why should it be explicit call for each module?
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 should be explicit call for the module that support tests with booted simulator. For now, only one module needs this environment.
Without going too deep into the implementation itself, a few concerns:
|
Test was extracted into another source-set |
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/uikitInstrumentedTest/kotlin/android/compose/ui/test/UIKitInstrumentedTest.kt
Outdated
Show resolved
Hide resolved
dfcc821
to
cad7f4c
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.
buildSrc
changes LGTM
Run iOS UIKit tests on simulator
Implement basic keyboard appearing tests
Improvements needed:
Release Notes
Run iOS UIKit instrumented tests on simulator