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

runBlockingTest doesn't cancel child coroutines when an assertion or exception fails the test #1910

Closed
mrf7 opened this issue Apr 8, 2020 · 7 comments
Assignees
Labels

Comments

@mrf7
Copy link

@mrf7 mrf7 commented Apr 8, 2020

If an assertion fails in a unit test, coroutines started using the scope of runBlockingTest do not get cancelled and can end up making the test hang forever.

This test never completes because the job inside launch isn't cancelled when Assert.fail() cancels the job running the test.

val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher()
@Test 
fun eternalTest() = testDispatcher.runBlockingTest {
    launch {
        while(isActive) {
            delay(1000)
        }
    }
    Assert.fail() // Or throw Exception()/ failed verify etc
}

This also applies to functions that launch coroutines on a scope passed as a parameter.

val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher()
@Test 
fun eternalTest() = testDispatcher.runBlockingTest {
    loopForever()
    Assert.fail() // Or throw Exception()/ failed verify etc
}
...
fun loopForever(){ 
    launch {
        while(isActive) {
            delay(1000)
        }
    }
}

Explicitly using cancel, however does properly cancel the TestCoroutineScope as well as all of its children

val testDispatcher: TestCoroutineDispatcher = TestCoroutineDispatcher()
@Test 
fun eternalTest() = testDispatcher.runBlockingTest {
    launch {
        while(isActive) {
            delay(1000)
        }
    }
  this.cancel()
}
@aknauf
Copy link

@aknauf aknauf commented Jun 13, 2020

I'll add that this behaviour is not consistent with that of runBlocking. The example below is probably redundant, given the excellent example provided by the OP, other than to show the test that uses runBlocking passes, while runBlockingTest causes this to fail.

coroutines v1.3.7
kotlin v1.3.72

class SomeException : Exception()

class RunawayTest {
    @Test
    fun `runBlocking should cancel coroutines on exception`() {
        var job: Job? = null
        assertFailsWith<SomeException> {
            runBlocking {

                job =launch {
                    while (true) {
                        delay(1000)
                    }
                }

                throw SomeException()
            }
        }

        assertTrue(job!!.isCancelled)
    }

    @Test
    fun `runBlockingTest should cancel coroutines on exception`() {
        var job: Job? = null
        assertFailsWith<SomeException> {
            runBlockingTest {

                job = launch {
                    while (true) {
                        delay(1000)
                    }
                }

                throw SomeException()
            }
        }

        assertTrue(job!!.isCancelled)

    }

}

@qwwdfsad qwwdfsad added the test label Jun 14, 2020
@qwwdfsad qwwdfsad self-assigned this Jun 14, 2020
@w2ji
Copy link

@w2ji w2ji commented Feb 5, 2021

Right now I am using a try / finally to stop the coroutine after assertion failed. I tried using assertFailsWith but doesn't seem like it's implemented yet? Have you found a solution on this? @mrf7

@luxmeter
Copy link

@luxmeter luxmeter commented Feb 9, 2021

This bug does affect all kind of exceptions. I throw an IllegalStateException and the coroutine das not stop. It feels like the delay has no affect and that the coroutine has no chance to react.

    @Test
    fun foo(): Unit = testCoroutineScope.runBlockingTest {
        launch {
            while(true) {
                delay(1000)
                LOG.debug("hello world")
            }
        }
        advanceTimeBy(1000)
        throw IllegalStateException("boom")
    }

@mrf7
Copy link
Author

@mrf7 mrf7 commented Feb 10, 2021

Right now I am using a try / finally to stop the coroutine after assertion failed. I tried using assertFailsWith but doesn't seem like it's implemented yet? Have you found a solution on this? @mrf7

It's been a long time since I raised this so I dont remember for sure but I don't think I found a real solution to it. If I remember right I just made some workaround that either avoids launching the coroutine in the test or manually cleans up the coroutine before failing the test.

@fluidsonic
Copy link

@fluidsonic fluidsonic commented Jun 15, 2021

I've also stumbled upon this.

runBlockingTest {
    launch {
        error("Fail.")
    }

    delay(1_000) // expected: IllegalStateException: Fail.

    Mutex(locked = true).lock() // actual: IllegalStateException: This job has not completed yet
}

Makes it hard to find the root cause of such errors in more complex scenarios.
Works well when using runBlocking instead.

@thuytrinh
Copy link

@thuytrinh thuytrinh commented Jun 23, 2021

I also encountered this issue yesterday. Spent a lot of hours narrowing down to this root cause. Turns out my business logic has a custom implementation of the throttle operation that uses a while & delay combo similar to below:

@Test
fun test() = runBlockingTest {
  launch {
    while (isActive) {
      delay(1000)
    }
  }
}

I'm thinking that, maybe runBlockingTest should have thrown an exception instead of hanging forever like that? @qwwdfsad

I'm using coroutine v1.4.2 & kotlin v1.4.30.

dkhalanskyjb added a commit that referenced this issue Nov 1, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
@joffrey-bion
Copy link
Contributor

@joffrey-bion joffrey-bion commented Nov 8, 2021

I also find this behaviour unexpected.

This is due to the fact that the test scope created by runBlockingTest does not have a Job, so structured concurrency doesn't apply and children are not cancelled.

A very easy way to workaround it is to use runBlockingTest(Job()) { ... }.

Are there any plans to fix it in the library? Or is this scope lacking a Job for a reason?

dkhalanskyjb added a commit that referenced this issue Nov 17, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
dkhalanskyjb added a commit that referenced this issue Nov 17, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
dkhalanskyjb added a commit that referenced this issue Nov 19, 2021
Implement a multiplatform runTest as an initial implementation of #1996.

Fixes #1204
Fixes #1222
Fixes #1395
Fixes #1881
Fixes #1910
Fixes #1772
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this issue Jan 28, 2022
This commit introduces the new version of the test module.
Please see README.md and MIGRATION.md for a thorough
discussion of the changes.

Fixes Kotlin#1203
Fixes Kotlin#1609
Fixes Kotlin#2379
Fixes Kotlin#1749
Fixes Kotlin#1204
Fixes Kotlin#1390
Fixes Kotlin#1222
Fixes Kotlin#1395
Fixes Kotlin#1881
Fixes Kotlin#1910
Fixes Kotlin#1772
Fixes Kotlin#1626
Fixes Kotlin#1742
Fixes Kotlin#2082
Fixes Kotlin#2102
Fixes Kotlin#2405
Fixes Kotlin#2462

Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

9 participants