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

Update Turbine to 0.13.0 #1035

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Update Turbine to 0.13.0 #1035

merged 1 commit into from
Jun 20, 2023

Conversation

steve-the-edwards
Copy link
Contributor

Update Turbine to 0.13.0; Update WorkerTester

assertNoOutput now fails if there is a completion of the WOrker's flow.

Update Turbine to 0.13.0; Update WorkerTester

assertNoOutput now fails if there is a completion of the WOrker's flow.
@steve-the-edwards steve-the-edwards merged commit 5b15d37 into main Jun 20, 2023
@steve-the-edwards steve-the-edwards deleted the sedwards/update-turbine branch June 20, 2023 20:52
worker.test {
assertNoOutput()
}
}

@Test fun `assertNoOutput fails after worker finishes without emitting`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

if worker finishes without emitting anything, then assertNoOutput should succeed right? as the name suggest it should check for no output

Copy link
Contributor

Choose a reason for hiding this comment

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

@steve-the-edwards I'm confused by this too.

Copy link
Contributor

@rjrjr rjrjr Jun 23, 2023

Choose a reason for hiding this comment

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

Actually, I'm not confused by the test method name, but by the semantic: why is it now an error for a worker to complete when we assertNoOutput()? This change broke some existing tests that were building workers via Observable.just(), for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you guys are right I should have made more noise about the change in this semantic. I just took the easy way out and inherited the change from expectNoEvents() when it started throwing on Completion after Turbine 0.3.0.

I've done a quick look and I'm not sure there is good way to reproduce the behavior we had before. @RotBolt could you create an issue to update this or rename the assertion to something like assertNoOutputOrCompletion()?

I think I am ok with having places where we use flowOf { 1 } or Observable.just(false) to start using assertFinished() instead of assertNoOutput(). But maybe that's a cop out?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's easy to replace Observable.just(false) with BehaviorRelay.createDefault(false). Is there a similar idiom for Flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

flow { emit(false); suspendCancellableCoroutine {} }

so tidy right?

hmm. must be something better

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.

4 participants