Unit tests for task manager

chrome/browser/resources/google_now/utility_unittest.gtestjs

Index: chrome/browser/resources/google_now/utility_unittest.gtestjs
diff --git a/chrome/browser/resources/google_now/utility_unittest.gtestjs b/chrome/browser/resources/google_now/utility_unittest.gtestjs
index 86cc6c6a302a32081c608fb3c18981502fa7d478..c7ffebeb7adc55490b46378c5113f51a0488c621 100644
--- a/chrome/browser/resources/google_now/utility_unittest.gtestjs
+++ b/chrome/browser/resources/google_now/utility_unittest.gtestjs
@@ -426,3 +426,289 @@ TEST_F('GoogleNowUtilityUnitTest',
              'onSuspendHandlerContainer.length must be 1');
   onSuspendHandlerContainer[0]();
 });
+
+var taskNameA = 'TASK A';

[robliao, 2013/08/29 21:15:38]

Why are these global? It would be good to isolate the tests.

[vadimt, 2013/08/29 21:42:47]

They are used by the tests below.
I could include them into the structure returned by setUpTaskManagerTest, but
they are just constants, setUpTaskManagerTest() returns stuff that cannot be
constant.

I feel that it's not worth to change anything. Isolation would be nice, but this
is a test, and also, even in background.js we don't try to isolate task names
from the code that doesn't need them.

+var taskNameB = 'TASK B';
+var taskNameC = 'TASK C';
+
+function areTasksConflicting(newTaskName, scheduledTaskName) {

[robliao, 2013/08/29 21:15:38]

Mock this.

[vadimt, 2013/08/29 21:42:47]

No clear what would be the benefit of this. The code becomes very clear when
areTasksConflicting() method is near task names.
Note also that we don't 'override' the original areTasksConflicting() because
that one is in background.js

+  return newTaskName == taskNameB && scheduledTaskName == taskNameA;
+}
+
+function setUpTaskManagerTest(fixture) {

[robliao, 2013/08/29 21:15:38]

Add banner comment.

[vadimt, 2013/08/29 21:42:47]

I believe this would be an overkill for a test helper that is local to a test
file.

+  wrapper = undefined;

[robliao, 2013/08/29 21:15:38]

Comment on why this statement is necessary.

[vadimt, 2013/08/29 21:42:47]

Done.

+  fixture.makeAndRegisterMockApis([
+    'wrapper.checkInWrappedCallback',
+    'wrapper.registerWrapperPluginFactory',
+    'wrapper.debugGetStateString'
+  ]);
+  fixture.makeMockLocalFunctions(['task1', 'task2', 'task3']);
+  fixture.makeAndRegisterMockGlobals(['reportError']);
+
+  fixture.mockApis.stubs().wrapper_checkInWrappedCallback();
+  fixture.mockApis.stubs().wrapper_debugGetStateString().
+      will(returnValue('testWrapperDebugState'));
+
+  var registerWrapperPluginFactorySavedArgs = new SaveMockArguments();
+  fixture.mockApis.expects(once()).wrapper_registerWrapperPluginFactory(
+      registerWrapperPluginFactorySavedArgs.match(ANYTHING));
+  var tasks = buildTaskManager(areTasksConflicting);
+  Mock4JS.verifyAllMocks();
+
+  return {
+    tasks: tasks,
+    pluginFactory: registerWrapperPluginFactorySavedArgs.arguments[0]
+  };
+}
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() {

[robliao, 2013/08/29 21:15:38]

Use a more descriptive name instead of 1-7.

[vadimt, 2013/08/29 21:42:47]

I tried, but this resulted in long boring identifiers.
Given that these names are not used anywhere and that there are comments
explaining what's happening, I felt that numbering would be sufficient, and I
remember seeing something like this in other places.

OK/not OK?

[robliao, 2013/08/30 17:34:09]

Short descriptive identifiers are better than 1-7. If a test fails, you can
immediately see the failure. With 1-7, developers will always need to refer to
the test code to determine the scenario that failed.
On 2013/08/29 21:42:47, vadimt wrote:

[vadimt, 2013/08/30 20:18:52]

Please suggest names (say, for first 3 tests), and I'll convert the rest.

[robliao, 2013/08/30 22:33:42]

1. TaskManager-2Sequential
2. TaskManager-Conflicting
3. TaskManager-NestedTaskEnqueue
On 2013/08/30 20:18:52, vadimt wrote:

[vadimt, 2013/08/30 22:59:32]

Done.

+  // Tests that 2 tasks get successfully executed consequentially, even if the
+  // second one conflicts with the first.
+  
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+
+  // Step 1. Adding 1st task that doesn't create pending callbacks.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING);
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Adding 2nd task.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task2(ANYTHING);
+  // Invocation.
+  test.tasks.add(taskNameB, this.mockLocalFunctions.functions().task2);
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager2', function() {
+  // Tests that adding a task while a conflicting task is being executed, causes
+  // the second one to be ignored.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var task1PluginInstance;
+
+  // Step 1. Adding 1st task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Adding 2nd task.
+  test.tasks.add(taskNameB, this.mockLocalFunctions.functions().task2);
+  Mock4JS.verifyAllMocks();
+
+  // Step 3. Entering the callback of task1.
+  task1PluginInstance.prologue();
+  Mock4JS.verifyAllMocks();

[robliao, 2013/08/29 21:15:38]

Why not just verify all mocks at the end?

[vadimt, 2013/08/29 21:42:47]

Then we are not able to differentiate between:
Invocation 1, causing Expectation 1;
Invocation 2, causing Expectation 2;

from
Invocation 1;
Invocation 2, causing Expectation 1 and Expectation 2;

[robliao, 2013/08/30 17:34:09]

There are no other tests in Chrome that call verifyAllMocks as many times as we
do within a single test.

This seems to indicate we would be better off breaking up these tests with each
scenario as a different test.

On 2013/08/29 21:42:47, vadimt wrote:

[vadimt, 2013/08/30 20:18:52]

I believe, we are in a situation when we set the standards, not simply following
someone's lead. Most of extensions don't have unit tests at all, and the
remaining have very few test.

The structure of the tests looks intuitively reasonable, and in most cases
cannot be broken into multiple tests because to execute Step N+1, you need Step
N, which has its own expectations. etc.

So, let's create best in the world unit tests and let others use them as an
example.

+
+  // Step 4. Leaving the callback of task1.
+  task1PluginInstance.epilogue();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager3', function() {
+  // Tests that adding a task while a non-conflicting task is being executed,
+  // causes the second one to be executed after the first one completes.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var task1PluginInstance;
+
+  // Step 1. Adding 1st task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Adding 2nd task.
+  test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2);
+  Mock4JS.verifyAllMocks();
+
+  // Step 3. Entering the callback of task1.
+  task1PluginInstance.prologue();
+  Mock4JS.verifyAllMocks();
+
+  // Step 4. Leaving the callback of task1.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task2(ANYTHING);
+  // Invocation.
+  task1PluginInstance.epilogue();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager4', function() {
+  // Tests that task manager correctly detects completion of tasks that create
+  // branching chains of callbacks.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var task1PluginInstance1, task1PluginInstance2, task1PluginInstance3;
+
+  // Step 1. Adding 1st task that creates a 2 pending callbacks.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance1 = test.pluginFactory();
+        task1PluginInstance2 = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Adding 2nd task.
+  test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2);
+  Mock4JS.verifyAllMocks();
+
+  // Step 3. Entering callback 1, create pending callback 3, exit callback 1.
+  // Enter/exit callback 2. Enter callback 3.
+  task1PluginInstance1.prologue();
+  task1PluginInstance3 = test.pluginFactory();
+  task1PluginInstance1.epilogue();
+  task1PluginInstance2.prologue();
+  task1PluginInstance2.epilogue();
+  task1PluginInstance3.prologue();
+  Mock4JS.verifyAllMocks();
+
+  // Step 4. Leaving 3rd callback of task1. Now task1 is complete, and task2
+  // should start.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task2(ANYTHING);
+  // Invocation.
+  task1PluginInstance3.epilogue();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager5', function() {
+  // Tests that task manager's onSuspend method reports an error if there are
+  // pending tasks.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var onSuspendHandlerContainer = getMockHandlerContainer('runtime.onSuspend');
+
+  // Step 1. Adding a task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Invoke onSuspend event of the task manager.
+  // Setup and expectations. The 2 callbacks in onSuspendHandlerContainer are
+  // from the wrapper and the task manager.
+  assertTrue(onSuspendHandlerContainer.length == 2,
+             'onSuspendHandlerContainer.length must be 2');
+  this.mockGlobals.expects(once()).reportError(eqToString(
+      'Error: ASSERT: Incomplete task when unloading event page,' +
+      ' queue = [{"name":"TASK A"}], testWrapperDebugState'));
+  // Invocation.
+  onSuspendHandlerContainer[1]();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager6', function() {
+  // Tests that task manager's onSuspend method does not reports an error if all
+  // tasks completed.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var onSuspendHandlerContainer = getMockHandlerContainer('runtime.onSuspend');
+  var task1PluginInstance;
+
+  // Step 1. Adding a task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Invoke task's callback and the onSuspend event of the task manager.
+  // The 2 callbacks in onSuspendHandlerContainer are from the wrapper and the
+  // task manager.
+  task1PluginInstance.prologue();
+  task1PluginInstance.epilogue();
+  onSuspendHandlerContainer[1]();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager7', function() {
+  // Tests that 3 tasks can be executed too. In particular, that if the second 
+  // task is a single-step task which execution was originally blocked by task1,
+  // unblocking it causes immediate synchronous execution of both tasks 2 and 3.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var task1PluginInstance;
+
+  // Step 1. Adding 1st task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Adding 2nd and 3rd tasks.
+  test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2);
+  test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task3);
+  Mock4JS.verifyAllMocks();
+
+  // Step 3. Entering the callback of task1.
+  task1PluginInstance.prologue();
+  Mock4JS.verifyAllMocks();
+
+  // Step 4. Leaving the callback of task1.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task2(ANYTHING);
+  this.mockLocalFunctions.expects(once()).task3(ANYTHING);
+  // Invocation.
+  task1PluginInstance.epilogue();
+});
+
+TEST_F('GoogleNowUtilityUnitTest', 'TaskManager8', function() {
+  // Tests callbacks requested while a task is running, but not from a callback
+  // belonging to a task, are not counted as a part of the task.
+
+  // Setup.
+  var test = setUpTaskManagerTest(this);
+  var task1PluginInstance, nonTaskPluginInstance;
+
+  // Step 1. Adding 1st task that creates a pending callback.
+  // Expectations.
+  this.mockLocalFunctions.expects(once()).task1(ANYTHING).
+      will(callFunction(function() {
+        task1PluginInstance = test.pluginFactory();
+      }));
+  // Invocation.
+  test.tasks.add(taskNameA, this.mockLocalFunctions.functions().task1);
+  Mock4JS.verifyAllMocks();
+
+  // Step 2. Code that is not a part of the task creates a pending callback.
+  nonTaskPluginInstance = test.pluginFactory();
+  Mock4JS.verifyAllMocks();
+
+  // Step 3. Entering the callback of task1. After this, task1 should be
+  // finished despite the pending non-task callback.
+  task1PluginInstance.prologue();
+  task1PluginInstance.epilogue();
+  Mock4JS.verifyAllMocks();
+
+  // Step 4. Checking that task1 is finished by submitting task2, which should
+  // be executed immediately.
+  this.mockLocalFunctions.expects(once()).task2(ANYTHING);
+  test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2);
+});