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

910: Integrate WorkerTester with TestScope #918

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

steve-the-edwards
Copy link
Contributor

@steve-the-edwards steve-the-edwards commented Feb 6, 2023

Closes #910. We do not allow a CoroutineContext/CoroutineDispatcher to be passed in, instead favouring keeping the API for WorkerTester.test self contained. Within the test though you can now access the TestCoroutineScheduler to control virtual time.

We drive the flow backing the Worker with a Turbine rather than our own handle around a channel.

@steve-the-edwards steve-the-edwards force-pushed the sedwards/910-tester-refactor branch from f927ad7 to d1c2fac Compare February 6, 2023 17:33
* This can be used to advance virtual time for the [CoroutineDispatcher] that the the Worker's
* flow is flowing on.
*/
public val testCoroutineScheduler: TestCoroutineScheduler
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added to control virtual time on the scope in which the Turbine is launched and thus where the Worker's upstream flow is producing values.

@@ -31,7 +39,7 @@ public interface WorkerTester<T> {
/**
* Suspends until the worker emits an output or finishes.
*
* Throws an [AssertionError] if an output was emitted.
* Throws an [AssertionError] if an output was emitted or the Worker has an error.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prior to this change, we would not throw if there was an exception (we counted that as 'finished'). I changed that, in favour of explicitly requiring the Worker to complete unexceptionally as a Turbine would.

This seemed the best choice to me. WDYT @RBusarow @rjrjr ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, I agree.

if (!channel.isEmpty && !channel.isClosedForReceive) {
try {
expectNoEvents()
} catch (e: AssertionError) {
throw AssertionError("Expected no output to have been emitted.")
}
}

override suspend fun assertFinished() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This mapping is the most complicated in order to get the same error messages in the AssertionError as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems worth saying so in a comment, e.g. "Complicated because these messages predate our Turbine integration and we wanted to keep them stable."

@steve-the-edwards steve-the-edwards marked this pull request as ready for review February 6, 2023 17:56
if (!channel.isEmpty && !channel.isClosedForReceive) {
try {
expectNoEvents()
} catch (e: AssertionError) {
throw AssertionError("Expected no output to have been emitted.")
}
}

override suspend fun assertFinished() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems worth saying so in a comment, e.g. "Complicated because these messages predate our Turbine integration and we wanted to keep them stable."


worker.test {
assertNoOutput()
assertNotFinished()

testDispatcher.scheduler.advanceTimeBy(999)
testDispatcher.scheduler.runCurrent()
testCoroutineScheduler.advanceTimeBy(999)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Great argument in favor of the API change.

**Note this contains minor BREAKING changes to the WorkerTester API**

Closes #910
@steve-the-edwards steve-the-edwards force-pushed the sedwards/910-tester-refactor branch from d1c2fac to 00b3789 Compare February 6, 2023 19:54
@steve-the-edwards steve-the-edwards merged commit f750b5b into main Feb 6, 2023
@steve-the-edwards steve-the-edwards deleted the sedwards/910-tester-refactor branch February 6, 2023 20:32
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.

Extend WorkerTester API to allow passing in a TestDispatcher or TestScheduler
3 participants