Push API: test that default notification is shown on Android when needed.

chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java

Index: chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
index e7270ff76b6628b8e8e0bb1c804e5c23f12ee997..f9e28adb2b19a95a99903132f166dfa9204123e9 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/push_messaging/PushMessagingTest.java
@@ -4,21 +4,29 @@
 
 package org.chromium.chrome.browser.push_messaging;
 
+import static org.chromium.base.test.util.ScalableTimeout.scaleTimeout;
+
 import android.annotation.SuppressLint;
 import android.app.Notification;
 import android.content.Context;
 import android.os.Bundle;
+import android.test.MoreAsserts;
+import android.test.suitebuilder.annotation.LargeTest;
 import android.test.suitebuilder.annotation.MediumTest;
 
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.browser.notifications.NotificationTestBase;
 import org.chromium.chrome.browser.preferences.website.ContentSetting;
+import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.test.util.TestHttpServerClient;
 import org.chromium.chrome.test.util.browser.TabTitleObserver;
 import org.chromium.chrome.test.util.browser.notifications.MockNotificationManagerProxy.NotificationEntry;
 import org.chromium.components.gcm_driver.FakeGoogleCloudMessagingSubscriber;
 import org.chromium.components.gcm_driver.GCMDriver;
+import org.chromium.content.browser.test.util.JavaScriptUtils;
+
+import java.util.concurrent.TimeoutException;
 
 /**
  * Instrumentation tests for the Push API and the integration with the Notifications API on Android.
@@ -28,8 +36,9 @@
 public class PushMessagingTest extends NotificationTestBase {
     private static final String PUSH_TEST_PAGE = TestHttpServerClient.getUrl(
             "chrome/test/data/push_messaging/push_messaging_test_android.html");
+    private static final String ABOUT_BLANK = "about:blank";
     private static final String SENDER_ID_BUNDLE_KEY = "from";
-    private static final int TITLE_UPDATE_TIMEOUT_SECONDS = 5;
+    private static final int TITLE_UPDATE_TIMEOUT_SECONDS = (int) scaleTimeout(2);

[johnme, 2015/12/08 17:12:31]

You might want this in milliseconds? Otherwise if
/data/local/tmp/chrome_timeout_scale is 1.49, scaleTimeout will still return 2
(since it returns the floor). Or change the scaleTimeout implementation to ceil
instead of floor...

Also, 2 seconds is low enough that flakiness seems more likely; perhaps leave it
as 5?

[Michael van Ouwerkerk, 2015/12/09 15:02:51]

Sure I can keep it at 5. TabTitleObserver#waitForTitle requires an int to
specify the number of seconds though. I think that the (unlikely?) rounding
problem is mitigated by the larger timeout.

 
     /**
      * Verifies that a notification can be shown from a push event handler in the service worker.
@@ -44,24 +53,89 @@ public void testPushAndShowNotification() throws Exception {
         setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW);
         runScriptAndWaitForTitle("subscribePush()", "subscribe ok");
 
-        sendPushMessage(subscriber.getLastSubscribeSubtype(), subscriber.getLastSubscribeSource());
+        sendPushAndWaitForTitle(subscriber.getLastSubscribeSubtype(),
+                subscriber.getLastSubscribeSource(), "push 1 ok");
         NotificationEntry notificationEntry = waitForNotification();
-        assertEquals("push notification",
+        assertEquals("push notification 1",
+                notificationEntry.notification.extras.getString(Notification.EXTRA_TITLE));
+    }
+
+    /**
+     * Verifies that the default notification is shown when no notification is shown from the push
+     * event handler while no tab is visible for the origin, and grace has been exceeded.
+     */
+    @LargeTest
+    @Feature({"Browser", "PushMessaging"})
+    public void testDefaultNotification() throws Exception {
+        FakeGoogleCloudMessagingSubscriber subscriber = new FakeGoogleCloudMessagingSubscriber();
+        GCMDriver.overrideSubscriberForTesting(subscriber);
+
+        // Load the push test page into the first tab.
+        loadUrl(PUSH_TEST_PAGE);
+        assertEquals(1, getActivity().getCurrentTabModel().getCount());
+        Tab tab = getActivity().getActivityTab();
+        assertEquals(PUSH_TEST_PAGE, tab.getUrl());
+        assertFalse(tab.isHidden());
+
+        // Set up the push subscription and capture its details.
+        setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW);

[johnme, 2015/12/08 17:12:31]

It's tempting to factor out a helper method:

FakeGoogleCloudMessagingSubscriber trySubscribe() {
    FakeGoogleCloudMessagingSubscriber subscriber = new
FakeGoogleCloudMessagingSubscriber();
    GCMDriver.overrideSubscriberForTesting(subscriber);
    setNotificationContentSettingForCurrentOrigin(ContentSetting.ALLOW);
    runScriptAndWaitForTitle("subscribePush()", "subscribe ok");
    assertEquals("1234567890", subscriber.getLastSubscribeSource());
    return subscriber;
}

(maybe even assert that subtype starts with "wp:" + origin + "#", but ignore the
GUID at the end if so)

[Michael van Ouwerkerk, 2015/12/09 15:02:51]

Note that we would also have to load the test page before subscribing. If we add
more tests here, this might be a nice bit of deduplication. As it is, I think we
should try to avoid having quite so many browser and instrumentation tests, and
focus on unit tests where feasible.

+        runScriptAndWaitForTitle("subscribePush()", tab, "subscribe ok");
+        String appId = subscriber.getLastSubscribeSubtype();
+        String senderId = subscriber.getLastSubscribeSource();
+
+        // Make the tab invisible by opening another one with a different origin.
+        loadUrlInNewTab(ABOUT_BLANK);
+        assertEquals(2, getActivity().getCurrentTabModel().getCount());
+        assertEquals(ABOUT_BLANK, getActivity().getActivityTab().getUrl());
+        assertTrue(tab.isHidden());
+
+        // The first time a push event is fired and no notification is shown from the service
+        // worker, grace permits it so no default notification is shown.
+        runScriptAndWaitForTitle("setNotifyOnPush(false)", tab, "setNotifyOnPush ok");
+        sendPushAndWaitForTitle(appId, senderId, tab, "push 1 ok");

[johnme, 2015/12/08 17:12:30]

It's sad that we can't check that the default notification is not yet shown :|
That would also avoid the race condition where the second push gets sent whilst
still processing the first push. Please add a comment noting these issues.

It would also be nice to reduce the chance of race conditions. Even something
hacky might be an improvement, e.g.:
for (int i = 0; i < 10; i++)
    runScriptAndWaitForTitle("document.title = " + i, tab, "" + i);

[Michael van Ouwerkerk, 2015/12/09 15:02:51]

I've added a callback mechanism that waits until the push was fully handled.
That should make things quite complete now.

+
+        // After grace runs out a default notification will be shown.
+        sendPushAndWaitForTitle(appId, senderId, tab, "push 2 ok");
+        NotificationEntry notificationEntry = waitForNotification();
+        assertEquals("This site has been updated in the background.",
+                notificationEntry.notification.extras.getString(Notification.EXTRA_TEXT));
+        MoreAsserts.assertContainsRegex("user_visible_auto_notification", notificationEntry.tag);

[johnme, 2015/12/08 17:12:31]

assertEquals should be fine here

[Michael van Ouwerkerk, 2015/12/09 15:02:51]

Oh... no there is a lot of additional stuff in that tag on the Java level :-)
See NotificationUIManager#makePlatformTag

+
+        // When another push does show a notification, the default notification is automatically
+        // dismissed (an additional mutation) so there is only one left in the end.
+        runScriptAndWaitForTitle("setNotifyOnPush(true)", tab, "setNotifyOnPush ok");
+        sendPushAndWaitForTitle(appId, senderId, tab, "push 3 ok");
+        waitForNotificationManagerMutation();
+        notificationEntry = waitForNotification();
+        assertEquals("push notification 1",
                 notificationEntry.notification.extras.getString(Notification.EXTRA_TITLE));
     }
 
     /**
      * Runs {@code script} in the current tab and waits for the tab title to change to
-     * {@code expectedResult}.
+     * {@code expectedTitle}.
      */
-    private void runScriptAndWaitForTitle(String script, String expectedResult) throws Exception {
-        runJavaScriptCodeInCurrentTab(script);
-        TabTitleObserver titleObserver =
-                new TabTitleObserver(getActivity().getActivityTab(), expectedResult);
-        titleObserver.waitForTitleUpdate(TITLE_UPDATE_TIMEOUT_SECONDS);
+    private void runScriptAndWaitForTitle(String script, String expectedTitle) throws Exception {
+        runScriptAndWaitForTitle(script, getActivity().getActivityTab(), expectedTitle);
     }
 
-    private void sendPushMessage(final String appId, final String senderId) {
+    /**
+     * Runs {@code script} in {@code tab} and waits for the tab title to change to
+     * {@code expectedTitle}.
+     */
+    private void runScriptAndWaitForTitle(String script, Tab tab, String expectedTitle)

[johnme, 2015/12/08 17:12:31]

Probably clearer to move the optional argument to the end. Ditto for
sendPushAndWaitForTitle

[Michael van Ouwerkerk, 2015/12/09 15:02:51]

Done.

+            throws Exception {
+        JavaScriptUtils.executeJavaScript(tab.getWebContents(), script);
+        waitForTitle(tab, expectedTitle);
+    }
+
+    private void sendPushAndWaitForTitle(String appId, String senderId, String expectedTitle)
+            throws Exception {
+        sendPushAndWaitForTitle(appId, senderId, getActivity().getActivityTab(), expectedTitle);
+    }
+
+    private void sendPushAndWaitForTitle(final String appId, final String senderId, Tab tab,
+            String expectedTitle) throws Exception {
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
             public void run() {
@@ -71,5 +145,16 @@ public void run() {
                 GCMDriver.onMessageReceived(context, appId, extras);
             }
         });
+        waitForTitle(tab, expectedTitle);
+    }
+
+    private void waitForTitle(Tab tab, String expectedTitle) throws Exception {
+        TabTitleObserver titleObserver = new TabTitleObserver(tab, expectedTitle);
+        try {
+            titleObserver.waitForTitleUpdate(TITLE_UPDATE_TIMEOUT_SECONDS);
+        } catch (TimeoutException e) {
+            // The title is not as expected, this assertion neatly logs what the difference is.
+            assertEquals(expectedTitle, tab.getTitle());
+        }
     }
 }