Restoring recently deleted unit tests

chrome/browser/resources/google_now/background_unittest.gtestjs

Index: chrome/browser/resources/google_now/background_unittest.gtestjs
diff --git a/chrome/browser/resources/google_now/background_unittest.gtestjs b/chrome/browser/resources/google_now/background_unittest.gtestjs
index 0099576f4a1e4c67b983095e34186f27dff82e50..25556880da56d87118a56230a65daf07b56c3b64 100644
--- a/chrome/browser/resources/google_now/background_unittest.gtestjs
+++ b/chrome/browser/resources/google_now/background_unittest.gtestjs
@@ -477,3 +477,418 @@ TEST_F(
       onNotificationClicked(
           testNotificationId, this.mockLocalFunctions.functions().selector);

[robliao, 2013/12/30 01:59:49]

To reduce test churn do this refactor first:

https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources...

[vadimt, 2014/01/02 21:46:04]

Yeah, I originally agreed that this would make testing easier. Then I looked at
this, and changed my opinion. The proposal was:

Suppose we have function A calling B:
A () {
  Calculate X;
  B(X);
}

So let's replace it with:
AB_Caller () {
  A(function(X) {
    B(X);
  });
}

Now presumably testing of A will become easier. However, I have a some concerns:
1. We add additional method AB_Caller
2. Testing doesn't become easier since we can happily test the original A() byt
mocking B() (note that we still have to use mocking for API calls).

So, I decided that it's better to live everything as is.

[robliao, 2014/01/03 09:09:34]

The callback approach in the CR comment decouples the logic between the separate
steps and creates a single function that shows the stages of processing the
data, request, parse, combine, and display. Since the callback calls the next
step, it's clearer and there's no mocking required. All the test has to ensure
is the callback is called.

In the approach where there is no callback, if we insert a step (like combine)
into the chain, then the test for the previous step needs to mock this new step.

By keeping everything separate, adding new functionality becomes a snap and
tests of the individual steps are independent of the chain.

On 2014/01/02 21:46:04, vadimt wrote:

[vadimt, 2014/01/03 18:50:34]

I wonder what other reviewers think. The proposal is essentially about the
style; you can do this either way. Thoughts?

[rgustafson, 2014/01/07 20:36:44]

Let me look at this more in depth after lunch. 

[robliao, 2014/01/10 21:50:42]

This is a structure request. It should be done to make the code easier to test
and maintain.
On 2014/01/07 20:36:44, rgustafson wrote:

     });
+

[robliao, 2013/12/30 01:59:49]

Overall: Could use some more test cases.
* Fresh run. What happens where there is nothing stored.
* Would these tests have caught http://crbug.com/327079 ?

[vadimt, 2014/01/02 21:46:04]

We should be pragmatic about the testing, and I agree with rgustafson's note:
"I'd like to have some unit tests on the new functionality. ... You don't need
to have quite so many tests as previously, but more than 0 would be nice."

We should not overdo with the tests:
1. The change has been already tested manually;
2. Our code either stays unchanged, or changes dramatically, where you simply
have to rewrite all units. Unit tests are useful when you expect incremental
changes.

The bug you've mentioned was found by manual testing; to find it using unit
tests would require inadequately large set of units.

I believe that current coverage is adequate, not an overkill and works for where
we are now.

[robliao, 2014/01/03 09:09:34]

The bug was found by manual testing, but broke the build for a day while we put
a workaround in. It would be better to catch this kind of breaking issue at
commit queue time.

The responses should be varied to make sure the code continues to handle the
optional cases correctly.
On 2014/01/02 21:46:04, vadimt wrote:

[vadimt, 2014/01/03 18:50:34]

This is a legitimate way of finding bugs :) You are not expected to check-in
code with 0 bugs. I wonder what other reviewers think...

[rgustafson, 2014/01/07 20:36:44]

While I don't agree with the general sentiment of not testing things and waiting
for them to be caught by manual testing, the purpose of this CL was just to
restore _minimal_ testing rather than having 0 tests. There can be a followup
with additional tests where you see places for more, rob.

+TEST_F(

[robliao, 2013/12/30 01:59:49]

Comment targeted test behavior and expectations.

[vadimt, 2014/01/02 21:46:04]

Done.

+    'GoogleNowBackgroundUnitTest',
+    'ShowNotificationCards',
+    function() {
+      // Tests showNotificationCards function.
+
+      // Setup and expectations.
+      var existingNotifications = {
+        'SHOULD BE DELETED': 'SOMETHING',
+        'SHOULD BE KEPT': 'SOMETHING'
+      };
+
+      var notificationGroups = {
+        TEST_FIELD: 'TEST VALUE'
+      };
+
+      var cards = {
+        'SHOULD BE KEPT': [1],
+        'NEW CARD': [2]
+      };
+
+      var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION';

[robliao, 2013/12/30 01:59:49]

Is this necessary? This parameter used for the functions used below is optional.
The expectation is that this is either a function or undefined.

[vadimt, 2014/01/02 21:46:04]

I'm checking that the parameter (if passed) is not ignored, i.e. is passed to
cardSet.update. If I don't pass it, this won't be tested.

+
+      var expectedUpdatedNotifications = {
+        'SHOULD BE KEPT': 'KEPT CARD NOTIFICATION DATA',
+        'NEW CARD': 'NEW CARD NOTIFICATION DATA'
+      };
+
+      this.makeAndRegisterMockApis([
+        'cardSet.update',
+        'chrome.storage.local.set',
+        'instrumented.notifications.getAll'
+      ]);
+      this.makeMockLocalFunctions([
+        'onSuccess'
+      ]);
+
+      var notificationsGetAllSavedArgs = new SaveMockArguments();
+      this.mockApis.expects(once()).
+          instrumented_notifications_getAll(
+              notificationsGetAllSavedArgs.match(ANYTHING)).
+          will(invokeCallback(
+              notificationsGetAllSavedArgs, 0, existingNotifications));
+
+      this.mockApis.expects(once()).
+          cardSet_update(
+              'SHOULD BE KEPT',
+              [1],
+              eqJSON(notificationGroups),
+              fakeOnCardShownFunction).
+          will(returnValue('KEPT CARD NOTIFICATION DATA'));
+      this.mockApis.expects(once()).
+          cardSet_update(
+              'NEW CARD',
+              [2],
+              eqJSON(notificationGroups),
+              fakeOnCardShownFunction).
+          will(returnValue('NEW CARD NOTIFICATION DATA'));
+      this.mockApis.expects(once()).
+          cardSet_update(
+              'SHOULD BE DELETED',
+              [],
+              eqJSON(notificationGroups),
+              fakeOnCardShownFunction).
+          will(returnValue(undefined));
+
+      this.mockApis.expects(once()).
+          chrome_storage_local_set(
+              eqJSON({notificationsData: expectedUpdatedNotifications}));
+
+      this.mockLocalFunctions.expects(once()).
+          onSuccess();
+
+      // Invoking the tested function.
+      showNotificationCards(
+          notificationGroups,
+          cards,
+          this.mockLocalFunctions.functions().onSuccess,
+          fakeOnCardShownFunction);
+    });
+
+TEST_F(
+    'GoogleNowBackgroundUnitTest',
+    'CombineGroup',
+    function() {
+      // Tests combineGroup function.

[robliao, 2013/12/30 01:59:49]

Comment targeted test behavior and expectations.

[vadimt, 2014/01/02 21:46:04]

Done.

+
+      // Setup and expectations.
+      var combinedCards = {
+        'EXISTING CARD': [1]
+      };
+
+      var receivedNotificationNoShowTime = {
+        chromeNotificationId: 'EXISTING CARD',
+        trigger: {hideTimeSec: 1}
+      };
+      var receivedNotificationWithShowTime = {
+        chromeNotificationId: 'NEW CARD',
+        trigger: {showTimeSec: 2, hideTimeSec: 3}
+      }
+
+      var storedGroup = {
+        cardsTimestamp: 10000,
+        cards: [
+          receivedNotificationNoShowTime,
+          receivedNotificationWithShowTime
+        ]
+      };
+
+      // Invoking the tested function.
+      combineGroup(combinedCards, storedGroup);
+
+      // Check the output value.
+      var expectedCombinedCards = {
+        'EXISTING CARD': [
+          1, 
+          {
+            receivedNotification: receivedNotificationNoShowTime,
+            hideTime: 11000
+          }
+        ],
+        'NEW CARD': [
+          {
+            receivedNotification: receivedNotificationWithShowTime,
+            showTime: 12000,
+            hideTime: 13000
+          }
+        ]
+      };
+
+      assertEquals(
+          JSON.stringify(expectedCombinedCards),
+          JSON.stringify(combinedCards));
+    });
+
+TEST_F(
+    'GoogleNowBackgroundUnitTest',
+    'CombineAndShowNotificationCards',
+    function() {
+      // Tests combineAndShowNotificationCards function.
+      // The test passes 2 groups to combineAndShowNotificationCards, checks
+      // that it calls combineGroup() for each of these groups and calls
+      // showNotificationCards() with the results of these combineGroup() calls.
+
+      // Setup and expectations.
+      var testGroups = {
+        'TEST GROUP 1': {testField: 'TEST VALUE 1'},
+        'TEST GROUP 2': {testField: 'TEST VALUE 2'}
+      };
+
+      var fakeOnSuccessFunction = 'FAKE ON SUCCESS FUNCTION';

[robliao, 2013/12/30 01:59:49]

Should use a fake function to simulate the correct function signature.

[vadimt, 2014/01/02 21:46:04]

Here we test that the value is simple passed to other functions. So, its
signature doesn't matter since the functions are not called.

Having said this, there are probably ways to compare functions, so I could pass
functions and expect functions, but this will result in longer code, so I don't
see big benefits in doing this.

+      var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION';

[robliao, 2013/12/30 01:59:49]

See optional argument above.

[vadimt, 2014/01/02 21:46:04]

Se above.

+
+      this.makeAndRegisterMockGlobals(
+          ['combineGroup', 'showNotificationCards']);
+
+      var combineGroupSavedArgs = new SaveMockArguments();
+      this.mockGlobals.expects(once()).
+          combineGroup(
+              combineGroupSavedArgs.match(eqJSON({})),
+              combineGroupSavedArgs.match(eqJSON({testField: 'TEST VALUE 1'}))).
+          will(callFunction(function() {
+            combineGroupSavedArgs.arguments[0].card1 = {
+              testValue: 'TEST CARD VALUE 1'
+            };
+          }));
+      this.mockGlobals.expects(once()).
+          combineGroup(
+              combineGroupSavedArgs.match(
+                  eqJSON({card1: {testValue: 'TEST CARD VALUE 1'}})),
+              combineGroupSavedArgs.match(
+                  eqJSON({testField: 'TEST VALUE 2'}))).
+          will(callFunction(function() {
+              combineGroupSavedArgs.arguments[0].card2 = {
+                testValue: 'TEST CARD VALUE 2'
+              };
+          }));
+      this.mockGlobals.expects(once()).
+          showNotificationCards(
+              eqJSON(testGroups),
+              eqJSON({
+                card1: {testValue: 'TEST CARD VALUE 1'},
+                card2: {testValue: 'TEST CARD VALUE 2'}
+              }),
+              fakeOnSuccessFunction,
+              fakeOnCardShownFunction);
+
+      // Invoking the tested function.
+      combineAndShowNotificationCards(
+          testGroups, fakeOnSuccessFunction, fakeOnCardShownFunction);
+    });
+
+TEST_F(
+    'GoogleNowBackgroundUnitTest',
+    'ProcessServerResponse',
+    function() {
+      // Tests processServerResponse function.
+
+      // Setup and expectations.
+      Date.now = function() { return 3000000; };
+
+      // GROUP1 was requested and contains cards c4 and c5. For c5, there is a
+      // non-expired dismissal, so it will be ignored.
+      // GROUP2 was not requested, but is contained in server response to
+      // indicate that the group still exists. Stored group GROUP2 won't change.
+      // GROUP3 is stored, but is not present in server's response, which means
+      // it doesn't exist anymore. This group will be deleted.
+      // GROUP4 doesn't contain cards, but it was requested. This is treated as
+      // if it had an empty array of cards. Cards in the stored group will be
+      // replaced with an empty array.
+      // GROUP5 doesn't have next poll time, and it will be stored without next
+      // poll time.
+      var serverResponse = {
+        groups: {
+          GROUP1: {requested: true, nextPollSeconds: 46},
+          GROUP2: {requested: false},
+          GROUP4: {requested: true, nextPollSeconds: 45},
+          GROUP5: {requested: true}
+        },
+        notifications: [
+          {notificationId: 'c4', groupName: 'GROUP1'},
+          {notificationId: 'c5', groupName: 'GROUP1'}
+        ]
+      };
+
+      var recentDismissals = {
+        c4: 1800000, // expired dismissal
+        c5: 1800001  // non-expired dismissal
+      };
+
+      var storedGroups = {
+        GROUP2: {
+          cards: [{notificationId: 'c2'}],
+          cardsTimestamp: 239,
+          nextPollTime: 10000
+        },
+        GROUP3: {
+          cards: [{notificationId: 'c3'}],
+          cardsTimestamp: 240,
+          nextPollTime: 10001
+        },
+        GROUP4: {
+          cards: [{notificationId: 'c44'}],

[robliao, 2013/12/30 01:59:49]

c4?

[vadimt, 2014/01/02 21:46:04]

No, I wanted it to be different from c4.

[robliao, 2014/01/03 09:09:34]

Document this so that future coders won't view this as a typo, or choose
something more different than c4. On 2014/01/02 21:46:04, vadimt wrote:

[vadimt, 2014/01/03 18:50:34]

Renamed.

+          cardsTimestamp: 241,
+          nextPollTime: 10002
+        }
+      };
+
+      var expectedUpdatedGroups = {
+        GROUP1: {
+          cards: [{notificationId: 'c4', groupName: 'GROUP1'}],
+          cardsTimestamp: 3000000,
+          nextPollTime: 3046000
+        },
+        GROUP2: {
+          cards: [{notificationId: 'c2'}],
+          cardsTimestamp: 239,
+          nextPollTime: 10000
+        },
+        GROUP4: {
+          cards: [],
+          cardsTimestamp: 3000000,
+          nextPollTime: 3045000
+        },
+        GROUP5: {
+          cards: [],
+          cardsTimestamp: 3000000
+        }
+      };
+
+      var expectedUpdatedRecentDismissals = {
+        c5: 1800001
+      };
+
+      var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION';
+
+      this.makeAndRegisterMockGlobals([
+        'scheduleNextPoll',
+        'combineAndShowNotificationCards',
+        'recordEvent'
+      ]);
+
+      this.makeAndRegisterMockApis([
+        'chrome.storage.local.set',
+        'instrumented.storage.local.get'
+      ]);
+
+      var storageGetSavedArgs = new SaveMockArguments();
+      this.mockApis.expects(once()).
+          instrumented_storage_local_get(
+              storageGetSavedArgs.match(
+                  eq(['notificationGroups', 'recentDismissals'])),
+              storageGetSavedArgs.match(ANYTHING)).
+          will(invokeCallback(
+              storageGetSavedArgs,
+              1,
+              {
+                notificationGroups: storedGroups,
+                recentDismissals: recentDismissals
+              }));
+
+      this.mockGlobals.expects(once()).
+          scheduleNextPoll(eqJSON(expectedUpdatedGroups), true);
+
+      var combineAndShowNotificationCardsSavedArgs = new SaveMockArguments();
+      this.mockGlobals.expects(once()).
+          combineAndShowNotificationCards(
+              combineAndShowNotificationCardsSavedArgs.match(
+                  eqJSON(expectedUpdatedGroups)),
+              combineAndShowNotificationCardsSavedArgs.match(
+                  ANYTHING),
+              combineAndShowNotificationCardsSavedArgs.match(
+                  eq(fakeOnCardShownFunction))).
+          will(invokeCallback(combineAndShowNotificationCardsSavedArgs, 1));
+
+      this.mockApis.expects(once()).
+          chrome_storage_local_set(
+              eqJSON({
+                notificationGroups: expectedUpdatedGroups,
+                recentDismissals: expectedUpdatedRecentDismissals}));
+
+      this.mockGlobals.expects(once()).
+          recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+
+      // Invoking the tested function.
+      processServerResponse(serverResponse, fakeOnCardShownFunction);
+    });
+
+TEST_F(
+    'GoogleNowBackgroundUnitTest',

[robliao, 2013/12/30 01:59:49]

Possible to generalize with the above?

[vadimt, 2014/01/02 21:46:04]

You probably mean factoring out the common part.
I'd say if we had 3 units, it would make sense. For 2 it's still OK to have
duplication.

+    'ProcessServerResponseGoogleNowDisabled',
+    function() {
+      // Tests processServerResponse function for the case when the response
+      // indicates that Google Now is disabled.
+
+      // Setup and expectations.
+      var serverResponse = {
+        googleNowDisabled: true,
+        groups: {
+          GROUP1: {nextPollTimeSeconds: 200}
+        }
+      };
+
+      var storedGroups = {
+        GROUP2: {
+          cards: [{notificationId: 'c2'}],
+          cardsTimestamp: 239,
+          nextPollTime: 10000
+        },
+        GROUP3: {
+          cards: [{notificationId: 'c3'}],
+          cardsTimestamp: 240,
+          nextPollTime: 10001
+        }
+      };
+
+      var expectedUpdatedGroups = {
+      };
+
+      var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION';
+
+      this.makeAndRegisterMockGlobals([
+        'scheduleNextPoll',
+        'combineAndShowNotificationCards',
+        'recordEvent',
+        'onStateChange'
+      ]);
+
+      this.makeAndRegisterMockApis([
+        'chrome.storage.local.set',
+        'instrumented.storage.local.get'
+      ]);
+
+      this.mockApis.expects(once()).
+          chrome_storage_local_set(
+              eqJSON({googleNowEnabled: false}));
+
+      this.mockGlobals.expects(once()).onStateChange();
+
+      var storageGetSavedArgs = new SaveMockArguments();
+      this.mockApis.expects(once()).
+          instrumented_storage_local_get(
+              storageGetSavedArgs.match(
+                  eq(['notificationGroups', 'recentDismissals'])),
+              storageGetSavedArgs.match(ANYTHING)).
+          will(invokeCallback(
+              storageGetSavedArgs, 1, {notificationGroups: storedGroups}));
+
+      this.mockGlobals.expects(once()).
+          scheduleNextPoll(eqJSON(expectedUpdatedGroups), false);
+
+      var combineAndShowNotificationCardsSavedArgs = new SaveMockArguments();
+      this.mockGlobals.expects(once()).
+          combineAndShowNotificationCards(
+              combineAndShowNotificationCardsSavedArgs.match(
+                  eqJSON(expectedUpdatedGroups)),
+              combineAndShowNotificationCardsSavedArgs.match(
+                  ANYTHING),
+              combineAndShowNotificationCardsSavedArgs.match(
+                  eq(fakeOnCardShownFunction))).
+          will(invokeCallback(combineAndShowNotificationCardsSavedArgs, 1));
+
+      this.mockApis.expects(once()).
+          chrome_storage_local_set(
+              eqJSON({
+                notificationGroups: expectedUpdatedGroups,
+                recentDismissals: {}}));
+
+      this.mockGlobals.expects(once()).
+          recordEvent(GoogleNowEvent.CARDS_PARSE_SUCCESS);
+
+      // Invoking the tested function.
+      processServerResponse(serverResponse, fakeOnCardShownFunction);
+    });