touchlab / KaMPKit

KaMP Kit by Touchlab. A collection of code & tools designed to get your mobile team started quickly w/Kotlin Multiplatform
https://touchlab.co/
Apache License 2.0
2.2k stars 199 forks source link

Refactor BreedModel and tests #190

Closed russhwolf closed 2 years ago

russhwolf commented 3 years ago

Things I'd like to change:

  1. Currently BreedModel assumes callers will sequentially call refreshBreedsIfStale() and getBreedsFromCache(). We should have a single function that coordinates things internally so callers (both platform viewmodels and tests) don't have to.
  2. Currently, getBreedsFromNetwork() returns a DataState, but this is actually only called internally inside refreshBreedsIfStale() which creates its own DataState. This gets a bit awkward because we need a when statement to convert the network call to the final state in refreshBreedsIfStale(), but not all cases of the when actually happen in practice (eg the loading state never triggers). We should wait to convert to a DataState until the last-mile function that gets called externally
  3. getBreedsFromCache() will never emit if the database is empty. If we always refresh before calling it then this won't be a problem, but callers need to know this.
  4. The naming and semantics of the test cases in BreedModelTest are inaccurate and confusing. staleDataCheckTest() asserts on errors that probably shouldn't be happening, and notifyErrorOnException() doesn't assert on any errors even though it expects them to happen ~5. KtorApiMock is currently a separate implementation of KtorApi. We could test our network/serialization logic more directly if it used DogApiImpl but passed a MockEngine to the httpClient.~ ~6. Should KtorApi be renamed DogApi?~
russhwolf commented 3 years ago

I did some partial work on this when starting #189, then pulled back because it was a bigger rabbit-hole than I realized. Was having trouble getting tests to pass with MockEngine and seeing some different behavior per-platform, so I'd like to do a bunch of cleanup and get this stuff all working better.

kpgalligan commented 2 years ago

Please review and see if still relevant. If not, close, otherwise, I guess let's chat or just do it if not huge?

russhwolf commented 2 years ago

Points 5 and 6 have been fixed but the rest of stuff in here is still outstanding