[Offline pages] Refactoring: Merge Connectivity Listener into TabObserver

chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java

Index: chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
diff --git a/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java b/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
new file mode 100644
index 0000000000000000000000000000000000000000..8f360132d7e109a824fb8c6f80bbe2a08e186588
--- /dev/null
+++ b/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserverTest.java
@@ -0,0 +1,308 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.offlinepages;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.any;

[Pete Williamson, 2016/03/28 19:52:39]

Approach question:  Why use Mockito instead of creating custom test stubs?  I
usually find the code with custom stubs easier to write, maintain, and reason
about.

[fgorski, 2016/03/28 22:52:33]

Reasons to use Mockito:
Amongst it's weponry are such diverse elements as:
* Boilerplate is gone with this approach
* Mocks are easily (re-)configurable
* You can verify what calls where made and which not much easier.
* (My favorite) You can wrap a spy over the mocked class to actually invoke the
actual implementation
* The fluent interface applied in mockito makes things very readable
* blah blah blah

+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import android.content.Context;
+
+import org.chromium.base.BaseChromiumApplication;
+import org.chromium.base.test.util.Feature;
+import org.chromium.chrome.browser.snackbar.SnackbarManager;
+import org.chromium.chrome.browser.snackbar.SnackbarManager.SnackbarController;
+import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.testing.local.LocalRobolectricTestRunner;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.annotation.Config;
+
+/**
+ * Unit tests for OfflinePageUtils.
+ */
+@RunWith(LocalRobolectricTestRunner.class)
+@Config(manifest = Config.NONE, application = BaseChromiumApplication.class)
+public class OfflinePageTabObserverTest {
+    // Using a null tab, as it cannot be mocked. TabHelper will help return proper mocked responses.
+    private static final Tab TAB = null;
+    private static final int TAB_ID = 77;
+
+    @Mock private Context mContext;
+    @Mock private SnackbarManager mSnackbarManager;
+    @Mock private SnackbarController mSnackbarController;
+    @Mock private OfflinePageTabObserver.TabHelper mTabHelper;
+
+    private OfflinePageTabObserver createObserver() {
+        OfflinePageTabObserver observer =
+                spy(new OfflinePageTabObserver(mContext, mSnackbarManager, mSnackbarController));
+        // Mocking out all of the calls that touch on NetworkChangeNotifier, which we cannot
+        // directly mock out.
+        doNothing().when(observer).startObservingNetworkChanges();
+        doNothing().when(observer).stopObservingNetworkChanges();
+        // This call has to be mocked out until we update OfflinePageUtils.
+        // It also goes to NetworkChangeNotifier from there.
+        doReturn(false).when(observer).isConnected();
+        // This call has to be mocked out until we update OfflinePageUtils.

[Pete Williamson, 2016/03/28 19:52:39]

Perhaps we should have a TODO here to come back and update these when we do
update OfflinePageUtils.

[fgorski, 2016/03/28 22:52:33]

Done.

+        doNothing().when(observer).showReloadSnackbar();
+        return observer;
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        MockitoAnnotations.initMocks(this);
+
+        // Setting up a mock tab helper.
+        doReturn(TAB_ID).when(mTabHelper).getTabId(any(Tab.class));
+        doReturn(true).when(mTabHelper).isTabShowing(any(Tab.class));

[Pete Williamson, 2016/03/28 19:52:39]

Does it make sense to mock these in the individual tests instead of here?  We
might want to have tests where the tab is not showing at some point, or we start
with an online page instead of an offline page.  It's OK if your answer is
"let's keep what we have and refactor later".  It's also OK (but a bit
confusing) if other mocks in the test itself override this as a default.

[fgorski, 2016/03/28 22:52:32]

Done. I added a comment in code.
There are the common values used in most tests. Individual tests apply different
values where it is required.

+        doReturn(true).when(mTabHelper).isOfflinePage(any(Tab.class));
+        OfflinePageTabObserver.setTabHelperForTesting(mTabHelper);
+
+        // Setting up mock snackbar manager.
+        doNothing().when(mSnackbarManager).dismissSnackbars(eq(mSnackbarController));
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testBasicState() {
+        OfflinePageTabObserver observer = createObserver();
+        assertFalse(observer.isObservingNetworkChanges());
+        assertFalse(observer.wasSnackbarShown());

[dewittj, 2016/03/28 20:17:59]

can you use SnackbarManager#isShowing() or
SnackbarManager#getCurrentSnackbarForTesting() instead of a private method?

[fgorski, 2016/03/28 22:52:32]

I don't want to. This is part of the logic of this class. Snackbar might have
been shown, but is no longer visible at the time wasSnackbarShown is invoked.
This test makes sure that it is set to a reasonable value after creation.

[dewittj, 2016/03/28 23:40:02]

I'll leave it up to you then.

*aside*

I'm asking these types of questions simply because this type of test is brittle
to changes.  If you assert that a certain bit is set or not, then bugs in
changing the bit could end up passing a test.  If the test whether a snackbar
was shown or not, then the test reflects the desired outcome, not internal
state.

Most of your tests verify both that showReloadSnackbar was or wasn't called, and
also check Observer#wasSnackbarShown.  I'd argue that asserting the value of
Observer#wasSnackbarShown() is harmful to the resiliency of this test.

[fgorski, 2016/03/29 16:00:43]

Once I get OfflinePageUtils more testable I'll consider if exposing this still
makes sense.

+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testStartObservingTab() {
+        OfflinePageTabObserver observer = createObserver();
+
+        doReturn(false).when(mTabHelper).isOfflinePage(any(Tab.class));
+        observer.startObservingTab(TAB);
+
+        assertFalse(observer.isObservingNetworkChanges());
+        assertFalse(observer.isObservingTab(TAB));
+        assertFalse(observer.wasSnackbarShown());
+
+        doReturn(true).when(mTabHelper).isOfflinePage(any(Tab.class));
+        observer.startObservingTab(TAB);
+
+        assertTrue(observer.isObservingNetworkChanges());
+        assertTrue(observer.isObservingTab(TAB));
+        assertFalse(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testStopObservingTab() {
+        OfflinePageTabObserver observer = createObserver();
+
+        doReturn(true).when(mTabHelper).isOfflinePage(any(Tab.class));
+        observer.startObservingTab(TAB);
+
+        assertTrue(observer.isObservingNetworkChanges());
+        assertTrue(observer.isObservingTab(TAB));
+        assertFalse(observer.wasSnackbarShown());
+
+        // Try to stop observing a tab that is not observed.
+        doReturn(42).when(mTabHelper).getTabId(any(Tab.class));
+        observer.stopObservingTab(TAB);
+
+        assertFalse(observer.isObservingTab(TAB));
+        doReturn(TAB_ID).when(mTabHelper).getTabId(any(Tab.class));
+        assertTrue(observer.isObservingTab(TAB));
+        assertTrue(observer.isObservingNetworkChanges());
+        assertFalse(observer.wasSnackbarShown());
+
+        observer.stopObservingTab(TAB);
+        assertFalse(observer.isObservingNetworkChanges());
+        assertFalse(observer.isObservingTab(TAB));
+        assertFalse(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnShown() {
+        OfflinePageTabObserver observer = createObserver();
+
+        observer.startObservingTab(TAB);
+        observer.onShown(TAB);
+
+        verify(observer, times(0)).showReloadSnackbar();
+        assertFalse(observer.wasSnackbarShown());
+

[Pete Williamson, 2016/03/28 19:52:38]

// Hide the tab.

[fgorski, 2016/03/28 22:52:33]

Done.

+        doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class));
+        observer.onHidden(TAB);
+
+        // Testing on shown when connected.

[Pete Williamson, 2016/03/28 19:52:38]

-> // If we are connected and we show the tab, we should see the reload snackbar
show once.

[fgorski, 2016/03/28 22:52:32]

Done.

+        doReturn(true).when(observer).isConnected();
+        observer.onShown(TAB);
+
+        verify(observer, times(1)).showReloadSnackbar();
+        assertTrue(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnHidden() {
+        OfflinePageTabObserver observer = createObserver();
+
+        observer.startObservingTab(TAB);
+        doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class));

[Pete Williamson, 2016/03/28 19:52:39]

// Hide the tab.

[fgorski, 2016/03/28 22:52:32]

Done.

+        observer.onHidden(TAB);
+
+        assertFalse(observer.wasSnackbarShown());
+        assertTrue(observer.isObservingTab(TAB));
+        verify(mSnackbarManager, times(1)).dismissSnackbars(eq(mSnackbarController));
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnHidden_whenSnackbarShown() {

[Pete Williamson, 2016/03/28 19:52:39]

maybe rename: testOnHidden_snackbarNotShownForHiddenPage ?

[fgorski, 2016/03/28 22:52:32]

Done.

[Pete Williamson, 2016/03/29 08:09:36]

Done.

+        OfflinePageTabObserver observer = createObserver();
+
+        observer.startObservingTab(TAB);
+        doReturn(true).when(observer).isConnected();
+        observer.onShown(TAB);
+
+        doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class));
+        observer.onHidden(TAB);
+
+        verify(mSnackbarManager, times(1)).dismissSnackbars(eq(mSnackbarController));
+        assertFalse(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnDestroyed() {
+        OfflinePageTabObserver observer = createObserver();
+
+        observer.startObservingTab(TAB);
+        doReturn(true).when(observer).isConnected();
+        observer.onShown(TAB);
+
+        observer.onDestroyed(TAB);
+
+        verify(observer, times(1)).stopObservingTab(any(Tab.class));
+        verify(mTabHelper, times(1)).removeObserver(any(Tab.class), eq(observer));
+        assertFalse(observer.wasSnackbarShown());
+        assertFalse(observer.isObservingTab(TAB));
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnUrlUpdated() {
+        OfflinePageTabObserver observer = createObserver();
+
+        observer.startObservingTab(TAB);
+        // URL updated, but tab still shows offline page.
+        observer.onUrlUpdated(TAB);
+
+        assertTrue(observer.isObservingTab(TAB));
+        verify(observer, times(0)).stopObservingTab(any(Tab.class));
+        verify(mSnackbarManager, times(1)).dismissSnackbars(eq(mSnackbarController));
+
+        // URL updated and tab no longer shows offline page.
+        doReturn(false).when(mTabHelper).isOfflinePage(any(Tab.class));
+        observer.onUrlUpdated(TAB);
+
+        assertFalse(observer.isObservingTab(TAB));
+        verify(observer, times(1)).stopObservingTab(any(Tab.class));
+        verify(mSnackbarManager, times(2)).dismissSnackbars(eq(mSnackbarController));
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testAddObserverForTab() {
+        OfflinePageTabObserver observer = createObserver();
+        OfflinePageTabObserver.setInstanceForTesting(observer);
+
+        // Method under test.
+        OfflinePageTabObserver.addObserverForTab(TAB);
+
+        verify(mTabHelper, times(1)).addObserver(any(Tab.class), eq(observer));
+        verify(observer, times(1)).startObservingTab(any(Tab.class));
+        assertTrue(observer.isObservingTab(TAB));
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnConnectionTypeChanged_notConnected() {
+        OfflinePageTabObserver observer = createObserver();
+
+        // Ensures that observer actually listens for network notifications and tab is shown.
+        observer.startObservingTab(TAB);
+        observer.onShown(TAB);
+        assertTrue(observer.isObservingNetworkChanges());
+
+        // Notification comes, but we are still disconnected.
+        observer.onConnectionTypeChanged(0);
+
+        verify(observer, times(0)).showReloadSnackbar();
+        assertFalse(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnConnectionTypeChanged_tabNotShowing() {
+        OfflinePageTabObserver observer = createObserver();
+
+        // Ensures that observer actually listens for network notifications and tab is hidden.
+        observer.startObservingTab(TAB);
+        doReturn(false).when(mTabHelper).isTabShowing(any(Tab.class));
+        observer.onHidden(TAB);
+
+        doReturn(true).when(observer).isConnected();
+        assertTrue(observer.isObservingNetworkChanges());
+        observer.onConnectionTypeChanged(0);
+
+        verify(observer, times(0)).showReloadSnackbar();
+        assertFalse(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnConnectionTypeChanged_wasSnackbarShown() {
+        OfflinePageTabObserver observer = createObserver();
+
+        doReturn(true).when(observer).isConnected();
+
+        // Ensures that observer actually listens for network notifications and tab is shown when
+        // connected, which means snackbar is shown as well.
+        observer.startObservingTab(TAB);
+        observer.onShown(TAB);
+        verify(observer, times(1)).showReloadSnackbar();
+        assertTrue(observer.wasSnackbarShown());
+
+        assertTrue(observer.isObservingNetworkChanges());
+        observer.onConnectionTypeChanged(0);
+        verify(observer, times(1)).showReloadSnackbar();
+        assertTrue(observer.wasSnackbarShown());
+    }
+
+    @Test
+    @Feature({"OfflinePages"})
+    public void testOnConnectionTypeChanged() {
+        OfflinePageTabObserver observer = createObserver();
+
+        // Ensures that observer actually listens for network notifications and tab is shown.
+        observer.startObservingTab(TAB);
+        observer.onShown(TAB);
+
+        doReturn(true).when(observer).isConnected();
+        observer.onConnectionTypeChanged(0);
+
+        verify(observer, times(1)).showReloadSnackbar();
+        assertTrue(observer.wasSnackbarShown());
+        assertTrue(observer.isObservingNetworkChanges());
+    }
+}