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

Explore paused mode only for TestCoroutineDispatcher. #3

Open
wants to merge 1 commit into
base: coroutines-test
Choose a base branch
from

Conversation

objcode
Copy link
Owner

@objcode objcode commented Oct 16, 2019

Trying out removing the immediate mode API from TestCoroutineDispatcher and relying on runBlockingTest's event loop to pump the test lambda.

Overall, change seems like it might be heading the right direction with a large API surface reduction.

However, the automatic time advancement for the main test lambda is still present which has been fairly confusing to developers.

Things that are better

  • By removing immediate mode major issues around thread hopping and out-of
    order execution in other event loops (e.g. Android Main thread) are resolved
  • Developers that still desire Unconfined behavior can use Dispatchers.Unconfined.

Things that are neutral

  • By exposing an API for waiting for new dispatches DelayController.waitForDispatcherBusy(timeoutMillis) developers can now interact with multithreading calls directly from TestCoroutineDispatcher. This API is not required for the runBlockingTest usage. This is important for some major parts of Android where thread checks are explict and must be satisfied. However, the API is less than obvious and it tends toward coupling tests with impl. details (possibly of transitive dependencies).

Things that are worse

  • The "launch and don't join" pattern will always require a call to runCurrent to execute the body of the coroutine.
  • To write correct tests of functions that call withContext depends on if the dispatcher's are equal. If so, no dispatch happens and no additional API calls are required. If they are not equal, then a call to waitForDispatcherBusy/runCurrent is required when using TestCoroutineDispatcher. This is alleviated in runBlockingTest since the execution order of the advanceTimeUntilIdle will join the withContext coroutine before continuing the test.
  • Because of the above, calling waitForDispatcherBusy after every function that calls withContext is not a safe invocation. It will timeout and throw an exception in some invocations. This is a confusing API. How to cleanup?
  • Dispatchers.Unconfined is incompatible with TestCoroutineScope.

@yorickhenning
Copy link

Looks improved to me.

Have you explored decoupling the test clock from the Dispatcher entirely, or at least only exposing methods that separate the tasks of advancing time and executing tasks?

A test pattern I see a lot is:

{
  @Inject val clock: FakeClock
  @Inject val testDispatcher: TestDispatcher

  @Test fun testWallTimeDependentTask() {
    objectUnderTest.doSomeSideEffectingAsynchronousWorkWithAWallTimeWait()
    clock.advance(farEnoughMs)
    testDispatcher.runCurrent() // This name confuses me a little, it implies it might only run at most one task? runAllCurrent()? runAll()?
    assertThat(objectUnderTest.observableProperty).sparkles()
  }
}

This also ensures that the duration of time the programmer expects the task to take gets verified. If the task isn't complete after farEnoughMs, the production code is taking longer than expected to complete, which is itself a bug.

testDispatcher.awaitIdle() can print out which tasks were waiting in its thrown TimeoutException, along with a message like "these tasks were scheduled, did you forget to advance time by so that they will run: "?

There are few unit tests that require wall time to elapse. When IO is faked it completes immediately, everything happens "now". When a rare test relies on wall time elapsing, I think it's important to make the programmer check that the duration is the expected one by advancing time by that amount in the unit test.

I've got a thoughts on the other points, hopefully I can get time to dig in more tomorrow.

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.

2 participants