Add a a way to disable automatic delay skipping with runBlockingTest

[matejdro]

In many cases it would be useful to accurately control time within tests (for example when testing time-based coroutine logic). However, at the moment runBlockingTest seems to enforce delay-skipping behavior.

There does not seem to be a good alternative to this method at the moment if I don't want delay skipping. I can manually create TestCoroutineScope but that takes some boilerplate and does not bring all the benefits of runBlockingTest (such as making sure all launched tasks have been properly stopped).

At the moment I'm using copy paste version of runBlockingTest with dispatcher.advanceUntilIdle() removed, but that is really bad solution.

[elizarov]

I think pauseDispatcher call is doing what you are looking for as explained in the readme: https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/README.md
Does it help?

[matejdro]

pauseDispatcher also pauses immediate actions, not just time-delayed ones.

Plus, runBlockingTest manually calls dispatchers.advanceUntilIdle() which appears to advance regardless of paused status.

[elizarov]

I'm not sure what you are trying to achieve. What is your use-case? What kind of test are you trying to write?

[matejdro]

Let's say I have simple method like that:

fun CoroutineScope.showAnimationForTwoSeconds() = launch {
    animationController.startAnimation()

    delay(2000)
    animationController.stopAnimation()
}

Then I want to properly test this:

fun testAnimationStart() = runBlockingTest {
    showAnimationForTwoSeconds()
    
    assertThat(animationController).isAnimationRunning()

    advanceTimeBy(2000)
    assertThat(animationController).isAnimationNotRunning()
}

At the moment this test would fail since runBlockingTest skips all delays automatically, which means that stopAnimation call would be made immediatelly, before test can check whether animation is running.

[matejdro]

I just noticed that documentation states that manually launched coroutines should not auto-advance delays. However, this does not appear to be the case.

Copy pasting that example code fromTesting launch or async with delay section and removing advanceTimeBy call still prints both 1 and 2 with coroutines 1.2.1. Maybe this is a bug?

[streetsofboston]

What about using runBlocking and provide it a TestCoroutineDispatcher and then calling advanceTimeBy when needed on that dispatcher?

Or, call testCoroutineScope.launch and call advanceTimeBy on its dispatcher when needed (be sure to cast it to a TestCoroutineDispatcher).

[matejdro]

That works, but:

1. It creates a lot of boilerplate as opposed to just calling runBlockingTest
2. I loose automatic check for all children completion, which could cause hidden bugs.

[streetsofboston]

True...
To 'fix' issue (2.), what if you use a top runBlocking, then call launch(testCoroutineDispatcher) { ... }?

[elizarov]

cc @objcode

[objcode]

Ahoy!

Thanks for the issue let me take a look into this and make a longer reply.

Cheers,
Sean

[objcode]

Ok had time to look into this more - I wanted to write some tests and check on behaviors before posting a reply to ensure that there were no bugs uncovered here.

Auto-advancing after test completion

To start with, most of the issues raised above arise from confusion about exactly when runBlockingTest does auto-advancing of time. Here are all the situations where runBlockingTest may auto-advance time during test execution:

1. If the runBlockingTest coroutine itself calls delay time will auto-advance through the delay. If it did not, the next line of runBlockingTest would never be reachable. This was intentionally done to make test code that effectively calls this less surprising.

@Test
fun autoAdvance() = runBlockingTest {
    // 1: virtual-time 0ms
    delay(1_000)
    // 2: virtual-time 1_000ms; this line requires time auto-advance to be reached
}

2. If the runBlockingTest coroutine suspends waiting for another coroutine that has called delay. This is another version of the first - since if time did not auto-advance the test would have to fail or suspend forever in this case. However, this is less obvious.

@Test
fun autoAdvance_inLambda() = runBlockingTest {
    val job = launch {
        // 1: launch body is entered eagerly, but time is not auto-advanced
        delay(1_000)
        // 3: time is auto-progressed due to call to .join()
    }
    // 2: Launch body is suspended on delay, time is zero here
    job.join()
    // 4: virtual-time is 1_000ms, which must happen in order to execute this line
}

3. If the lambda passed to runBlockingTest has completed and there are any outstanding calls to delay. This is intentional to avoid common test cases failing by default with uncompleted coroutines (if the uncompleted coroutines checks were relaxed, this would not be necessary). The other alternative would be that many tests would need to call `advanceUntilIdle' as the last line.

@Test
fun advanceToAvoidLeaks() = runBlockingTest {
    launch {
        // 1: launch body is entered eagerly, time is not auto-advanced
        delay(1_000)
        // 3: after lambda passed to runBlockingTest completes, time is auto-advanced to 1_000
        // 4: this coroutine competes
    }
    // 2: test body completes - time is zero
} // 5: Test ends successfully, all jobs are completed

Should you fork runBlockingTest?

Yes! Please do - I do hope that people find it useful but if you have unique needs it's very reasonable to implement your own test coroutine builder. runBlockingTest is designed to be a entry point that has nice default behaviors, but may not meet everyones needs (especially in advanced use cases). That's why TestCoroutineScope, TestCoroutineDispatecher, and TestCoroutineExceptionHandler are exposed as supported API. It was an intentional goal to support using pieces of this library it to build alternative implementations as it sits in kotlinx-coroutines.

Before you do - take a look at some of the test cases in the repo that fail with this change to make sure you're happy with the API surface. There are a some tests that capture the surprising behaviors of runBlockingTest without the auto-time advancement.

Can we move some of the cleanup code into TestCoroutineDispatcher?

Maybe? Since the initial implementation I saw a neat trick of caching every Job that flows through a Dispatcher via dispatch, dispatchYield and scheduleResumeAfterDelay. This would add some execution overhead to TestCoroutineDispatcher but it seems promising. It would be pretty cool to move some of the coroutine leak logic to TestCoroutineDispatcher.cleanupTestCoroutines() instead of runBlockingTest (TestCoroutineDispatcher does what it can with the data structures available right now - but it lets suspended coroutines pass right through).

I'm not 100% sure this would work as a general solution, but it's worth exploring more.

Can we relax runBlockingTest to not call advanceUntilIdle?

Overall, I think this leads to a more surprising API - especially for novices. However, I'm interested in hearing use cases that are difficult or impossible to write tests for with the existing API. If there's concrete use cases that cannot be tested with the current API, it might be something that could be added as a CoroutineStart style option in advanced cases.

There's a relevant PR #1204 [WIP] to this discussion that adds an event loop after test completion by default to allow for multi-dispatcher tests (e.g. testing something that makes a 1ms database query on another dispatcher is possible after this PR). This is going the other direction to improve usability by allowing both wall-clock and virtual-time to progress.

That said, if there are use cases that are difficult or impossible to test in runBlockingTest I'd like to see if there's a way to make it all work together. It would be helpful to know which use cases are failing to explore possible solutions.

Specific test code in this thread:

I just checked and this test passes as expected:

@Test
fun simplifiedAnimationTest() {
    var state = 0
    fun CoroutineContext.subject() = launch {
        state = 1
        delay(1000)
        state = 2
    }

    runBlockingTest {
        subject()
        // state == 1
        advanceTimeBy(1_000)
        // state == 2
    }
}

Can you check it again and see if there's another issue that's triggering your failure?

The test code in the README works without calling advance* due to the auto-advance after test as mentioned above.

Hopefully this helped understand what's going on better! Looking forward to hearing what you think!

Cheers,
Sean

[matejdro]

Thanks for the writeup. runBlockingTest makes much more sense to me now.

I've done some testing and I've managed to isolate I've been facing in this code sample, which is a modification of your test above.

@Test(timeout = 1_000)
fun testAdvancement() {
    fun CoroutineScope.startAsync() = rxFlowable {
        while (isActive) {
            delay(10_000)
            println("Delay skipped...")
            send(Unit)
        }
    }

    runBlockingTest {
        val subscriber = startAsync().test()

        subscriber.assertValueCount(1)
        subscriber.dispose()
    }
}

with kotlinx-coroutines-rx2:1.2.2 added.

For me, above test will for some reason start skipping all delays at assertValueCount line (log is filled with Delay skipped lines), causing the test to descend into infinite loop. There is no advanceTime in this test and from what I can gather, none of the three situations from above apply.

Weirldy enough, assertValueCount is not suspending function and is even read-only function (just compares some numbers). If removed, tests succeeds fine.

[objcode]

That is unexpected. Thanks for the repro! Let me take a look to see if I can figure out what's going on.

[matejdro]

I've managed to shorten my repro case significantly and only using coroutines this time (no rx):

    @Test
    fun testAdvancement() {
        runBlockingTest {
            val extraJob = launch {
                while (isActive) {
                    delay(1_000)
                }
            }

            // Perform some assertion (just true in this case to simplify).
            // If assertion fails, test will start looping infinitely 
            if (true) {
                throw AssertionError()
            }

            extraJob.cancel()
        }
    }

Workaround is to put extraJob.cancel into try.. finally. Is that expected? This is part of the advanceToAvoidLeaks, but if I'm testing against coroutine that is being executed for a long time, doing something periodically, it becomes pain to test.

[dkhalanskyjb]

Looks like this issue is an instance of #1910.

[matejdro]

You are correct. Closing this as duplicate.