Hookup MvRxMocker and add tests #81
Conversation
| @@ -79,8 +78,6 @@ internal open class MvRxStateStore<S : Any>( | |||
| * all of the code required. | |||
| */ | |||
| fun set(stateReducer: S.() -> S) { | |||
| if (mocker != null) return | |||
There was a problem hiding this comment.
shouldn't we prevent other state from being set? I suppose it doesn't really matter, since we will still override it with the mocked state, but if other things set state I think it will trigger view invalidations and subscribe callbacks, which may interfere with the tests
|
|
||
| fun <S : MvRxState> setMockedState(viewModel: BaseMvRxViewModel<S>, state: S) { | ||
| mockedState[viewModel] = state | ||
| fun <S : MvRxState> setMockedState(viewModel: BaseMvRxViewModel<S>, state: S) { |
There was a problem hiding this comment.
maybe state should be nullable so the mocked state can be cleared?
| @@ -1,23 +1,21 @@ | |||
| package com.airbnb.mvrx | |||
|
|
|||
| import android.os.Parcelable | |||
|
|
|||
|
|
|||
| object MvRxMocker { | |||
There was a problem hiding this comment.
can you add some documentation to this file? always good to have it on the class and the methods to explain usage a bit
| private val mockState = TestState("updated", 1) | ||
|
|
||
| @Test | ||
| fun testDisabled() { |
| fun <S : MvRxState> setMockedStateFromArg(viewModel: BaseMvRxViewModel<S>, args: Parcelable) { | ||
| mockedState[viewModel] = TODO() | ||
| fun <S : MvRxState> setMockedStateFromArgs(viewModel: BaseMvRxViewModel<S>, args: Any?) { | ||
| mockedState[viewModel.stateStore] = _initialStateProvider(viewModel.state::class.java, args) |
There was a problem hiding this comment.
can you chain this to setMockedState?
|
|
||
| fun <S : MvRxState> setMockedState(viewModel: BaseMvRxViewModel<S>, state: S) { | ||
| mockedState[viewModel] = state | ||
| fun <S : MvRxState> setMockedState(viewModel: BaseMvRxViewModel<S>, state: S) { |
There was a problem hiding this comment.
I think we may want to have the viewmodel trigger it's subscriber callbacks so they can act on the new state - that seems important for tests that want to test state transitions.
not immediately important - could be a follow up
There was a problem hiding this comment.
yeah that makes sense will address in another PR
|
@elihart updated with your feedback and changed the base branch |
There was a problem hiding this comment.
@mmcwong nice! just two minor things.
@BenSchwab @hellohuanlin could you also take a look?
| initialState: S | ||
| ) : Disposable { | ||
|
|
||
| internal val isMocked = MvRxMocker.enabled |
There was a problem hiding this comment.
I think you need get() =, otherwise this won't update
| @@ -72,6 +78,8 @@ internal open class MvRxStateStore<S : Any>(initialState: S) : Disposable { | |||
| * all of the code required. | |||
| */ | |||
| fun set(stateReducer: S.() -> S) { | |||
| if (MvRxMocker.getMockedState(this) != null) return | |||
There was a problem hiding this comment.
do you see reasons to only block this if mocked state is set? I think it should be just
if (isMocked)
Because the view model or fragment might do things to set state before the mock is in place (the mock can only be set once the view model is created). Since setting state is async and can trigger subscribe callbacks it would be nice to eliminate any side effects from that
| /** | ||
| * Enable or disable whether the viewModels use mocked states | ||
| */ | ||
| var enabled: Boolean = false |
There was a problem hiding this comment.
Using static variables can cause a lot of headaches for tests. This will not be reset between tests. So if some test turns this on and forgets to turn it off it will stay on for all remaining tests.
|
Closing as #88 is a more testable approach |
This PR builds on to #72
It finishes up setting state directly and through args.
It also adds a test for the
MvRxMocker@elihart