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

wip: use virtual time for timeout #269

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

Conversation

mightyguava
Copy link

@mightyguava mightyguava commented Sep 24, 2023

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.

@@ -208,7 +208,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.

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.

None yet

2 participants