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.

Sign up for GitHub

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

Jump to bottom

wip: use virtual time for timeout #269

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Draft

wip: use virtual time for timeout #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dad41df
use virtual time for timeout
mightyguava Sep 24, 2023
570fe47
add a test
mightyguava Sep 24, 2023
7ba8b1f
add new tests suggested by @jingibus
mightyguava Sep 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/commonMain/kotlin/app/cash/turbine/channel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,7 @@ private suspend fun <T> withAppropriateTimeout(
timeout: Duration,
block: suspend CoroutineScope.() -> T,
): T {
return if (coroutineContext[TestCoroutineScheduler] != null) {
// withTimeout uses virtual time, which will hang.
withWallclockTimeout(timeout, block)
} else {
withTimeout(timeout, block)
}
return withTimeout(timeout, block)
}

private suspend fun <T> withWallclockTimeout(
Expand Down
12 changes: 6 additions & 6 deletions src/commonTest/kotlin/app/cash/turbine/ChannelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,15 +238,15 @@ class ChannelTest {
}

@Test fun awaitHonorsCoroutineContextTimeoutNoTimeout() = runTest {
withTurbineTimeout(1500.milliseconds) {
val job = launch {
neverFlow().collectIntoChannel(this).awaitItem()
}
withContext(Dispatchers.Default) {
withTurbineTimeout(1500.milliseconds) {
val job = launch {
neverFlow().collectIntoChannel(this).awaitItem()
}

withContext(Dispatchers.Default) {
delay(1100)
job.cancel()
}
job.cancel()
}
}

Expand Down
35 changes: 25 additions & 10 deletions src/commonTest/kotlin/app/cash/turbine/FlowInScopeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,16 @@ import kotlin.test.assertSame
import kotlin.test.assertTrue
import kotlin.time.Duration.Companion.milliseconds
import kotlin.time.Duration.Companion.seconds
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CompletionHandlerException
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.CoroutineStart
import kotlinx.coroutines.*
import kotlinx.coroutines.Dispatchers.Default
import kotlinx.coroutines.cancel
import kotlinx.coroutines.channels.Channel
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.emitAll
import kotlinx.coroutines.flow.emptyFlow
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.onCompletion
import kotlinx.coroutines.flow.onStart
import kotlinx.coroutines.launch
import kotlinx.coroutines.test.runTest
import kotlinx.coroutines.withContext

class FlowInScopeTest {
@Test fun multipleFlows() = runTestTurbine {
Expand All @@ -38,6 +30,27 @@ class FlowInScopeTest {
turbine2.awaitComplete()
}

@Test fun awaitFailsOnVirtualTime() = runTestTurbine {
assertFailsWith<AssertionError> {
flow<Nothing> {
awaitCancellation()
}.test {
awaitComplete()
}
}
}

@Test
fun awaitFailsOnDelayVirtualTime() = runTestTurbine {
assertFailsWith<AssertionError> {
flow<Nothing> {
delay(1500.milliseconds)
}.test(timeout = 1000.milliseconds) {
awaitComplete()
}
}
}

@Test
fun channelCancellation() = runTestTurbine {
kotlin.runCatching {
Expand Down Expand Up @@ -208,7 +221,9 @@ class FlowInScopeTest {
delay(1100.milliseconds)
}
}.testIn(this, timeout = 1500.milliseconds)
turbine.awaitComplete()
withContext(Default) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding these withContext(Default) calls here defeats the entire purpose of these tests.

In any of these tests that uses withContext(Default) within the test subject, the test subject is simulating the experience of an engineer whose code is broken.

If we also run awaitComplete() on the Default dispatcher, we're no longer simulating the experience of that engineer, becuase they won't be calling awaitComplete() on Default: they'll be calling it on TestScope's dispatcher.

This is a critical service that Turbine provides: unlike runTest's timeout, Turbine provides specific feedback on which expected output failed to emit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any of these tests that uses withContext(Default) within the test subject, the test subject is simulating the experience of an engineer whose code is broken.

To clarify this: it simulates this because the engineer literally has a test that is hanging awaiting some value, and their wall clock is ticking.

To me, an interesting test for your change would be something like this:

@Test fun awaitFailsOnVirtualTime() = runTestTurbine {
  assertFailsWith<AssertionError> {
    flow<Nothing> {
      delay(1500.milliseconds)
    }.test(timeout = 1000.milliseconds) {
      awaitComplete()
    }
  }
}

Or:

@Test fun awaitFailsOnVirtualTime() = runTestTurbine {
  assertFailsWith<AssertionError> {
    flow<Nothing> {
      awaitCancellation()
    }.test {
      awaitComplete()
    }
  }
}

...in other words, can we just ditch the wallclock time stuff and rely entirely on virtual time?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I really had no idea what the broken tests were for.

7ba8b1f adds both tests you suggested, both passing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we just ditch the wallclock time stuff and rely entirely on virtual time?

That would be really nice 😄 and also what I'm hoping for.

turbine.awaitComplete()
}
}

@Test fun awaitHonorsCoroutineContextTimeoutTimeout() = runTestTurbine {
Expand Down
37 changes: 32 additions & 5 deletions src/commonTest/kotlin/app/cash/turbine/FlowTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,6 +608,7 @@ class FlowTest {
flow<Nothing> {
delay(5.seconds)
}.test {
advanceTimeBy(5.seconds)
awaitComplete()
}
}
Expand All @@ -629,13 +630,13 @@ class FlowTest {
}

@Test fun awaitHonorsTestTimeoutNoTimeout() = runTest {
flow<Nothing> {
withContext(Default) {
delay(1100.milliseconds)
flow<Nothing> {
delay(1100.milliseconds)
}.test(timeout = 1500.milliseconds) {
awaitComplete()
}
}
}.test(timeout = 1500.milliseconds) {
awaitComplete()
}
}

@Test fun awaitHonorsCoroutineContextTimeoutTimeout() = runTest {
Expand Down Expand Up @@ -734,6 +735,32 @@ class FlowTest {
}
}

@Test
fun delaysCanBeTested() = runTest {
val took = measureTime {
flow {
delay(5000)
emit("1")
delay(5000)
emit("2")
}.test {
expectNoEvents()

advanceTimeBy(5000)
expectNoEvents()

runCurrent()
assertEquals("1", awaitItem())

val exception = assertFailsWith<AssertionError> {
awaitItem()
}
assertEquals(exception.message, "No value produced in 3s")
}
}
assertTrue(took < 1.seconds, "$took > 1s")
}

@Test
fun timeoutsAreCaptured() = runTest {
flow<Nothing> {
Expand Down