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

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

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Sep 9, 2024

We currently hang in tests if there's an infinite loop with delay in a LaunchedEffect:

    @Test
    fun loopInLaunchedEffectTest() = runSkikoComposeUiTest {
        setContent {
            LaunchedEffect(Unit) {
                while (true) {
                    delay(1000)
                    println("Tick")
                }
            }
        }
    }

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:

@Test
fun advancingClockCausesRecompositions() = runComposeUiTest {
    mainClock.autoAdvance = false
    var value by mutableStateOf(0)
    val compositionValues = mutableListOf<Int>()
    setContent {
        compositionValues.add(value)
        val capturedValue = value
        LaunchedEffect(capturedValue) {
            delay(1000)
            value = capturedValue + 1
        }
    }

    assertEquals(0, value)
    mainClock.advanceTimeBy(10000)
    assertContentEquals(0..9, compositionValues)
}

Previously, only 0 and 1 would be seen by the composition.

Testing

  • Added unit tests.

Release Notes

Fixes - Multiple Platforms

  • waitForIdle, awaitIdle and runOnIdle no longer consider Compose to be non-idle when coroutines launched in a composition scope call delay. This prevents tests with an infinite loop with delay in a LaunchedEffect from hanging.
  • Tests that advance the test clock (via mainClock.advanceTimeBy) will now correctly (re)compose/layout/draw/effects each virtual frame as needed.

Breaking Changes - Multiple Platforms

  • Tests that relied on waitForIdle, awaitIdle or runOnIdle (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.
    For example, tests that previously did something like:
    var updateText by mutableStateOf(false)
    var text by mutableStateOf("0")
    setContent {
        LaunchedEffect(updateText) {
            if (updateText) {
                delay(1000)
                text = "1"
            }
        }
    }
    updateText = true
    waitForIdle()
    assertEquals("1", text)
    
    will need to add mainClock.advanceTimeBy(1000) after waitForIdle(), because waitForIdle no longer waits for the delay-ed coroutine to complete.
  • Tests that advance the test clock (via mainClock.advanceTimeBy) may see different behavior with regards to the amount and timing of recomposition, layout, drawing and effects.

@m-sasha m-sasha force-pushed the m-sasha/report-idle-with-delayed-launched-effect branch 4 times, most recently from 6eb6729 to 340a155 Compare September 11, 2024 09:09
@m-sasha m-sasha requested a review from igordmn September 18, 2024 14:17
@m-sasha m-sasha force-pushed the m-sasha/report-idle-with-delayed-launched-effect branch from 86efb93 to 24a9807 Compare September 27, 2024 06:47
scope.cancel("Run completed")
}
}

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

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

@igordmn igordmn requested a review from eymar September 27, 2024 11:08
@igordmn
Copy link
Collaborator

igordmn commented Sep 27, 2024

@eymar, could you also look at the PR? As it is a breaking API behaviour change.

@m-sasha m-sasha force-pushed the m-sasha/report-idle-with-delayed-launched-effect branch from 24a9807 to 9f62ba5 Compare September 27, 2024 12:09
@eymar
Copy link
Member

eymar commented Sep 27, 2024

Regarding the failing web tests. They pass locally ./gradlew :mpp:testWeb

@m-sasha m-sasha force-pushed the m-sasha/report-idle-with-delayed-launched-effect branch 2 times, most recently from 4cfc512 to c973fff Compare September 30, 2024 12:48
@m-sasha m-sasha requested review from igordmn and eymar September 30, 2024 12:48
@m-sasha m-sasha force-pushed the m-sasha/report-idle-with-delayed-launched-effect branch from c973fff to 28c3cda Compare September 30, 2024 16:32
scope.cancel("Run completed")
}
}

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

@m-sasha m-sasha requested a review from igordmn September 30, 2024 19:14
@m-sasha m-sasha merged commit 4fb25b9 into jb-main Oct 1, 2024
6 checks passed
@m-sasha m-sasha deleted the m-sasha/report-idle-with-delayed-launched-effect branch October 1, 2024 08:43
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.

3 participants