-
Notifications
You must be signed in to change notification settings - Fork 109
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
wip: use virtual time for timeout #269
base: trunk
Are you sure you want to change the base?
Conversation
@@ -208,7 +208,9 @@ class FlowInScopeTest { | |||
delay(1100.milliseconds) | |||
} | |||
}.testIn(this, timeout = 1500.milliseconds) | |||
turbine.awaitComplete() | |||
withContext(Default) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Use virtual time instead of wall time for turbine timeout.
Proof of concept for discussion in #268. This PR should not be merged since it is non-backwards compatible. It should probably be an option instead.
I only ran tests against jvm.