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

Provide a way to unset kotlinx.coroutines.DefaultDelay #3895

Closed
saket opened this issue Sep 20, 2023 · 12 comments
Closed

Provide a way to unset kotlinx.coroutines.DefaultDelay #3895

saket opened this issue Sep 20, 2023 · 12 comments

Comments

@saket
Copy link

saket commented Sep 20, 2023

On Android, kotlinx.coroutines.initializeDefaultDelay() chooses between a android.os.Looper-backed dispatcher or a custom event loop depending on whether Dispatchers.Main.immediate is available. It is an immutable value that can't be changed once it is initialized. This assumption made sense when it was initially implemented, as test sources were not expected to include a real Looper implementation.

However, this has changed with the introduction of JVM based screenshot testing with libraries such as paparazzi because they (transitively) ship with a fake implementation of Looper. This is causing unit tests that use delay to fail through out the module because paparazzi can't unset DefaultDelay once it is set (more context).

Can kotlin provide a way to unset or reinitialize kotlinx.coroutines.DefaultDelay to address this?

@dkhalanskyjb
Copy link
Collaborator

On Android, kotlinx.coroutines.initializeDefaultDelay() chooses between a android.os.Looper-backed dispatcher or a custom event loop depending on whether Dispatchers.Main.immediate is available.

It only does this if the kotlinx.coroutines.main.delay system property is set to true during the initialization. If you don't set it to true, it will always choose the custom event loop. Here's the commit that established this behavior: 9169d09. Does this help?

ibabuk added a commit to ibabuk/telephoto that referenced this issue Sep 21, 2023
* trunk:
  Prepare next development version
  Prepare to release v0.6.1
  Workaround Kotlin/kotlinx.coroutines#3895
  Remove usage of collectLatest in BitmapCache
@saket
Copy link
Author

saket commented Sep 21, 2023

Unfortunately not because setting that flag to false will prevent screenshot tests from working. That flag is enabled at build time so that the main dispatcher can be used for scheduling tasks in screenshot tests.

@dkhalanskyjb
Copy link
Collaborator

Please provide a simplified example of a "screenshot test" so we can work out together how to make it not rely on Dispatchers.Main being the default delay.

@jrodbx
Copy link

jrodbx commented Sep 23, 2023

Please provide a simplified example of a "screenshot test" so we can work out together how to make it not rely on Dispatchers.Main being the default delay.

cc: @gamepro65

@dkhalanskyjb
Copy link
Collaborator

@jrodbx, @saket, are there any news about this?

@saket
Copy link
Author

saket commented Nov 20, 2023

Sorry, I haven't been able to find time to create a demo project. However you should be able to checkout paparazzi's sample project and run the code mentioned here to reproduce the problem:

cashapp/paparazzi#1101 (comment)

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Nov 21, 2023

I understand why tests hang/fail when you set kotlinx.coroutines.main.delay and have a fake Looper. What I don't understand is why you have kotlinx.coroutines.main.delay set for tests in the first place. If you unset kotlinx.coroutines.main.delay, the problem will be gone, and the issue will be solved.

@saket
Copy link
Author

saket commented Nov 21, 2023

The PR that introduced kotlinx.coroutines.main.delay to paparazzi offers some context. Does this answer your question? cashapp/paparazzi#739

@dkhalanskyjb
Copy link
Collaborator

It does, thanks.

Some background information: kotlinx.coroutines.main.delay was introduced as a tiny performance optimization to save people from allocating one extra thread: #2972. It turned out to be a mistake, so we reverted it: 9169d09 The commit message contains the rationale.

  • I'm certain that forcibly exposing the users of paparazzi to those issues is a mistake.
  • Additionally, given that it uses virtual time, there's one extra problem: now the users of paparazzi have no clear way to use the real-world time. This becomes a problem when the test interacts with the real world in some way, which can happen with integration tests. For example, if you have withTimeout(1.seconds) { networkCall() } and the virtual time reaches 1 second, networkCall will fail with a timeout; if you have rate limiting, apiCall(); delay(rateLimiter.nextQuery()); apiCall(), then these API calls will either happen immediately one after another, or there will be an indeterminate delay; etc.
  • Lastly, I just think it breaks the separation of responsibilities. The project's README states that this is a way to render screens without relying on emulators. Setting kotlinx.coroutines.main.delay is a pretty explicit assumption that everything tests do is in service of rendering screens, as opposed to treating the rendering process as yet another component used alongside others. There are other ways to orchestrate the components to work in tandem than having one control all the others. For example,
val FRAME_DURATION: Duration = 11111.microseconds // 90 Hz refresh rate

// does delay skipping in the test dispatchers
kotlinx.coroutines.test.runTest {
  // this block of code can be provided by paparazzi for convenience
  val screen: StateFlow<ScreenState> = MutableStateFlow(renderScreen())
  backgroundScope.launch {
    while (true) {
      delay(FRAME_DURATION)
      screen.value = renderScreen()
    }
  }
  launch {
    delay(FRAME_DURATION * 30)
    addAThingToScreen()
  }
  delay(FRAME_DURATION * 31)
  validateThingOnScreen(screen.value)
}

Something like that: you have delay-skipping, you execute normal-looking code that's time-aware, and the changes get reflected on the screen in time.

However, paparazzi is not a library for which I'm responsible, so it's not my call.

If paparazzi chooses to double down on the decision to subsume all handling of time, then I still don't see why we should do anything about it. In order for kotlinx.coroutines.delay.main to work, the Main dispatcher must be provided. Since paparazzi already provides their Main dispatcher, can't they configure that dispatcher directly to use or not use the real-world time? Something like fun runPaparazziTest(testCode: () -> Unit) { PaparazziMainDispatcher.useVirtualTime(); try { testCode() } finally { PaparazziMainDispatcher.useRealWorldTime() } }.

@saket, I see you are active in the paparazzi's repository, so could you please forward this information to the responsible parties?

@saket
Copy link
Author

saket commented Nov 22, 2023

cc @jrodbx @gamepro65

@gamepro65
Copy link

👋 Thanks @dkhalanskyjb for the background on this flag it really helps. The missing piece for us was where to integrate our own Delay as the CoroutineContext used is abstracted away from consumers inside ComposeUi. Using this flag was convenient but agreed it isn't great for our end users. Took another look and found a better integration point which lets us ditch this flag.

@dkhalanskyjb
Copy link
Collaborator

Good to know that you found a solution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants