Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
13.06k stars 1.85k forks source link

runBlockingTest fails with "This job has not completed yet" #1204

Closed PaulWoitaschek closed 2 years ago

PaulWoitaschek commented 5 years ago

This simple block:

  runBlockingTest {
   suspendCancellableCoroutine<Unit> { cont ->
     thread {
       Thread.sleep(1000)
       cont.resume(Unit)
     }
   }
 }

Fails with the exception:

Exception in thread "main" java.lang.IllegalStateException: This job has not completed yet
    at kotlinx.coroutines.JobSupport.getCompletionExceptionOrNull(JobSupport.kt:1114)
    at kotlinx.coroutines.test.TestBuildersKt.runBlockingTest(TestBuilders.kt:53)
    at kotlinx.coroutines.test.TestBuildersKt.runBlockingTest$default(TestBuilders.kt:45)
    at SamplKt.main(Sampl.kt:8)
    at SamplKt.main(Sampl.kt)

This looks like a bug to me as it should pass.

Kotlin 1.3.31 Coroutines 1.2.1

objcode commented 5 years ago

Thank you for this minimal repro bug report!

Adding some tests for this and taking a look at getting this patched.

objcode commented 5 years ago

as an aside if anyone else is seeing this until it gets fixed - it appears the failure is caused by the thread (the blocking call to sleep is not required)

objcode commented 5 years ago

Patch https://github.com/Kotlin/kotlinx.coroutines/pull/1206 should fix it!

It turns out the behavior of completion in the presence of multiple threads had a non-deterministic outcome that could, in some cases, cause a random test result.

This patch fixes the issue reported here by making it pass (as expected) while providing a detailed error message and test failure if this race condition is detected.

rajab57 commented 5 years ago

Is there a workaround for this issue ?

jimmymorales commented 5 years ago

I came with this exception for some Room database tests (androidTest) using the AndroidJUnit4 runner. It stopped throwing the exception when I added the InstantTaskExecutorRule rule.

@get:Rule
val instantExecutorRule = InstantTaskExecutorRule()

I hope this helps!

Mugurell commented 5 years ago

Is there a workaround for this issue ?

I'm using something like

@Test
fun test() = runBlocking {
    coroutineJob.join()
}
Anton-Spaans-Intrepid commented 5 years ago

@objcode I'm not sure if this patch will fix this issue entirely.

E.g. this code, that uses no other threads, fails with the "This job has not completed" message"

    runBlockingTest {
        flow<Unit> {
            suspendCancellableCoroutine<Nothing> {  }
        }.collect {  }
    }

while this code just finishes without any problems:

    runBlockingTest {
        flow<Unit> {
            delay(Long.MAX_VALUE)
        }.collect {  }
    }
objcode commented 5 years ago

@Anton-Spaans-Intrepid I would expect that to fail as it the collect {} will never complete.

In the patched version, you should see a 30 second wait followed by an error.

What behavior are you expecting?

streetsofboston commented 5 years ago

(I just discovered I used my other GitHub account ... ah well 😄)

I expect the test to never end. It will hang.

objcode commented 5 years ago

@streetsofboston Hm, interesting. Can you elaborate on why?

To me this seems like it "should" fail since you're suspending the test coroutine (the coroutine in rbt) with suspendCancellableCoroutine<Nothing> { } which means during cleanup there is a non-completed coroutine.

There's probably three options: 1) Keep this error as is 2) Generate a different error if the only uncompleted job at the end is the test lambda 3) Actually let the test hang forever (this is probably not optimal imo since it's somewhat easy to trigger and hard to debug)

streetsofboston commented 5 years ago

@objcode If the code were regular non-suspendable code, just plain blocking code, writing this would hang the test forever as well:

fun test() {
    val flowable = Flowable.never<Unit>()
    flowable
        .firstOrError().blockingGet()
}

The above test would never return.

You could make the case that if a non-suspending piece of code never completes, a corresponding suspending piece of code should never resume (and when wrapping that in a runBlockingXXX { ... } block, the code would never return).

Also, a 30 seconds timeout may not be enough for some tests... should the timeout be configurable?

In the end, it is a matter of taste 😄 Hanging tests are not good, but when regular non-suspending code never completes, the test will hang as well. Should that be the same for suspending code inside a runBlockingTest { ... } block or should we use a timeout, keeping in mind that a plain runBlocking { ... } will hang forever?

premnirmal commented 5 years ago

Any update on when this fix will be released?

nschwermann commented 5 years ago

Having this issue as well, Seems it only happens if touching files in android framework or that contain livedata not quite sure...

premnirmal commented 5 years ago

Happens to my tests (not using livedata). Here are the tests: https://github.com/premnirmal/StockTicker/blob/master/app/src/test/kotlin/com/github/premnirmal/ticker/network/StocksApiTest.kt I have to use runBlocking right now instead of runBlockingTest until this is fixed

kimble commented 4 years ago

Here is another minimal test case:

@Test
fun demo() {
    val scope = TestCoroutineScope()

    scope.runBlockingTest {
        val message = suspendCancellableCoroutine<String> { c ->
            thread(start = true) {
                Thread.sleep(10)
                c.resume("Hello..")
            }
        }

        assertEquals("Hello..", message)
    }
}

This fails with

java.lang.IllegalStateException: This job has not completed yet

at kotlinx.coroutines.JobSupport.getCompletionExceptionOrNull(JobSupport.kt:1188)
at kotlinx.coroutines.test.TestBuildersKt.runBlockingTest(TestBuilders.kt:53)
at kotlinx.coroutines.test.TestBuildersKt.runBlockingTest(TestBuilders.kt:72)
thesurlydev commented 4 years ago

I just ran into this issue.

Any update on a permanent fix for the issue or suggested workarounds would be appreciated.

nschwermann commented 4 years ago

Just use fun test() = runBlocking{} instead of TestCoroutineScope

On Sat, Jan 11, 2020, 6:40 PM Shane Witbeck notifications@github.com wrote:

I just ran into this issue.

Any update on a permanent fix for the issue or suggested workarounds would be appreciated.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/Kotlin/kotlinx.coroutines/issues/1204?email_source=notifications&email_token=AAGZSBJMQXXQS5HWZZBYSVLQ5JRG5A5CNFSM4HNWA4R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWOPDQ#issuecomment-573368206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGZSBOBG7HKCV7GUYNT5KDQ5JRG5ANCNFSM4HNWA4RQ .

thesurlydev commented 4 years ago

Saw that workaround after I posted. Thanks!

caiodev commented 4 years ago

Having the same issue. But temporarily one can use runBlocking without having any issues

DanielAsher commented 4 years ago

I've run into this issue testing a long running time-line schedule. Using runBlocking is not an option for me as the test would run too long.

LouisCAD commented 4 years ago

To have your test run quicker, you can implement your own testing dispatcher, implementing Delay, where you would divide the requested time by the desired amount.

On Fri, Jan 24, 2020 at 12:20 PM Daniel Asher notifications@github.com wrote:

I've run into this issue testing a long running time-line schedule. Using runBlocking is not an option for me as the test would run too long.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Kotlin/kotlinx.coroutines/issues/1204?email_source=notifications&email_token=ABVG6BIX4FDYC5ZEQYSPRVDQ7LFG7A5CNFSM4HNWA4R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJ2POPQ#issuecomment-578090814, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVG6BMISUFUNO2TMKTXUUTQ7LFG7ANCNFSM4HNWA4RQ .

Fakher-Hakim commented 4 years ago

I had the same issue, turned be that I call the room query in my code in Dispatchers.IO Try it without defining any Dispatcher, Room run query in main-safe way!!

sanders9 commented 4 years ago

Hi guys, I published a post on stackoverlow and just saw your response about this issue. I tried to replace the runBlockingTest by runBlocking but my test seems to wait in an infinite loop. Can someone help me pass this unit test please?

lupangsogood commented 4 years ago

Just use fun test() = runBlocking{} instead of TestCoroutineScope On Sat, Jan 11, 2020, 6:40 PM Shane Witbeck @.***> wrote: I just ran into this issue. Any update on a permanent fix for the issue or suggested workarounds would be appreciated. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1204?email_source=notifications&email_token=AAGZSBJMQXXQS5HWZZBYSVLQ5JRG5A5CNFSM4HNWA4R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWOPDQ#issuecomment-573368206>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGZSBOBG7HKCV7GUYNT5KDQ5JRG5ANCNFSM4HNWA4RQ .

This work for my situation, Thnak you.

iRYO400 commented 4 years ago

Hey everyone! I struggled with this exception too, but today found solution to avoid it. Add this dependency if you don't have it testImplementation "androidx.arch.core:core-testing:$versions.testCoreTesting" (2.1.0)

Then in your test class declare InstantTaskExecutorRule rule:

    @get:Rule
    val liveDataRule = InstantTaskExecutorRule()

Now runBlockingTest won't throw IllegalStateException: This job has not completed yet

P.S. Yep, I know that this rule usually used for LiveData testing. P.P.S. Would be cool, if someone will explain why it works :D

sohaghareb commented 4 years ago

I have the same issue any update ?

benny1611 commented 4 years ago

Still having the same issue...

fgiris commented 4 years ago

If anyone interested in a temporary solution ➡️ If you have only one element expected, Flow.first() can be used. Otherwise, I am using a helper method like below for multiple emissions until this issue is fixed. It has a very similar implementation with Flow.first() but returning a list of data and it only works if the number of expected elements are known.

suspend fun <T> Flow<T>.take(count: Int): List<T> {
    var results: MutableList<T>? = null
    try {
        collectIndexed { index, value ->
            if (index == 0) result = mutableListOf()
            results?.add(value)

            // If the number of  requested data is collected, finish the collection
            if (index + 1 == count) throw AbortFlowException() // Defined my own AbortFlowException
        }
    } catch (e: AbortFlowException) {
        // Do nothing
    }

    if (results == null) throw NoSuchElementException("Expected at least one element")
    return results!!.toList()
}

@Test
fun testSomething() {
  val results = fetchSomethingAsFlow().take(2) // Takes the first 2 elements emitted from flow
  assert(results[0] == firstExpectedValue)
  assert(results[1] == secondExpectedValue)
}
erikhuizinga commented 4 years ago

Another minimal reproducible example:

class CrashTest {

    @Test
    fun `this test passes!`() = runBlockingTest {
        assertNull(flowOf(null).single())
    }

    @Test
    fun `this test crashes!`() = runBlockingTest {
        assertNull(MutableStateFlow(null).single())
    }
}

This is on Kotlin 1.3.72 and Coroutines 1.3.8.

Workaround: like said in the previous comment, using first() instead of single() doesn't crash.


❗ I assume that the underlying mechanics of the crash are due to the same issue. If not, please create a separate issue for this, or let me know: then I'll create a separate issue. This could be a problem with (Mutable)StateFlow instead of being related to this issue.

audkar commented 4 years ago

This works as expected and actually reports bug/issue in your code.

flowOf(null) != MutableStateFlow(null)

First one emits one item AND completes while second just emits one item and continues to run

You could see difference if run these cases in blocking manner:

runBlocking {
  flowOf(null).collect{}
}
System.out.println("hello. You can see me")
runBlocking {
  MutableStateFlow(null).collect{}
}
System.out.println("Deadloc, I am never visible")
audkar commented 4 years ago

Workaround: like said in the previous comment, using first() instead of single() doesn't crash.

And test correctly reports bug in code. Because single() expects to get one item and completion while first() just one item. In case of MutableStateFlow(null) completion is not sent.

erikhuizinga commented 4 years ago

Workaround: like said in the previous comment, using first() instead of single() doesn't crash.

And test correctly reports bug in code. Because single() expects to get one item and completion while first() just one item. In case of MutableStateFlow(null) completion is not sent.

Thanks for the explanation. As far as I could conclude, I understood the same. However, it remains a bit confusing, because both flows are ultimately of the same Flow supertype, but their behaviour is completely different. That is simply inheritance at play, but what I mean is that it is not very clearly documented that the state flow doesn't complete automatically (or can it?), so some Flow operators have no meaningful use, like single() (among some other operators that are well documented).

I'm eager to see how this will (soon?) change when SharedFlow is introduced, as well as operators to convert flows to state flows or shared flows. These kinds of implementation-specific behavioural details must be very well documented, as it seems from my usage mistake.

gildor commented 4 years ago

but their behaviour is completely different.

Yes, but what is surprising here? Flow has contract, that can emit 0..Infinity number of items, it may complete or may never complete or may return an error, so all those behaviors are implementation details of a particular flow And you should design your test in a way which checks your expected behavior (so do you expect for Flow to complete or not

fluidsonic commented 3 years ago

I cannot test kotlinx-coroutines-reactive-based coroutines due to this limitation. Still an issue in 1.4.2. There seems to be no progress in the PR related to this issue for more than a year.

Is there any workaround to make the test wait a little for such coroutines to complete?

fluidsonic commented 3 years ago

I found an ugly workaround 😅

launch { delay(100); repeat(100) { Thread.sleep(10); yield() } }

I call that right before calling my reactive-based coroutines. That gives these coroutines a chance to complete their work before the test dispatcher is responsive again. The coroutines' execution is back on the test dispatcher by the time it is unblocked again. The initial delay gives the coroutines a chance to start before the dispatcher gets blocked.

fluidsonic commented 3 years ago

Another workaround is to use runBlocking { … } selectively within your runBlockingTest { … } code. Likely won't work if you launch new coroutines in the test dispatcher but works great when waiting for coroutines on other dispatchers.

NXAN2901 commented 3 years ago

@fluidsonic, I follow this article and it works for me, you can try it.

If you need example, you can check this

fluidsonic commented 3 years ago

Thank you @NXAN2901. Unfortunately that doesn't help in my case. When using kotlinx-coroutines-reactive it uses suspendCancellableCoroutine under the hood which is not supported for testing as per this issue. Changing the coroutine context won't work around this.

gs-ts commented 3 years ago

any update on this?

alan-camilo commented 3 years ago

Similar to @fluidsonic, I need to test a suspend method which calls another suspend method that uses suspendCancellableCoroutine. Changing the dispatcher of the UT is not helping, runBlocking is leaving me with an infinite loop, runBlockingTest and testDispatcher.runBlockingTest throw me the "This job has not completed yet" error. Hope it will be fixed, the thread is almost 2 years old

EDIT: in my case the fault was in a badly mocked class, using runBlocking is enough

caiodev commented 3 years ago

@alan-camilo What do you think about making a cake and throwing a party to celebrate its anniversary? You guys coming? Drinks on me

alan-camilo commented 3 years ago

sorry @caiodev due to the covid-19 situation, I'm sure a party will not be possible in May, maybe we can do a Zoom party?

caiodev commented 3 years ago

That works as well 😎

laim2003 commented 3 years ago

@fluidsonic testing asynchronous code is cancer as long as they don't fix it...

LouisCAD commented 3 years ago

@laim2003 JUnit, kotest and other testing frameworks allow you to generate tests dynamically.

When you combine this capability with a single entry-point with runBlocking, you can run tests against asynchronous code in parallel so long as there's no shared mutable state between the tests. This is also closer to real-world conditions, no hacks involved, so you test your code better than with hacks that don't happen when your final program runs.

So, IMO, saying testing asynchronous code is cancer is a bit of an overstatement. I think that with Kotlin, we actually have the best facilities in the industry for this, despite that rarely usable runBlockingTest function.

BTW, if you want to test flows, here's a very good third-party library made from the renowned devs from Square: https://github.com/cashapp/turbine

laim2003 commented 3 years ago

@LouisCAD

@laim2003 JUnit, kotest and other testing frameworks allow you to generate tests dynamically.

When you combine this capability with a single entry-point with runBlocking, you can run tests against asynchronous code in parallel so long as there's no shared mutable state between the tests. This is also closer to real-world conditions, no hacks involved, so you test your code better than with hacks that don't happen when your final program runs.

So, IMO, saying testing asynchronous code is cancer is a bit of an overstatement. I think that with Kotlin, we actually have the best facilities in the industry for this, despite that rarely usable runBlockingTest function.

BTW, if you want to test flows, here's a very good third-party library made from the renowned devs from Square: https://github.com/cashapp/turbine

honestly my reply came from a short-term frustration so yes it was an overstatement. But thanks for the recommendations anyway

samg-stripe commented 3 years ago

I'm also using kotlinx-coroutines-reactive and found that using the executor of the TestCoroutineDispatcher to create a scheduler for RxJava subscriptions allowed me to use runBlockingTest:

private val testDispatcher = TestCoroutineDispatcher()
private val scheduler = Schedulers.from(testDispatcher.asExecutor())

@Test
fun someTest() = testDispatcher.runBlockingTest {
    val x = SomeClassThatUsesRxJava(scheduler)  // Use this scheduler with Observable::subscribeOn
    x.awaitSomething()
}

Of course, this means that you must use this same scheduler throughout the class you're testing.

thaibt commented 3 years ago

Hi guys, I published a post on stackoverlow and just saw your response about this issue. I tried to replace the runBlockingTest by runBlocking but my test seems to wait in an infinite loop. Can someone help me pass this unit test please?

Same issue with this as of May 12. runBlockingTest throws error with job never completed. runBlocking results in the stuck being stuck in an infinite wait after the mocks are created.

EDIT: Found a way to bypass though by instead of Mocking my Mono/Flux requests. I gave it values instead so my awaitFirstOrNull method would actually return.

siralam commented 3 years ago

Injecting dispatcher worked for me. The main point is to make sure you are using the same test dispatcher instance in the block inside runBlockingTest { }.

For my project, we have DI library to inject dispatchers, and when you override the dispatcher in tests, make sure you are using the same instance instead of creating new instances every time you ask for a dispatcher.

And then when you call runBlockingTest, make sure you call from your test dispatcher instance, e.g. coroutineRule.testDispatcher.runBlockingTest { ... } instead of just runBlockingTest {...}.

I spoke more on this SO thread.

thanhnh98 commented 3 years ago

You can get something from this issue

https://github.com/kotlin-hands-on/intro-coroutines/issues/4