-
Notifications
You must be signed in to change notification settings - Fork 500
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
Testing and mocking infrastructure #282
Conversation
I think this pattern seems reasonable. RxJava has the RxJavaPlugins class that this feels similar to. Maybe we can unify this pattern with the existing debug pattern into a MvRxPlugin's class? It would be nice to define an interface for what is configurable, and then ship as much of the testing/mocking code as a separate artifact. |
@BenSchwab We already have a testing artifact we can use for this 😄 |
* This function should be preferred over calling [Observable.subscribe] directly so that disposal is handled for you, and so setting state | ||
* is simplified. | ||
*/ | ||
fun <T> Observable<T>.executeWithoutAsync(stateReducer: S.(value: T) -> S): Disposable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about just calling this subscribe
? Or something with that terminology. MvRx adds execute terminology and it has been working great. Not sure if that should be used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I prefixed this with execute was to include it in autocomplete. If people are used to using execute with mvrx, then executeWithoutAsync
is discoverable
import java.util.LinkedList | ||
|
||
// TODO: Is there a better way to set this? | ||
var mvrxViewModelConfigProvider = MvRxViewModelConfigProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from changing the debugMode boolean and changing properties, do you expect users to subclass the Provider? I realized that it is open.
Not sure how different it is but I would prefer keeping this Val and provide a function like below to configure further.
fun mvrxConfigure(configure: MvRxViewModelConfigProvider.() -> Unit) {
mvrxViewModelConfigProvider.configure()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think many people will need to subclass MvRxViewModelConfigProvider, but I left it open so that they could provide a custom implementation of buildStateStore
if desired.
Any particular reasons you like the function configuration better? It seems like it would make it harder to override functions like buildStateStore
, although we could have setter functions to override default behavior.
Needing to call a function like mvrxConfigure
is a little less discoverable than just setting the val though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do the function approach in case you have setters. It would look like a DSL and allow others to configure. I put that idea since some properties are variables already.
I think if you want to make it open and provide people to replace the whole implementation, then shouldn't it be immutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick 😊
/** | ||
* Used with [MockableMvRxView.provideMocks] for mocking a MvRx fragment that has no view models (eg only static content and fragment arguments). | ||
* | ||
* See https://airbnb.quip.com/4S6aAV3uzBRP/Testing-MvRx-Screens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment or change to public?
* Switch between using a mock view model store and a normal view model store. | ||
* | ||
* @param debugMode True if this is a debug build of the app, false for production builds. | ||
* When true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfinished comment
|
||
/** | ||
* Defines configuration for how ViewModels are created and what settings they use. | ||
* By default this applies debug settings to all ViewModels and SHOULD be overridden in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this will be easier to miss than the old setting which required you to specify a base view that handled this or do this every time. I feel like it would be safer to default to off, or to have an Unitialized value, and crash if something is not set by the time it is accessed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elihart @BenSchwab I like the idea of having it be uninitialized and crash with a good error message.
What we really don't want is for people to not read the docs (very common) then:
- Accidentally have debug mode always on
- Never have debug mode on
- Discover too late that it was never on, turn it on but have a bunch of random crashes that no one person can fix so they leave it off forever.
} | ||
} | ||
|
||
open fun <S : Any> buildStateStore( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KDoc since this is open?
onStateSetListeners.remove(callback) | ||
} | ||
|
||
fun addOnDisposeListener(callback: (MockableMvRxStateStore<*>) -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just take a disposable, that would be more thematically accurate with the rest of the reactive infrastructure
*/ | ||
@PublishedApi | ||
@VisibleForTesting | ||
internal fun freezeStateForTesting(state: S) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why the name "freeze", the freezing behavior is not controlled by this method, but rather set-up by the type of state store you use.
It is seems possible now that ScriptableStateStore
is not necessarily one that is frozen if I am understanding Mockable state store properly.
import com.airbnb.mvrx.mock.ConstructorCode | ||
import com.airbnb.mvrx.mock.MockPrinterConfiguration | ||
import com.airbnb.mvrx.mock.MockStateHolder | ||
import com.airbnb.mvrx.mock.MvRxMockPrinter.Companion.ACTION_COPY_MVRX_STATE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few unused imports.
*/ | ||
class MockedView<V : MvRxView>( | ||
val viewInstance: V, | ||
val viewName: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My ide marks these as unused
|
||
/** | ||
* Clear the stored mock info for the given view. This should be called after the view is done initializing itself with the mock. | ||
* This should be done to prevent the mock data from interfering with future views of the same type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of interference happens? This seems to be keyed on the view instance itself, so why does it matter that a future view could be of the same type? It would be a different view presumably (but even so, why would they mock differ - since a provideMocks function seems like it should be idempotent)
import io.reactivex.Observable | ||
import io.reactivex.schedulers.Schedulers | ||
|
||
interface MockableStateStore<S : Any> : ScriptableStateStore<S> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this scriptable state store needed any more? It seems equivalent to using the block action api and the freeze state call. There seems to be a lot of state stores, and it's a bit confusing to track all their different purposes.
val blockExecutions = | ||
config.currentMockBehavior?.blockExecutions ?: MockBehavior.BlockExecutions.No | ||
|
||
if (blockExecutions != MockBehavior.BlockExecutions.No) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can handle this in the state store? It seems like the concept of blocking etc. is a state store concept, not a view model concept.
03fe7a2
to
bc0c745
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good! let me know when you make the last pass to switch the ConfigFactory to injection and I can give it a final look.
* | ||
* This should be accessed in a background thread since this data may be loaded synchronously when accessed! | ||
* | ||
* TODO Access mocks from annotation generated code before falling back to dex approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worthwhile to make this configurable now for anyone who wants to manually leverage dagger multibinding to collect their mock view set?
// sure the parsing and display it tested as well. | ||
val isInitializing = | ||
mock.mock.isDefaultInitialization || mock.mock.isForProcessRecreation | ||
val finishAfterMs: Long = if (isInitializing) 3000 else 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be configurable, similiar to ViewModelEnabled
or use the same source of truth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if this can leverage some of the TTI concepts we use -- i.e. waiting until async props marked as necessary for load go to a terminal state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those are nice ideas for follow up work 👍
* This is left mutable so users can customize the default. Too short and it may not result | ||
* in an accurate mock (as requests can override the mock state). Too long and it may |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "too short" -- what factors are at play here? The length of running the init
function? It seems that this is used when BlockExecutions.Completely
is used, so any requests made during initialization are not fired. Or are these network requests triggered by non-mvrx code?
Idea for polish - We create an overlay in MvRxFragment that makes the UI opaque during this period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any requests made during initialization are not fired
- this is it. Basically what I am concerned about is some delayed behavior in initialization that triggers a state change or execution. Maybe I am being overly cautious here, since this generally shouldn't happen, but it seems safer.
@BenSchwab this is ready for review again. I have updated the mvrx testing module test rule to override mock behavior, and also cleaned up some other settings to decouple things more. I spent a while trying to make the config factory injectable into the view model, but in the end I really needed it to be static because the mocking system needs to be able to access it before the view model is created which made things messy. This makes the overall interface not as clean as we would perhaps like, but I think it is reasonable |
I've released this branch as a 2.0.0-alpha1 release - https://github.com/airbnb/MvRx/releases/tag/2.0.0-alpha1 I've added documentation for all the new features to the wiki, as well as details on what breaking changes this introduced. I am keeping this as an alpha to gather feedback on the new API's and breaking changes before we solidify them as a stable release. I plan to keep all this work in this branch, with updates added as PR's on top of this branch, until we are ready for stable (or possibly beta). This will allow us to easily continue to ship small updates if needed |
Because of #334 I need to pull the mocking code into a separate module since it relies on kotlin reflect. Will make a new release (including fixes to other reported issues) once that is done |
@elihart This can be closed right? |
I am migrating our internal testing/mocking tools to this open source repo.
Mock Behavior
At a high level the idea is that a view model and state store can have a
MockBehavior
applied to it when it is created. This allows things such as blocking state from changing, allowing synchronous state changes to be forced, blocking async executions, etc.I have found that it is necessary for testing infrastructure to be able to have hooks into when viewmodels and state stores are created and disposed, as well as allow mock behavior of existing state stores to be modified dynamically and globally. For example, the test system may need to globally toggle whether state changes are allowed. The approach I have taken to do that is to have a single
MvRxViewModelConfigProvider
which has a customizable mock behavior, creates state stores based on that behavior, and keeps references to active stores and viewmodels to allow toggling their behavior.I have created singletons to manage this configuration which are all under the
MvRx
object.Mock State
I've included a system for easily generating mock state via a script. Since these are run from the command line they aren't a part of the library that is shipped, but I have packaged it as an executable so users can easily download and use it.
MvRx launcher
This allows loading and screen/state in the app. To ease review, I've set this up as a separate PR onto this branch - #292
Unit Test Framework
A separate piece is a unit test framework for view models that builds on this mocking behavior. This work is still to come.
Fixes #265