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

Add UserProfile unit tests / MVP infrastructure #613

Merged
2 commits merged into from Mar 22, 2016

Conversation

Projects
None yet
4 participants
@ghost

ghost commented Mar 2, 2016

So, this just covers the screen where you view a user profile, but I need to solicit feedback before continuing any further, to make sure this is heading in the right direction, as this is a significant change to how we architect the UI.

The most significant change is moving towards an MVP architecture, where:

  • the "view" (a.k.a UI; views/fragments/activities) contains no logic (unless it's purely for rendering)
  • the "presenter", which contains all the logic/behaviour/decision-making
  • the "model", which interacts with the database/network layers.

Each UI component that needs any logic or data access will be paired with a presenter. The presenter should not know or care about any UI, network, or database implementation details; communicating with the view only through an abstract "view interface" (See UserProfilePresenter.ViewInterface) and with the network/database only through an interface (a.k.a. interactor/service/repository).

For this PR, I've considered the UserProfileFragment to be a unit, and paired it with a UserProfilePresenter. Each of which can be tested individually (See UserProfileFragmentTest and UserProfilePresenterTest).

In the future, we could consider Activities or custom Views to be units as well, with their own presenters, when it makes sense to do so.

I also implemented a rudimentary "observer" pattern to facilitate running background tasks from a presenter, and synchronizing them with the view.

The tests are not exhaustive, I can add more if we like how this looks.

Also partially addresses https://openedx.atlassian.net/browse/MA-2055

@ghost

This comment has been minimized.

ghost commented Mar 2, 2016

@BenjiLee @aleffert @1zaman Food for thought.

observeOnView(userProfileInteractor.observeAccount()).subscribe(new Observer<Account>() {
@Override
public void onData(@NonNull Account account) {
if (account.requiresParentalConsent() || account.getAccountPrivacy() == Account.Privacy.PRIVATE) {

This comment has been minimized.

@aleffert

aleffert Mar 2, 2016

Contributor

I think here is where we would want to use a view model, so we can make all this logic functional? I have some nits, but I don't think we're at that stage of the PR yet. I'd also be really happy to see a little conceptual documentation, or something that walked through this example a bit.

how-to-make-a-new-screen.txt

This comment has been minimized.

@ghost

ghost Mar 4, 2016

@aleffert I might prefer more methods rather than a concrete "ViewModel". But I gave it a try. Is this what you are imagining? 09ad164

@aleffert

This comment has been minimized.

Contributor

aleffert commented Mar 2, 2016

This looks good to me, except for my comment about a view model.

protected final Logger logger = new Logger(getClass().getName());
@Nullable

This comment has been minimized.

@BenjiLee

BenjiLee Mar 8, 2016

Member

Question: What makes this function Nullable?

This comment has been minimized.

@ghost

ghost Mar 8, 2016

Fragment's don't have to refer to a view.

See Fragment.java.

@BenjiLee

This comment has been minimized.

Member

BenjiLee commented Mar 8, 2016

Profile summary text does not populate.

@ghost

This comment has been minimized.

ghost commented Mar 8, 2016

@BenjiLee Sadly that seems to be some kind of weird Android bug. I didn't full track it down but I added a workaround 😢 58e3a6f

@BenjiLee

This comment has been minimized.

Member

BenjiLee commented Mar 9, 2016

This more or less LGTM.

How do we go about merging this? Should we test out all of the feature as well?

import java.util.concurrent.Callable;
import java.util.concurrent.Executor;
public enum AsyncObservable {

This comment has been minimized.

@aleffert

aleffert Mar 9, 2016

Contributor

Can you add some comments about use cases/contracts for these different observables?

if (selectedTopicIndex >= 0) {
topicsSpinner.setSelection(selectedTopicIndex);
}
// Otherwise, leave the default option, which is "Choose a topic..."

This comment has been minimized.

@aleffert

aleffert Mar 9, 2016

Contributor

Is this meant to be part of this change?

This comment has been minimized.

@ghost
@aleffert

This comment has been minimized.

Contributor

aleffert commented Mar 10, 2016

This is great. I'd love to see some architecture documentation so that we can give guidance on following these patterns.

Or maybe a wiki page, like "So you want to add a screen..."

@aleffert

This comment has been minimized.

Contributor

aleffert commented Mar 16, 2016

@bguertin so is this good to go?

@1zaman

This comment has been minimized.

Contributor

1zaman commented Mar 16, 2016

How about using RxJava, along with possibly RxAndroid, for the observable/subscription pattern? I haven't used it myself, but it seems to be a lot more versatile, and offers support for almost any kind of workflow. There's an adapter to it for Retrofit 2 as well (which by the way seems to have been released now, so we should upgrade to it at some point).

@1zaman

This comment has been minimized.

Contributor

1zaman commented Mar 16, 2016

Overall, this seems to be a very complex and granular setup, though I guess it does make unit testing simpler to do. As I suggested in MA-2060, a simple Loader framework, along with a generic Retrofit callback framework that posts to the event bus for global POST calls, would have solved all the actual issues with our controller interface. Still, if this is what everyone is agreed on, then I guess we can go ahead with it.

@aleffert

This comment has been minimized.

Contributor

aleffert commented Mar 16, 2016

@1zaman it's worth talking about the details (it may be that we're better off with RxJava even though it's a pretty heavy dependency, since other people are starting to be familiar with it) but we definitely want to be doing a much more MVP kind of style like we have here. Getting our logic away from the activity as much as possible will be a big win for testability.

when(userProfileInteractor.observeProfileImage()).thenReturn(photoObservable);
}
private void createPresenter() {

This comment has been minimized.

@1zaman

1zaman Mar 16, 2016

Contributor

Should this just have a @Before annotation added, as it seems to be invoked from all the test methods?

This comment has been minimized.

@ghost

ghost Mar 17, 2016

Ah, good point. Before I switched to the Observer pattern I had to mock the network calls in each test before initializing the presenter. But yeah, now it's possible to create it upfront always. Even better :)

public void setName_updatesTextView() {
final String text = "hello";
view.setName(text);
assertThat(binding.nameText.getText().toString(), is(text));

This comment has been minimized.

@1zaman

1zaman Mar 16, 2016

Contributor

Note that we're using the AssertJ Android library, which has specific assertion utility methods defined for all common Java and Android framework methods, and can be used instead of the Hamcrest matchers.

This comment has been minimized.

@ghost

ghost Mar 17, 2016

I see. I switched over to AssertJ for this test.

CachingObservable<UserProfileViewModel> accountObservable;
CachingObservable<UserProfileImageViewModel> photoObservable;

This comment has been minimized.

@1zaman

1zaman Mar 16, 2016

Contributor

The local variables should be private or protected, I guess. This applies to the other test classes too.

This comment has been minimized.

@ghost

ghost Mar 17, 2016

made private

// Using AsyncTask.THREAD_POOL_EXECUTOR to automatically support Espresso idling resources
@VisibleForTesting
@NonNull
public static Executor EXECUTOR = AsyncTask.THREAD_POOL_EXECUTOR;

This comment has been minimized.

@1zaman

1zaman Mar 17, 2016

Contributor

The AsyncTask.THREAD_POOL_EXECUTOR has it's maximum pool size defined as CPU_COUNT * 2 + 1 post-KitKat (and as 128 pre-KitKat). It's work queue is also bounded accordingly. I think you should define your own ThreadPoolExecutor with a reasonable and consistent maximum pool and queue size, and rejection handler (or just set the max pool size to unlimited). Note that OkHttp has a Dispatcher class that defines a reasonable maximum for the total number of concurrent requests and also a maximum for concurrent request to one host, and this is also used in the Retrofit 2 enqueueing logic, so we might also want to define a method that uses it.

This comment has been minimized.

@ghost

ghost Mar 17, 2016

Per my comment, I'm using this thread pool because it is automatically compatible with Espresso.

Once we switch to async retrofit / okhttp calls (which I would like to do ASAP), we won't need this thread pool any longer.

This comment has been minimized.

@aleffert

aleffert Mar 17, 2016

Contributor

Is there a ticket for that?

This comment has been minimized.

@ghost

ghost Mar 17, 2016

For updating Retrofit or for replacing this class?

Here, I just filed both: https://openedx.atlassian.net/browse/MA-2185

I also commented in the code that it's a temporary interim solution.

This comment has been minimized.

@1zaman

1zaman Mar 18, 2016

Contributor

How about changing the executor when running tests on Espresso, as you have
done for the Robolectric BaseTest class? Also, doing a quick search on
this, it looks like it may possible to implement a
custom ThreadPoolIdlingResource:
https://stefanodacchille.github.io/blog/2014/04/05/wait-for-it/

On Thu, Mar 17, 2016 at 11:05 PM, Brian Guertin notifications@github.com
wrote:

In
VideoLocker/src/main/java/org/edx/mobile/util/observer/AsyncCallableUtils.java
#613 (comment):

+package org.edx.mobile.util.observer;
+
+import android.os.AsyncTask;
+import android.support.annotation.NonNull;
+import android.support.annotation.VisibleForTesting;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+public enum AsyncCallableUtils {

  • ;
  • // Using AsyncTask.THREAD_POOL_EXECUTOR to automatically support Espresso idling resources
  • @VisibleForTesting
  • @nonnull
  • public static Executor EXECUTOR = AsyncTask.THREAD_POOL_EXECUTOR;

Per my comment, I'm using this thread pool because it is automatically
compatible with Espresso.

Once we switch to async retrofit / okhttp calls (which I would like to do
ASAP), we won't need this thread pool any longer.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-app-android/pull/613/files/1b79f86bec593c27df395eb26a9eb9586f474ec4#r56550782

This comment has been minimized.

@1zaman

1zaman Mar 18, 2016

Contributor

Yeah, it looks like we'll need to wait to upgrade to Retrofit 2 to be able
to use OkHttp's Dispatcher class, as it requires handling it's Call
implementation directly, which is not exposed by Retrofit 1.

On Fri, Mar 18, 2016 at 12:43 AM, Brian Guertin notifications@github.com
wrote:

In
VideoLocker/src/main/java/org/edx/mobile/util/observer/AsyncCallableUtils.java
#613 (comment):

+package org.edx.mobile.util.observer;
+
+import android.os.AsyncTask;
+import android.support.annotation.NonNull;
+import android.support.annotation.VisibleForTesting;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.Executor;
+
+public enum AsyncCallableUtils {

  • ;
  • // Using AsyncTask.THREAD_POOL_EXECUTOR to automatically support Espresso idling resources
  • @VisibleForTesting
  • @nonnull
  • public static Executor EXECUTOR = AsyncTask.THREAD_POOL_EXECUTOR;

For updating Retrofit or for replacing this class?

Here, I just filed both: https://openedx.atlassian.net/browse/MA-2185

I also commented in the code that it's a temporary interim solution.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-app-android/pull/613/files/1b79f86bec593c27df395eb26a9eb9586f474ec4#r56565885

}
@Override
public final P onRetainCustomNonConfigurationInstance() {

This comment has been minimized.

@1zaman

1zaman Mar 17, 2016

Contributor

Note that this method is deprecated, and we're supposed to use a retained Fragment for this kind of stuff instead.

This comment has been minimized.

@ghost

ghost Mar 17, 2016

No, this method is not deprecated.

However I didn't end up using this class, so happy to delete it for now.

This comment has been minimized.

@1zaman

1zaman Mar 18, 2016

Contributor

Ah, looks like FragmentActivity defines non-deprecated versions of these
methods with the "custom" tag.

On Thu, Mar 17, 2016 at 10:38 PM, Brian Guertin notifications@github.com
wrote:

In VideoLocker/src/main/java/org/edx/mobile/view/PresenterActivity.java
#613 (comment):

  • }
  • @override
  • @callsuper
  • protected void onPostCreate(Bundle savedInstanceState) {
  •    super.onPostCreate(savedInstanceState);
    
  •    presenter.attachView(createView());
    
  • }
  • @override
  • public final P getLastCustomNonConfigurationInstance() {
  •    return (P) super.getLastCustomNonConfigurationInstance();
    
  • }
  • @override
  • public final P onRetainCustomNonConfigurationInstance() {

No, this method is not deprecated.

However I didn't end up using this class, so happy to delete it for now.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-app-android/pull/613/files/1b79f86bec593c27df395eb26a9eb9586f474ec4#r56545954

@1zaman

This comment has been minimized.

Contributor

1zaman commented Mar 17, 2016

I took a quick look through the new framework implementation, and added a few comments. Overall, it looks like a robust design. I haven't used an MVP framework before, so I guess I'll have to learn this convoluted thing as well now 😜

RxJava integration might be beneficial in the long term if we're doing it this way, because it seems to provide a lot of flexible patterns. On the other hand, it's another framework to learn (for me at least!), so up to you guys if you think it worth it.

@ghost

This comment has been minimized.

ghost commented Mar 17, 2016

Yeah, I considered using RxJava but that library does a LOT more than the simple callback interface I defined here, so I thought it wasn't worth it. I'm not against it though.

@aleffert

This comment has been minimized.

Contributor

aleffert commented Mar 17, 2016

I was thinking about this some more and I think it's worth pursuing RxJava, even if it's more complicated than we would really like. This is for the usual using a library reasons: one less thing to maintain, other people will be familiar with it, there are plenty of tutorials about it, if we need to pick up some of its more advanced features, they'll already be there.

That said, I'd like to get this landed so we can ship user profiles. It has good test coverage so, in theory at least, it should pretty safe to mess with it, and it wouldn't be any more wasted work since this code is already written.

@ghost

This comment has been minimized.

ghost commented Mar 17, 2016

So, I think this is this good to go?

I'm sure we'll continue to evolve our approach as we update more screens. I'm happy with this as a jumping off point.

I also added a few more tests to get the coverage up a bit more (~84% for the "profiles" package)

ghost pushed a commit that referenced this pull request Mar 22, 2016

Merge pull request #613 from edx/bguertin/profile_mvp
Add UserProfile unit tests / MVP infrastructure

@ghost ghost merged commit 29da73b into master Mar 22, 2016

2 checks passed

codecov/project 16.22% (+1.34%) compared to ed7dda4 at 14.88%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost deleted the bguertin/profile_mvp branch May 26, 2016

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment