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

Small fixes in MyModelViewModelTest #121

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

Conversation

kworth
Copy link

@kworth kworth commented Oct 31, 2023

As things are currently, the MyModelViewModelTest gives the impression of being a useful starting point, but ultimately leaves a lot to be desired. It has 2 tests with different names that have the same logic, and as soon as you go to update the second test to do what it says it does, the FakeMyModelRepository implementation doesn't act as one would reasonably expect (you can call addMyModel all you want, but still the only thing that was ever emitted was an empty list).

This PR aims to make the class more correct and helpful. Please see individual commits which should be self-explanatory.

@JoseAlcerreca
Copy link
Contributor

Thanks for your PR and thanks for the catch!

Why Unconfined and not StandardTestDispatcher?

@kworth
Copy link
Author

kworth commented Nov 2, 2023

Good question. If Unconfined is not the right way to go, I'll fix that and keep that in mind in the future.

@kworth
Copy link
Author

kworth commented Nov 3, 2023

Actually, it looks like the tests fail using StandardTestDispatcher, but it looks like what we should be using is in fact a third thing, UnconfinedTestDispatcher. @JoseAlcerreca good to merge now?

@kworth
Copy link
Author

kworth commented Nov 13, 2023

@JoseAlcerreca any reason not to merge this in?

@JoseAlcerreca
Copy link
Contributor

Sorry, looking into it.

@JoseAlcerreca
Copy link
Contributor

Thanks, approved!

@kworth this needs to be replicated in the multimodule branch as well. Do you want to do it?

@kworth
Copy link
Author

kworth commented Nov 17, 2023

Ah, good point. I think I should be able to make time for that. I haven't looked at that branch at all yet. Hopefully it's not too divergent and will be a quick cherry-pick.

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.

2 participants