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

Add expectNoEvents to MoleculeTurbine #77

Closed
wants to merge 2 commits into from

Conversation

abyrnes
Copy link

@abyrnes abyrnes commented Feb 16, 2022

Please close if this is silly or useless, but I don't see another way of verifying this behavior other than expecting a timeout, which extends the runtime of a test by a full second.

Comment on lines 274 to 279
try {
expectNoEvents()
fail()
} catch (t: AssertionError) {
assertEquals(t.message, "Expected no events but found Item(kotlin.Unit)")
}
Copy link
Member

Choose a reason for hiding this comment

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

Please use assertFailsWith rather than try/fail/catch. Right now you're getting lucky in that the AssertionError that would be thrown by fail() is being caught but is failing in the message check. Would rather using something a bit safer.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@JakeWharton
Copy link
Member

This exists in Turbine so I'm okay duplicating it here.

I'll just comment once again for the record that this assertion is prone to race conditions and does not guarantee that no events will ever be produced in the future (anywhere from 1ms to the heat death of the universe). It has it's place, but no doubt someone will get upset when it's doesn't do exactly what they think.

@abyrnes
Copy link
Author

abyrnes commented Feb 16, 2022

This exists in Turbine so I'm okay duplicating it here.

I'll just comment once again for the record that this assertion is prone to race conditions and does not guarantee that no events will ever be produced in the future (anywhere from 1ms to the heat death of the universe). It has it's place, but no doubt someone will get upset when it's doesn't do exactly what they think.

Would it be worthwhile to mention this in the kdoc?

@mattprecious
Copy link
Collaborator

I don't think this is ever a valid assertion to make given the current implementation of the testing library. Since recomposition is delayed until you call awaitItem(), there's no way for expectNoEvents() to fail in any case except the first synchronous composition, which will always emit an item.

We've moved off of this testing library internally and are experimenting with a more aggressive recomposition strategy. Once that implementation settles I think we can push it upstream here.

@JakeWharton
Copy link
Member

Ah, yes. I thought we had a reason for not including it I just couldn't... remember.

@JakeWharton JakeWharton closed this Mar 4, 2022
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

3 participants