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

Not using lifecycle aware observer for unit tests #235

Merged
merged 6 commits into from May 3, 2019

Conversation

@kojadin
Copy link
Contributor

kojadin commented Apr 24, 2019

Using MvRxTestRule to force not using life cycle aware observable for unit tests.
Fixes #198

@kojadin kojadin changed the title Not using lifecycle aware observer for unit tests (airbnb#198) Not using lifecycle aware observer for unit tests Apr 24, 2019
if (deliveryMode is UniqueOnly) {
activeSubscriptions.remove(deliveryMode.subscriptionId)
private fun <T : Any> Observable<T>.resolveSubscription(
subscriber: (T) -> Unit,

This comment has been minimized.

Copy link
@tasomaniac

tasomaniac Apr 24, 2019

Contributor

Small comment: I would keep the order of these params consistent with the above functions.

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 25, 2019

Author Contributor

Make sense.

@@ -33,6 +33,7 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
private val stateStore: MvRxStateStore<S> = RealMvRxStateStore(initialState)
) : ViewModel() {
private val debugMode = if (MvRxTestOverrides.FORCE_DEBUG == null) debugMode else MvRxTestOverrides.FORCE_DEBUG
private val forceUserTestObserver = MvRxTestOverrides.FORCE_USE_TEST_OBSERVER

This comment has been minimized.

Copy link
@elihart

elihart Apr 24, 2019

Contributor

looks like a spelling mistake user instead of use

Is it necessary to add a property instead of referencing FORCE_USE_TEST_OBSERVER directly?

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 25, 2019

Author Contributor

Hm, actually on second thought , maybe I should call this FORCE_DISABLE_LIFECYCLE_AWARE_OBSERVER .
About using directly flag I was just trying to fallow same code style as you guys have for debugMode . I see that is a bit different in this case because of conditional. I don't have strong opinion on this, whatever you guys want.

This comment has been minimized.

Copy link
@gpeal

gpeal Apr 25, 2019

Collaborator

@kojadin you can use it directly inline and the new name sounds good 👍

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 26, 2019

Author Contributor

👍 Done

Copy link
Collaborator

gpeal left a comment

This seems pretty reasonable to me. Can you include some tests to both test and demonstrate the usage of this?

@BenSchwab @elihart What do you think?

@@ -33,6 +33,7 @@ abstract class BaseMvRxViewModel<S : MvRxState>(
private val stateStore: MvRxStateStore<S> = RealMvRxStateStore(initialState)
) : ViewModel() {
private val debugMode = if (MvRxTestOverrides.FORCE_DEBUG == null) debugMode else MvRxTestOverrides.FORCE_DEBUG
private val forceUserTestObserver = MvRxTestOverrides.FORCE_USE_TEST_OBSERVER

This comment has been minimized.

Copy link
@gpeal

gpeal Apr 24, 2019

Collaborator

typo: forceUseTestObserver

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 25, 2019

Author Contributor

Actually, I was thinking to rename to forceDisableLifecycleAwareObserver and flag to FORCE_DISABLE_LIFECYCLE_AWARE_OBSERVER . WYT?

@@ -32,6 +32,7 @@ class MvRxTestRule(
RxAndroidPlugins.setMainThreadSchedulerHandler { Schedulers.trampoline() }
if (setRxImmediateSchedulers) setRxImmediateSchedulers()
MvRxTestOverridesProxy.forceMvRxDebug(debugMode.value)
MvRxTestOverridesProxy.forceUseTestObserver(true)

This comment has been minimized.

Copy link
@gpeal

gpeal Apr 24, 2019

Collaborator

Did you intentionally make this not configurable? You can include it in the TestRule constructor as an optional and default-true param.

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 25, 2019

Author Contributor

I was thinking to make it configurable, similar as for debugMode, but I could think on case when you will need lifecycle aware observer for unit tests. You can't use MvRxTestRule for automation tests because everything would happen in main thread. Maybe robolectric ? WYT ?

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 25, 2019

Author Contributor

Actually on second look Rx immediate schedulers are also configurable, so I make this also configurable.

@kojadin

This comment has been minimized.

Copy link
Contributor Author

kojadin commented Apr 25, 2019

Tests added.


init {
viewModel.subscribe {
subscribeCallCount++

This comment has been minimized.

Copy link
@gpeal

gpeal Apr 25, 2019

Collaborator

Would this not have been called before without your TestRule? Subscribe from inside of a viewModel isn't lifecycle aware because the ViewModel itself only has one lifecycle: onCleared. I expected this to be used to tests subscribing from the test itself.

This comment has been minimized.

Copy link
@BenSchwab

BenSchwab Apr 26, 2019

Contributor

This is actually a fragment, I think the fact that it is in init is confusing, but I understand that's probably easier than driving lifecycle in the tests.

This comment has been minimized.

Copy link
@tasomaniac

tasomaniac Apr 26, 2019

Contributor

So, this was the exact problem. This subscribe is an extension function available inside MvRxView. Without the TestRule, this callback was never called in unit tests.

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 26, 2019

Author Contributor

Exactly, this is a view and it would not be called before. There is a test MvRxTestRuleTestLifecycleAwareObserverEnabled where you can see that this wouldn't be called. The reason I used init is because I was trying to be consistent and use same approach that you guys have in TestRuleViewModel

This comment has been minimized.

Copy link
@gpeal

gpeal Apr 26, 2019

Collaborator

Got it. Thanks for the explanation

Copy link
Contributor

BenSchwab left a comment

Seems reasonable to me

* This should only be set by the MvRxTestRule from the mvrx-testing artifact.
*
* This can be used to force MvRxViewModels to disable lifecycle aware observer for unit testing.
* This is Java so it can be packgage private.

This comment has been minimized.

Copy link
@BenSchwab

BenSchwab Apr 26, 2019

Contributor

typo packgage

This comment has been minimized.

Copy link
@kojadin

kojadin Apr 26, 2019

Author Contributor

Fixed 👍

@gpeal gpeal merged commit 10febde into airbnb:master May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.