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

System property to control BlockingChecker extension point for runBlocking #458

Merged
merged 1 commit into from
Jul 26, 2018

Conversation

elizarov
Copy link
Contributor

@elizarov elizarov commented Jul 26, 2018

Using -Dkotlinx.coroutines.blocking.checker=disable to ignore all installed blocking checkers.

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

LGTM

@elizarov elizarov merged commit 46a598e into develop Jul 26, 2018
@PaulWoitaschek
Copy link
Contributor

How can I use this in unit tests only? This broke all my robolectric tests.

@jcornaz
Copy link
Contributor

jcornaz commented Jul 28, 2018

@PaulWoitaschek, execute System.setProperty(BLOCKING_CHECKER_PROPERTY_NAME, BLOCKING_CHECKER_VALUE_DISABLE) before your tests

@PaulWoitaschek
Copy link
Contributor

Do you know how I can specify in my root gradle file how to disable this for just everything? Trying to figure that out for ever now.

@PaulWoitaschek
Copy link
Contributor

PaulWoitaschek commented Jul 29, 2018

This needs to have a better solution. runBlocking is the base of all my unit tests. Maybe a dedicated method that disables this check? Like fun runTest(()->Unit)

@jcornaz
Copy link
Contributor

jcornaz commented Jul 29, 2018

This needs to have a better solution. runBlocking is the base of all my unit tests. Maybe a dedicated method that disables this check? Like fun runTest(()->Unit)

  1. You can block in unit test without any problem. What you cannot do is block in a UI context.
  2. Why doesn't the system property suits your needs?

@jcornaz
Copy link
Contributor

jcornaz commented Jul 29, 2018

Do you know how I can specify in my root gradle file how to disable this for just everything? Trying to figure that out for ever now.

As said above use the system property.

test {
    systemProperty "kotlinx.coroutines.blocking.checker", "disable"
}

@PaulWoitaschek
Copy link
Contributor

Test doesn't exist. When I run my android unit tests, they get executed on the UI thread and all fail.

@jcornaz
Copy link
Contributor

jcornaz commented Jul 29, 2018

Test doesn't exist

I don't understand. Can you share an example? It may be simpler.

@PaulWoitaschek
Copy link
Contributor

Sure:
https://github.com/PaulWoitaschek/RunBlockingTest

If you run ./gradlew app:test:

ExampleUnitTest fails with java.lang.RuntimeException: Method myLooper in android.os.Looper not mocked. See http://g.co/androidstudio/not-mocked for details..

ExampleUnitTest2 fails with: `java.lang.IllegalStateException: runBlocking is not allowed in Android main looper thread

If I add

test {
}

to my app/build.gradle it immediately fails with: Could not find method test() for arguments [build_cw4byuz3o88dfeyqmi7qarigi$_run_closure4@64695182] on project ':app' of type org.gradle.api.Project.

In general I really dislike this change. When I add runBlocking on the main thread I have a really good reason to do so. For example I have functions that depends on Environment.getXYZstorageDir as a suspending function because it takes a few milliseconds.

However sometimes i just need to block for these 5ms because my state expects this for further processing.
The change that there is now an exception being thrown is just too opinionized.

@tsuharesu
Copy link

tsuharesu commented Jul 30, 2018

To set the property to run the test with gradlew:

android {
   ...
    testOptions {
        unitTests.all {
            systemProperty "kotlinx.coroutines.blocking.checker", "disable"
        }
    }
}

Unfortunately, this does not work when running inside Android Studio. To set in Android Studio:

image

Android Studio runs the test using Java directly, that's why gradle properties doesn't work:
"...Android Studio 3.2 Preview.app/Contents/jre/jdk/Contents/Home/bin/java" -ea -Didea.test.cyclic.buffer.size=1048576 ...
cc @PaulWoitaschek

@jcornaz the way you wrote works for Java modules, but not in Android ones.

@tsuharesu
Copy link

Another option is to override MainLooperChecker in your test. Put this code in a file inside your test directory (don't mind the package, that's what do the trick):

package kotlinx.coroutines.experimental.android

import kotlinx.coroutines.experimental.BlockingChecker

class MainLooperChecker : BlockingChecker {
    override fun checkRunBlocking() {}
}

@PaulWoitaschek
Copy link
Contributor

PaulWoitaschek commented Jul 30, 2018

I wrote an extension I now use in all my tests because I don't want to actually disable this check if its running. Then my implementation could fail while my tests pass.

fun <T> runTest(
  context: CoroutineContext = EmptyCoroutineContext,
  block: suspend CoroutineScope.() -> T
): T {
  val original = System.getProperty(BLOCKING_CHECKER_PROPERTY_NAME)
  System.setProperty(BLOCKING_CHECKER_PROPERTY_NAME, BLOCKING_CHECKER_VALUE_DISABLE)
  return runBlocking(context) {
    if (original == null) {
      System.clearProperty(BLOCKING_CHECKER_PROPERTY_NAME)
    } else {
      System.setProperty(BLOCKING_CHECKER_PROPERTY_NAME, original)
    }
    block()
  }
}

However I strongly think this should be an opt-in. @elizarov

@jcornaz
Copy link
Contributor

jcornaz commented Jul 30, 2018

Actually I would not be against an argument in runBlocking. Like:

runBlocking(failsInUI = false) {
   // call suspending code
}

It would still fails by default, but allowing users to block the UI when they really want to. (Of course it is bad practice, and should be avoided. But developers should be able to do it they want it / need it)

@svenjacobs
Copy link

svenjacobs commented Aug 1, 2018

I don't understand why now usages of runBlocking within my unit tests fail with java.lang.RuntimeException: Method myLooper in android.os.Looper not mocked. These are unit tests of an Android project however they are not run from an Android environment.

@gildor
Copy link
Contributor

gildor commented Aug 2, 2018

There are some valid use cases to blocking main thread, such as code on app initialization (you probably want to use some existing suspend function) or for some cases like component destroy when you want explicitly block the main thread to save state or clean up some resources. Using launch is not much better, because you have a situation when your component is leaked until operation end. Also in the case of very long operation at least you see that your UI is laggy, instead of hidden memory leak. Again, it's not a perfect solution, but for such edge cases I would like to have real way to block thread.
I would rather provide Strict Mode warning for such cases

@elizarov elizarov deleted the blocking-checker-prop branch August 3, 2018 07:51
@elizarov
Copy link
Contributor Author

elizarov commented Aug 3, 2018

@PaulWoitaschek @svenjacobs @tsuharesu @jcornaz We should automatically detect test environment (with mocked classes) and you will not have this problem in tests and there will be no need to mess with system properties for tests. Let's move that discussion to #464

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.

7 participants