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.

Sign up for GitHub

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

Change isIdle in tests to return true when there exist delayed tasks #1550

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Change isIdle in tests to return true when there exist delayed tasks #1550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
065e0fe
Change `isIdle` in tests to return `true` when there exist delayed (e…
m-sasha Sep 9, 2024
9078e5c
Re-render scene every 16ms (in test-time) when testing.
m-sasha Sep 18, 2024
2b0eb08
Fix faulty tests.
m-sasha Sep 18, 2024
28c3cda
Address review feedback.
m-sasha Sep 30, 2024
75b8c7d
Changed sleep to delay in `awaitForIdle`.
m-sasha Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 10 additions & 22 deletions compose/ui/ui-test-junit4/src/desktopTest/kotlin/androidx/compose/ui/test/TestBasicsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class TestBasicsTest {

// See https://github.com/JetBrains/compose-multiplatform/issues/3117
@Test
fun recompositionCompletesBeforeSetContentReturns() = repeat(1000) {
fun recompositionCompletesBeforeSetContentReturns() = repeat(100) {
runSkikoComposeUiTest {
var globalValue by atomic(0)
setContent {
Expand Down Expand Up @@ -92,27 +92,6 @@ class TestBasicsTest {
assertTrue(clockAfter > clockBefore, "performClick did not advance the test clock")
}

@Test
fun advancingClockRunsRecomposition() {
rule.mainClock.autoAdvance = false

rule.setContent {
var text by remember { mutableStateOf("1") }
Text(text, modifier = Modifier.testTag("text"))

LaunchedEffect(Unit) {
delay(1_000)
text = "2"
}
}

rule.onNodeWithTag("text").assertTextEquals("1")
rule.mainClock.advanceTimeBy(999, ignoreFrameDuration = true)
rule.onNodeWithTag("text").assertTextEquals("1")
rule.mainClock.advanceTimeBy(1, ignoreFrameDuration = true)
rule.onNodeWithTag("text").assertTextEquals("2")
}

@Test
fun obtainingSemanticsNodeInteractionWaitsUntilIdle() {
var text by mutableStateOf("1")
Expand Down Expand Up @@ -251,4 +230,13 @@ class TestBasicsTest {

assertContentEquals(listOf("Composition", "LaunchedEffect"), actions)
}

@Test
fun waitForIdleDoesNotAdvanceClockIfAlreadyIdle() = runComposeUiTest {
setContent { }

val initialTime = mainClock.currentTime
waitForIdle()
assertEquals(initialTime, mainClock.currentTime)
}
}
68 changes: 36 additions & 32 deletions compose/ui/ui-test/src/skikoMain/kotlin/androidx/compose/ui/test/ComposeUiTest.skikoMain.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import androidx.compose.ui.platform.InfiniteAnimationPolicy
import androidx.compose.ui.platform.PlatformContext
import androidx.compose.ui.platform.WindowInfo
import androidx.compose.ui.scene.ComposeScene
import androidx.compose.ui.scene.ComposeSceneContext
import androidx.compose.ui.scene.CanvasLayersComposeScene
import androidx.compose.ui.semantics.SemanticsNode
import androidx.compose.ui.text.input.EditCommand
Expand Down Expand Up @@ -189,7 +188,7 @@ class SkikoComposeUiTest @InternalTestApi constructor(
fun <R> runTest(block: SkikoComposeUiTest.() -> R): R {
return composeRootRegistry.withRegistry {
withScene {
renderingAtFrameRate {
withRenderLoop {
block()
}
}
Expand All @@ -210,13 +209,15 @@ class SkikoComposeUiTest @InternalTestApi constructor(
}
}

private inline fun <R> renderingAtFrameRate(block: () -> R): R {
private inline fun <R> withRenderLoop(block: () -> R): R {
val scope = CoroutineScope(coroutineContext)
return try {
scope.launch {
while (isActive) {
delay(FRAME_DELAY_MILLIS)
render(mainClock.currentTime)
runOnUiThread {
render(mainClock.currentTime)
}
}
}
block()
Expand All @@ -225,20 +226,16 @@ class SkikoComposeUiTest @InternalTestApi constructor(
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

executing delay-ed coroutines

This is not only about delay, right? I.e. any suspend on an external operation (network call) is no longer non-idle.

To fix this advance the test time via mainClock manually, as needed.

Could you add an example?

  • Tests that relied on waitForIdle (whether explicit or implicit) executing delay-ed coroutines will no longer work correctly. To fix this advance the test time via mainClock manually, as needed:
    code

Also, we need to write what people should do if they waited not only for delay, but for something else.

Copy link
Member Author

@m-sasha m-sasha Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is not only about delay, right? I.e. any suspend on an external operation (network call) is no longer non-idle.

No, it's only about delay. Coroutines suspended not through delay are not registered in FlushCoroutineDispatcher.

Could you add an example?

Where should I add it? In this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where should I add it? In this PR?

In the release notes as I in a example I provided (just add indentation)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Coroutines suspended not through delay are not registered in FlushCoroutineDispatcher

Looked at the code, and indeed, such tasks are not registered until the coroutine is resumed by something else

Copy link
Collaborator

Choose a reason for hiding this comment

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

Open it again, as we still need a code example in Release Notes

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a code example, and documented the change in advanceTimeBy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change "Test API" to "Multiple Platforms". We have a strict list of categories in https://github.com/JetBrains/compose-multiplatform-core/edit/jb-main/.github/PULL_REQUEST_TEMPLATE.md. And usually don't categorize it by Compose modules

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* Render the scene at the given time.
*/
private fun render(timeMillis: Long) {
scene.render(
surface.canvas.asComposeCanvas(),
timeMillis * NanoSecondsPerMilliSecond
)
}

private fun renderNextFrame() = runOnUiThread {
render(mainClock.currentTime)
if (mainClock.autoAdvance) {
mainClock.advanceTimeByFrame()
}
}

private fun createUi() = CanvasLayersComposeScene(
density = density,
size = size,
Expand All @@ -247,49 +244,54 @@ class SkikoComposeUiTest @InternalTestApi constructor(
invalidate = { }
)

private fun shouldPumpTime(): Boolean {
return mainClock.autoAdvance &&
(Snapshot.current.hasPendingChanges()
|| Snapshot.isApplyObserverNotificationPending
|| scene.hasInvalidations())
private fun advanceIfNeededAndRenderNextFrame() {
if (mainClock.autoAdvance) {
mainClock.advanceTimeByFrame()
// The rendering is done by withRenderLoop
} else {
runOnUiThread {
render(mainClock.currentTime)
}
}
}

@OptIn(InternalComposeUiApi::class)
private fun isIdle(): Boolean {
var i = 0
while (i < 100 && shouldPumpTime()) {
mainClock.advanceTimeByFrame()
++i
if (composeRootRegistry.getComposeRoots().any { it.hasPendingMeasureOrLayout }) {
return false
}

val hasPendingMeasureOrLayout = composeRootRegistry.getComposeRoots().any {
it.hasPendingMeasureOrLayout
if (!mainClock.autoAdvance) {
return true
}

return !shouldPumpTime() && !hasPendingMeasureOrLayout && areAllResourcesIdle()
return !Snapshot.current.hasPendingChanges()
&& !Snapshot.isApplyObserverNotificationPending
&& !scene.hasInvalidations()
&& areAllResourcesIdle()
}

override fun waitForIdle() {
// TODO: consider adding a timeout to avoid an infinite loop?
do {
// always check even if we are idle
uncaughtExceptionHandler.throwUncaught()
renderNextFrame()
// always check even if we are idle
uncaughtExceptionHandler.throwUncaught()
while (!isIdle()) {
advanceIfNeededAndRenderNextFrame()
uncaughtExceptionHandler.throwUncaught()
if (!areAllResourcesIdle()) {
sleep(IDLING_RESOURCES_CHECK_INTERVAL_MS)
}
} while (!isIdle())
}
}

override suspend fun awaitIdle() {
// always check even if we are idle
uncaughtExceptionHandler.throwUncaught()
while (!isIdle()) {
renderNextFrame()
advanceIfNeededAndRenderNextFrame()
uncaughtExceptionHandler.throwUncaught()
if (!areAllResourcesIdle()) {
delay(IDLING_RESOURCES_CHECK_INTERVAL_MS)
sleep(IDLING_RESOURCES_CHECK_INTERVAL_MS)
m-sasha marked this conversation as resolved.
Show resolved Hide resolved
}
yield()
}
Expand All @@ -315,9 +317,11 @@ class SkikoComposeUiTest @InternalTestApi constructor(
val startTime = currentNanoTime()
val timeoutNanos = timeoutMillis * NanoSecondsPerMilliSecond
while (!condition()) {
renderNextFrame()
advanceIfNeededAndRenderNextFrame()
if (currentNanoTime() - startTime > timeoutNanos) {
buildWaitUntilTimeoutMessage(timeoutMillis, conditionDescription)
throw ComposeTimeoutException(
buildWaitUntilTimeoutMessage(timeoutMillis, conditionDescription)
)
}
}

Expand Down
18 changes: 12 additions & 6 deletions compose/ui/ui/src/desktopTest/kotlin/androidx/compose/ui/awt/ComposeFocusTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,10 @@ import androidx.compose.ui.unit.dp
import androidx.compose.ui.window.Popup
import com.google.common.truth.Truth.assertThat
import java.awt.BorderLayout
import java.awt.Component
import java.awt.Dimension
import java.awt.GraphicsEnvironment
import java.awt.KeyboardFocusManager
import java.awt.Window
import java.awt.event.FocusEvent
import java.awt.event.FocusEvent.Cause
import java.awt.event.KeyEvent
import javax.swing.JButton
Expand All @@ -51,8 +49,6 @@ import javax.swing.JPanel
import kotlin.random.Random
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertTrue
import kotlin.test.assertFalse
import kotlinx.coroutines.delay
import kotlinx.coroutines.runBlocking
import org.jetbrains.skiko.MainUIDispatcher
Expand Down Expand Up @@ -954,15 +950,25 @@ class FocusTestScope {
windows.clear()
}

private fun focusOwner() = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner.also {
if (it == null) {
throw AssertionError(
"The focus owner is `null`; " +
"This can happen due to interference from the user or the window manager. " +
"Try running the test without interacting with any windows."
)
}
}

suspend fun pressNextFocusKey() {
val focusOwner = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner
val focusOwner = focusOwner()
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_PRESSED, 0, 0, KeyEvent.VK_TAB, '\t'))
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_RELEASED, 0, 0, KeyEvent.VK_TAB, '\t'))
awaitEdtAfterDelay()
}

suspend fun pressPreviousFocusKey() {
val focusOwner = KeyboardFocusManager.getCurrentKeyboardFocusManager().focusOwner
val focusOwner = focusOwner()
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_PRESSED, 0, KeyEvent.SHIFT_DOWN_MASK, KeyEvent.VK_TAB, '\t'))
focusOwner.dispatchEvent(KeyEvent(focusOwner, KeyEvent.KEY_RELEASED, 0, KeyEvent.SHIFT_DOWN_MASK, KeyEvent.VK_TAB, '\t'))
awaitEdtAfterDelay()
Expand Down
19 changes: 10 additions & 9 deletions ...ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,20 @@ internal class FlushCoroutineDispatcher(
private val scope = CoroutineScope(scope.coroutineContext.minusKey(Job))
private var immediateTasks = ArrayDeque<Runnable>()
private val delayedTasks = ArrayDeque<Runnable>()
private val tasksLock = createSynchronizedObject()
private val immediateTasksLock = createSynchronizedObject()
private val delayedTasksLock = createSynchronizedObject()
private var immediateTasksSwap = ArrayDeque<Runnable>()
@Volatile
private var isPerformingRun = false
private val runLock = createSynchronizedObject()

override fun dispatch(context: CoroutineContext, block: Runnable) {
synchronized(tasksLock) {
synchronized(immediateTasksLock) {
immediateTasks.add(block)
}
scope.launch {
performRun {
val isTaskAlive = synchronized(tasksLock) {
val isTaskAlive = synchronized(immediateTasksLock) {
immediateTasks.remove(block)
}
if (isTaskAlive) {
Expand All @@ -70,12 +71,12 @@ internal class FlushCoroutineDispatcher(
* Whether the dispatcher has immediate tasks pending.
*/
fun hasImmediateTasks() = isPerformingRun ||
synchronized(tasksLock) { immediateTasks.isNotEmpty() }
synchronized(immediateTasksLock) { immediateTasks.isNotEmpty() }

/**
* Whether the dispatcher has delayed tasks pending.
*/
fun hasDelayedTasks() = synchronized(tasksLock) { delayedTasks.isNotEmpty() }
fun hasDelayedTasks() = synchronized(delayedTasksLock) { delayedTasks.isNotEmpty() }

/**
* Perform all scheduled tasks and wait for the tasks which are already
Expand All @@ -85,7 +86,7 @@ internal class FlushCoroutineDispatcher(
// Run tasks until they're empty in order to executed even ones that are added by the tasks
// pending at the start
while (true) {
synchronized(tasksLock) {
synchronized(immediateTasksLock) {
if (immediateTasks.isEmpty())
return@performRun

Expand All @@ -112,13 +113,13 @@ internal class FlushCoroutineDispatcher(
@OptIn(ExperimentalCoroutinesApi::class)
override fun scheduleResumeAfterDelay(timeMillis: Long, continuation: CancellableContinuation<Unit>) {
val block = Runnable { continuation.resume(Unit, null) }
synchronized(tasksLock) {
synchronized(delayedTasksLock) {
delayedTasks.add(block)
}
val job = scope.launch {
kotlinx.coroutines.delay(timeMillis)
performRun {
val isTaskAlive = synchronized(tasksLock) {
val isTaskAlive = synchronized(delayedTasksLock) {
delayedTasks.remove(block)
}
if (isTaskAlive) {
Expand All @@ -128,7 +129,7 @@ internal class FlushCoroutineDispatcher(
}
continuation.invokeOnCancellation {
job.cancel()
synchronized(tasksLock) {
synchronized(delayedTasksLock) {
delayedTasks.remove(block)
}
}
Expand Down
3 changes: 1 addition & 2 deletions compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,9 @@ class RenderPhasesTest {
assertContentEquals(emptyList(), phases)
startTest = true
mainClock.advanceTimeByFrame()
waitForIdle() // Make the effects run
assertContentEquals(
expected = listOf("draw", "launchedWithFrame", "launchedFromComposition", "launchedFromEffect"),
actual = phases.subList(0, 4),
actual = phases
)
}

Expand Down
2 changes: 1 addition & 1 deletion compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/window/DialogTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,8 @@ class DialogTest {

assertEquals(null, lastValueInComposition)
showDialog = true
mainClock.advanceTimeBy(1000)
waitForIdle()
mainClock.advanceTimeBy(1000)
assertEquals(2, lastValueInComposition)
}
}