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

Support runBlocking for UI Tests #464

Closed
jcornaz opened this issue Jul 30, 2018 · 14 comments
Closed

Support runBlocking for UI Tests #464

jcornaz opened this issue Jul 30, 2018 · 14 comments

Comments

@jcornaz
Copy link
Contributor

jcornaz commented Jul 30, 2018

As mentionned by @PaulWoitaschek in #458, there is a problem with the fact that runBlocking fails by default in a UI thread (see #227).

The problem is that if one wants to perform UI tests, then (depending on tool used) the test methods may be called on the UI thread. (this is apparently the case for android, and it is the case for TestFx with Junit5)

Then there is actually no good solution for theses tests. Yes, one could run the tests using the system property introduced in #458, and the tests will pass. But this is not a good solution, because the tests will pass even if there is a runBlocking in the actual production code, and we'd end-up with passing tests, but failing application.

So here is two proposed solutions:

  • Expose a runTest method which would not fail, even if in UI thread (proposed by @PaulWoitaschek).
  • Add an failsInUI (or better named) argument to runBlocking which would allow to explicitly by-pass the checkers (only for this call of runBlocking)
@fvasco
Copy link
Contributor

fvasco commented Jul 31, 2018

It is possible to create a test dependency for coroutine to put there runTest and whatever is needed.

As a personal consideration runBlocking should check the thread only in development mode.

In production mode :

  1. the check is superfluous
  2. the code have to run as much as possible

@jcornaz
Copy link
Contributor Author

jcornaz commented Jul 31, 2018

As a personal consideration runBlocking should check the thread only in development mode.
[...]
In production mode : the code have to run as much as possible

I totally agree. Using the system property, tests may pass where production would fail. It should be the opposite.

@jcornaz
Copy link
Contributor Author

jcornaz commented Aug 2, 2018

As mentionned by @gildor (in #458) and @svenjacobs (in #227), there are other uses-cases for which it is valid to block the UI thread when it comes to application initialization and cleanup. Especially when theses operations are fast, it would add unnecessary complexity to do it in a non-blocking fashion.

The more it goes, the more I am getting convinced think that:

  • developers should be able to block any thread whenever they decide it makes sense. (even if it is bad practice to do so)
  • the fail-fast feature of runBlocking should only fail in debug mode (as proposed by @fvasco)

@svenjacobs
Copy link

Reposting this from #227 so that it doesn't get lost:

I have a bit of coroutine code that needs to be run in a blocking fashion when my Android application starts. For instance in my Application class I need to fetch a value from a suspending function. Based on the result of that value certain application options are configured. This should actually block because the configuration needs to be finished before/while the application starts. This worked well before version 0.24 and was not noticeable.

How do I solve this use case now?

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Aug 8, 2018

Such features are usually implemented as default safety mechanism against accidental misusages, but when they start to make programming experience worse or require explicit opt-in (especially on every call-site) it's a good marker that feature is harmful.

I think we should revert this change:

  1. There are cases when blocking in UI is legit
  2. UI check is not well integrated with test frameworks
  3. The workaround with system property for tests is unstable and may lead to production crashes (where this property is not set) and this is the opposite of what we wanted
  4. Flag failsInUI is a debatable boilerplate-like solution. E.g. when somewhen have to write blocking test he may just pass this flag everywhere (because runTest is not well-discoverable or entry point may be hidden in production code)
  5. Enabling this feature only in developer mode is not that useful as it's opt-in feature (which should be discovered first) and carries much less value

@ZakTaccardi
Copy link

there are also valid times when you know runBlocking will not block the current thread. For example, I have a ReceiveChannel<T> that I know is backed by a ConflatedBroadcastChannel<T>, so the value is available immediately - but I do not have access to that ConflatedBroadcastChannel<T>.

@ZakTaccardi
Copy link

If the change is reverted (which I think it should be), maybe scarier documentation to discourage runBlocking?

@Hazer
Copy link

Hazer commented Aug 12, 2018

Some kind of opt-in (maybe Android StrictMode style) looks better to me, also because we can expand it further later on, like Android does, or at least, an opt-out as API, maybe per block.

Also, I am loosely familiar with CoroutineContexts so maybe it's a dumb question, but this can't be implemented as a CoroutineContext? Some kind of StrictModeCoroutineContext maybe? Or the Android's CoroutineContext implementation could have some kind of StrictMode property?

As @svenjacobs there was a legacy project, were I got 60% less startup time using runBlocking plus several async calls. By the moment I did it, it wasn't an option to refactor how the app initialization was working internally, but we needed to relieve the issue, for some users, before the internal refactor and then launching the full refactor. runBlocking was exactly what we needed to start using coroutines with the legacy code, and then refactoring small chunks into coroutines or removing then.

If coroutines had this behaviour by that time, we probably would solve it with less efficiency and a really uglier callback hell code, almost no reusable code for the new refactored version, but still there would be blocking code there, just not using coroutines. The easier integration was exactly the selling point by the time.

Sometimes we need to use a bad approach to help in an already worse situation. So then we can focus in re-architecting it all, I believe we can help developers do the right thing, but we must let them do the bad choices also. ¯\_(ツ)_/¯

@svenjacobs
Copy link

I'm very inclined to replace my usages of runBlocking in Application#onCreate() if maybe someone has an idea of how to do it with launch? As I said it is very important that this code block needs to finish before/while the Android application starts.

@PaulWoitaschek
Copy link
Contributor

Launch and then somehow making it blocking would be a workaround. That's exactly what runBlocking is good for.

I really hope for a soon revert.

@elizarov
Copy link
Contributor

elizarov commented Aug 14, 2018

It is already reverted in develop branch. Will be part of the next build (tentatively 0.24.1).

@SUPERCILEX
Copy link
Contributor

So this is case closed, right? It was released in v0.25.

@elizarov
Copy link
Contributor

elizarov commented Aug 25, 2018

It is released in 0.25.0. Limitation on runBlocking are reverted. Closing.

@tomgallagher
Copy link

tomgallagher commented Feb 15, 2019

I've got a question about this and please excuse my ignorance as I'm just getting started with coroutines.

One instance where I am using runBlocking as a convenience method is in Android WebClient, where request handling takes place on the ChromeIO thread and the view remains on the UI thread. If I wish to check that a request is the main frame, I can do this:

view.url == req.url.toString()

But the view is on the main thread so that will fail. I can use runBlocking as a pseudo thread manager, like this

val isMainFrame = runBlocking(Dispatchers.Main) { view.url == req.url.toString() }

And that does the trick but shows the error info message,

Note: end time exceeds epoch:

I could achieve the same thing with an RxJava .blockingFirst() call, moving in and out of the Android main thread to get the result and it wouldn't raise an error info message

So this seems like a good use case, unless I'm totally mistaken.

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

No branches or pull requests

10 participants