-
Notifications
You must be signed in to change notification settings - Fork 80
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
Change isIdle
in tests to return true
when there exist delayed tasks
#1550
Change isIdle
in tests to return true
when there exist delayed tasks
#1550
Conversation
6eb6729
to
340a155
Compare
86efb93
to
24a9807
Compare
compose/ui/ui-test/src/skikoMain/kotlin/androidx/compose/ui/test/ComposeUiTest.skikoMain.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui-test/src/skikoMain/kotlin/androidx/compose/ui/test/ComposeUiTest.skikoMain.kt
Outdated
Show resolved
Hide resolved
scope.cancel("Run completed") | ||
} | ||
} | ||
|
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.
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) executingdelay
-ed coroutines will no longer work correctly. To fix this advance the test time viamainClock
manually, as needed:
code
Also, we need to write what people should do if they waited not only for delay
, but for something else.
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.
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?
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.
Where should I add it? In this PR?
In the release notes as I in a example I provided (just add indentation)
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.
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
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.
Open it again, as we still need a code example in Release Notes
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.
Added a code example, and documented the change in advanceTimeBy
.
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 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
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
@eymar, could you also look at the PR? As it is a breaking API behaviour change. |
24a9807
to
9f62ba5
Compare
compose/ui/ui-test-junit4/src/desktopTest/kotlin/androidx/compose/ui/test/TestBasicsTest.kt
Show resolved
Hide resolved
compose/ui/ui/src/skikoTest/kotlin/androidx/compose/ui/platform/RenderPhasesTest.kt
Outdated
Show resolved
Hide resolved
...se/ui/ui/src/skikoMain/kotlin/androidx/compose/ui/platform/FlushCoroutineDispatcher.skiko.kt
Outdated
Show resolved
Hide resolved
Regarding the failing web tests. They pass locally |
4cfc512
to
c973fff
Compare
c973fff
to
28c3cda
Compare
compose/ui/ui-test/src/skikoMain/kotlin/androidx/compose/ui/test/ComposeUiTest.skikoMain.kt
Outdated
Show resolved
Hide resolved
scope.cancel("Run completed") | ||
} | ||
} | ||
|
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.
Open it again, as we still need a code example in Release Notes
We currently hang in tests if there's an infinite loop with
delay
in aLaunchedEffect
:This happens because we consider the scene to not be "idle" when there are delayed tasks. Android does not.
Fixes https://youtrack.jetbrains.com/issue/CMP-965/Tests-hang-if-theres-an-infinite-loop-in-LaunchedEffect
Additionally, this PR also runs
render
correctly (recomposition, layout, drawing and effects) each (virtual) frame when the clock is advanced manually by a large value:Previously, only 0 and 1 would be seen by the composition.
Testing
Release Notes
Fixes - Multiple Platforms
waitForIdle
,awaitIdle
andrunOnIdle
no longer consider Compose to be non-idle when coroutines launched in a composition scope calldelay
. This prevents tests with an infinite loop withdelay
in aLaunchedEffect
from hanging.mainClock.advanceTimeBy
) will now correctly (re)compose/layout/draw/effects each virtual frame as needed.Breaking Changes - Multiple Platforms
waitForIdle
,awaitIdle
orrunOnIdle
(whether explicit or implicit) executingdelay
-ed coroutines will no longer work correctly. To fix this advance the test time viamainClock
manually, as needed.For example, tests that previously did something like:
mainClock.advanceTimeBy(1000)
afterwaitForIdle()
, becausewaitForIdle
no longer waits for thedelay
-ed coroutine to complete.mainClock.advanceTimeBy
) may see different behavior with regards to the amount and timing of recomposition, layout, drawing and effects.