Support action button to fetch more content suggestions

chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java

Index: chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
index 399e687206a625fc0de42966cf56dd93851448a1..01a58ef966c3c3ff465ee402f5c4d5ca3206bd14 100644
--- a/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
@@ -19,6 +19,7 @@ import org.chromium.chrome.browser.ntp.snippets.CategoryStatus.CategoryStatusEnu
 import org.chromium.chrome.browser.ntp.snippets.ContentSuggestionsCardLayout;
 import org.chromium.chrome.browser.ntp.snippets.KnownCategories;
 import org.chromium.chrome.browser.ntp.snippets.SnippetArticleListItem;
+import org.chromium.chrome.browser.ntp.snippets.SnippetsBridge;
 import org.chromium.chrome.browser.ntp.snippets.SuggestionsSource;
 import org.chromium.testing.local.LocalRobolectricTestRunner;
 import org.junit.Before;
@@ -40,22 +41,6 @@ import java.util.Set;
 @RunWith(LocalRobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class NewTabPageAdapterTest {
-    /**
-     * Number of elements, not including content suggestions that are loaded
-     * in a populated recycler view.
-     * The 3 elements are: above-the-fold, header, bottom spacer
-     * TODO(dgn): Make this depend on the category info of the loaded sections
-     * instead of being a constant, as it needs to know if the MORE button is
-     * present for example.
-     */
-    private static final int PERMANENT_ELEMENTS_COUNT = 3;
-
-    /**
-     * Number of elements that are loaded in an empty recycler view
-     * The 5 elements are: above-the-fold, header, status card, progress
-     * indicator, bottom spacer.
-     */
-    private static final int EMPTY_STATE_ELEMENTS_COUNT = 5;
 
     private static class FakeSnippetsSource implements SuggestionsSource {
         private SuggestionsSource.Observer mObserver;
@@ -71,7 +56,8 @@ public class NewTabPageAdapterTest {
 
         public void setSuggestionsForCategory(
                 @CategoryInt int category, List<SnippetArticleListItem> suggestions) {
-            mSuggestions.put(category, suggestions);
+            // Copy the suggestions list so that it can't be modified anymore.
+            mSuggestions.put(category, new ArrayList<>(suggestions));
             if (mObserver != null) mObserver.onNewSuggestions(category);
         }
 
@@ -123,6 +109,9 @@ public class NewTabPageAdapterTest {
 
         @Override
         public List<SnippetArticleListItem> getSuggestionsForCategory(int category) {
+            if (!SnippetsBridge.isCategoryStatusAvailable(mCategoryStatus.get(category))) {
+                return Collections.emptyList();
+            }
             List<SnippetArticleListItem> result = mSuggestions.get(category);
             return result == null ? Collections.<SnippetArticleListItem>emptyList() : result;
         }
@@ -131,6 +120,32 @@ public class NewTabPageAdapterTest {
     private FakeSnippetsSource mSnippetsSource = new FakeSnippetsSource();
     private NewTabPageAdapter mNtpAdapter;
 
+    private int expectedElementsCount() {

[dgn, 2016/08/11 16:58:04]

Sorry for checking in so late. This is super convenient, but it doesn't make
obvious from the test code what the expectation is, so fixing or updating tests
might get tricky.

For example depending on SnippetsBridge#isCategoryStatusInitOrAvailable sounds a
bit meh. I suppose depending on the fake snippet source is ok since it's read
only from the point of view of the adapter for now.

Could we do something more explicit? Maybe something like
expectedCountFor(sectionCount, elementCount, buttonsCount)

Or maybe something that works a bit like a builder pattern, and could be used
like

// manipulate the objects to test
// ...
expectation = expectationFor().section(/*hasButton=*/false, /*suggestioncount=*/
42)
AssertEquals(expectation.count(), foo.getItemCount());

// some other stuff
// ...
expectation.section(true, 6); // building on the previous expectations
AssertEquals(expectation.count(), foo.getItemCount());

We would be spelling what the recycler view should look like in a clearer way

[tschumann, 2016/08/12 08:02:39]

Drive-by comment. Such a complicated logic for determining the expected count
(used in an assert statement) is usually a smell. The test setup should be
pretty clear about the number of expected results. When a test fails, you don't
want to have to debug through this function first to figure out how you got to
the expected value.

I'd vote for keeping this explicit in the test, e.g. write expressions like
these in the test: EMPTY_STATE_ELEMENTS_COUNT + 5 /* results in section FOO */ +
1 /* More button */

[Philipp Keck, 2016/08/12 08:12:51]

@Tim, please take a look at the new solution.
First, we wanted something a bit less static because we sometimes change things
like add a more-button, add a loading indicator, etc. and we always have to
update all those numbers, even though the expectation is technically still the
same ("it shows the sections I just added").

The solution implemented now, in my opinion, is even more expressive than
"EMPTY_STATE_ELEMENTS_COUNT + 5" because it also explains how you get the 5
(namely a section with 1 header, 3 elements and 1 more button -- or a section
with 1 header and 4 elements. Also, I think you don't have to debug through the
helper methods anymore because they're all rather small and have a telling name,
plus the failing assertion gives you usable output.

[Philipp Keck, 2016/08/12 08:12:51]

Ok. That's the issue I contacted you in Hangouts about. I like the builder
pattern idea and now implemented something similar (also fluent API):
assertItemsFor(section(numElements), section(...), sectionWithMoreButton(...))
This solution doesn't need the inline comments to explain things and has the
assertion built-in, which allows it to output the full list of actually present
items in case it fails, which greatly helps debugging (so you can immediately
see, for example, that there is one article more than you thought there should
be, etc.).

PTAL

+        int result = 1; // above-the-fold.
+        for (int category : mSnippetsSource.getCategories()) {
+            int numSuggestions;
+            if (SnippetsBridge.isCategoryStatusInitOrAvailable(
+                        mSnippetsSource.getCategoryStatus(category))) {
+                numSuggestions = mSnippetsSource.getSuggestionsForCategory(category).size();
+            } else {
+                // When the status is an unavailable status, the UI removes the existing suggestions
+                // (and displays a status card).
+                numSuggestions = 0;
+            }
+            result += 1; // header.
+            result += numSuggestions;
+            if (numSuggestions == 0) {
+                result += 2; // status card and progress indicator.
+            } else if (mSnippetsSource.getCategoryInfo(category).hasMoreButton()) {
+                result += 1; // more button.
+            }
+        }
+        if (mSnippetsSource.getCategories().length > 0) {
+            result += 1; // bottom spacer.
+        }
+        return result;
+    }
+
     @Before
     public void setUp() {
         RecordHistogram.disableForTests();
@@ -139,7 +154,7 @@ public class NewTabPageAdapterTest {
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.INITIALIZING);
         mSnippetsSource.setInfoForCategory(
                 KnownCategories.ARTICLES, new SuggestionsCategoryInfo("Articles for you",
-                                                  ContentSuggestionsCardLayout.FULL_CARD));
+                                                  ContentSuggestionsCardLayout.FULL_CARD, false));
         mNtpAdapter = new NewTabPageAdapter(null, null, mSnippetsSource, null);
     }
 
@@ -150,7 +165,7 @@ public class NewTabPageAdapterTest {
     @Test
     @Feature({"Ntp"})
     public void testSnippetLoading() {
-        assertEquals(EMPTY_STATE_ELEMENTS_COUNT, mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
         assertEquals(NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD, mNtpAdapter.getItemViewType(0));
         assertEquals(NewTabPageListItem.VIEW_TYPE_HEADER, mNtpAdapter.getItemViewType(1));
         assertEquals(NewTabPageListItem.VIEW_TYPE_STATUS, mNtpAdapter.getItemViewType(2));
@@ -158,11 +173,14 @@ public class NewTabPageAdapterTest {
         assertEquals(NewTabPageListItem.VIEW_TYPE_SPACING, mNtpAdapter.getItemViewType(4));
 
         List<SnippetArticleListItem> snippets = createDummySnippets();
+        mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
 
         List<NewTabPageListItem> loadedItems = new ArrayList<>(mNtpAdapter.getItems());
         assertEquals(NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD, mNtpAdapter.getItemViewType(0));
         assertEquals(NewTabPageListItem.VIEW_TYPE_HEADER, mNtpAdapter.getItemViewType(1));
+        // From the loadedItems, cut out aboveTheFold and header from the front,
+        // and bottom spacer from the back.
         assertEquals(snippets, loadedItems.subList(2, loadedItems.size() - 1));
         assertEquals(NewTabPageListItem.VIEW_TYPE_SPACING,
                 mNtpAdapter.getItemViewType(loadedItems.size() - 1));
@@ -184,7 +202,7 @@ public class NewTabPageAdapterTest {
         // If we don't get anything, we should be in the same situation as the initial one.
         mSnippetsSource.setSuggestionsForCategory(
                 KnownCategories.ARTICLES, new ArrayList<SnippetArticleListItem>());
-        assertEquals(EMPTY_STATE_ELEMENTS_COUNT, mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
         assertEquals(NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD, mNtpAdapter.getItemViewType(0));
         assertEquals(NewTabPageListItem.VIEW_TYPE_HEADER, mNtpAdapter.getItemViewType(1));
         assertEquals(NewTabPageListItem.VIEW_TYPE_STATUS, mNtpAdapter.getItemViewType(2));
@@ -193,10 +211,13 @@ public class NewTabPageAdapterTest {
 
         // We should load new snippets when we get notified about them.
         List<SnippetArticleListItem> snippets = createDummySnippets();
+        mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
         List<NewTabPageListItem> loadedItems = new ArrayList<>(mNtpAdapter.getItems());
         assertEquals(NewTabPageListItem.VIEW_TYPE_ABOVE_THE_FOLD, mNtpAdapter.getItemViewType(0));
         assertEquals(NewTabPageListItem.VIEW_TYPE_HEADER, mNtpAdapter.getItemViewType(1));
+        // From the loadedItems, cut out aboveTheFold and header from the front,
+        // and bottom spacer from the back.
         assertEquals(snippets, loadedItems.subList(2, loadedItems.size() - 1));
         assertEquals(NewTabPageListItem.VIEW_TYPE_SPACING,
                 mNtpAdapter.getItemViewType(loadedItems.size() - 1));
@@ -215,26 +236,27 @@ public class NewTabPageAdapterTest {
     @Feature({"Ntp"})
     public void testSnippetClearing() {
         List<SnippetArticleListItem> snippets = createDummySnippets();
+        mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
         // If we get told that snippets are enabled, we just leave the current
         // ones there and not clear.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.AVAILABLE);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
         // When snippets are disabled, we clear them and we should go back to
         // the situation with the status card.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.SIGNED_OUT);
-        assertEquals(EMPTY_STATE_ELEMENTS_COUNT, mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
         // The adapter should now be waiting for new snippets.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
     }
 
     /**
@@ -246,32 +268,34 @@ public class NewTabPageAdapterTest {
         List<SnippetArticleListItem> snippets = createDummySnippets();
 
         // By default, status is INITIALIZING, so we can load snippets
+        mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES, CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
-        // If we have snippets, we should not load the new list.
+        // If we have snippets, we should not load the new list (i.e. the extra item does *not*
+        // appear).
         snippets.add(new SnippetArticleListItem("https://site.com/url1", "title1", "pub1", "txt1",
                 "https://site.com/url1", "https://amp.site.com/url1", 0, 0, 0));
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size() - 1, mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount() - 1, mNtpAdapter.getItemCount());
 
-        // When snippets are disabled, we should not be able to load them
+        // When snippets are disabled, we should not be able to load them.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.SIGNED_OUT);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(EMPTY_STATE_ELEMENTS_COUNT, mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
         // INITIALIZING lets us load snippets still.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.INITIALIZING);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
 
         // The adapter should now be waiting for new snippets.
         mSnippetsSource.setStatusForCategory(KnownCategories.ARTICLES,
                 CategoryStatus.AVAILABLE);
         mSnippetsSource.setSuggestionsForCategory(KnownCategories.ARTICLES, snippets);
-        assertEquals(PERMANENT_ELEMENTS_COUNT + snippets.size(), mNtpAdapter.getItemCount());
+        assertEquals(expectedElementsCount(), mNtpAdapter.getItemCount());
     }
 
     /**