Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23623010:
Unit tests for task manager (Closed)
|
Created:
7 years, 3 months ago by vadimt Modified:
7 years, 3 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUnit tests for task manager.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222099
Patch Set 1 #
Total comments: 18
Patch Set 2 : robliao@ comments #Patch Set 3 : More comments. #
Total comments: 22
Patch Set 4 : robliao@ comments #Patch Set 5 : More comments #Patch Set 6 : AUTO #
Total comments: 12
Patch Set 7 : skare@ comments #Patch Set 8 : skare@ comments #Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:430: var taskNameA = 'TASK A'; Why are these global? It would be good to isolate the tests. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:434: function areTasksConflicting(newTaskName, scheduledTaskName) { Mock this. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:438: function setUpTaskManagerTest(fixture) { Add banner comment. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:439: wrapper = undefined; Comment on why this statement is necessary. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { Use a more descriptive name instead of 1-7. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:509: Mock4JS.verifyAllMocks(); Why not just verify all mocks at the end?
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:430: var taskNameA = 'TASK A'; On 2013/08/29 21:15:38, Robert Liao wrote: > Why are these global? It would be good to isolate the tests. 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. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:434: function areTasksConflicting(newTaskName, scheduledTaskName) { On 2013/08/29 21:15:38, Robert Liao wrote: > Mock this. 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 https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:438: function setUpTaskManagerTest(fixture) { On 2013/08/29 21:15:38, Robert Liao wrote: > Add banner comment. I believe this would be an overkill for a test helper that is local to a test file. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:439: wrapper = undefined; On 2013/08/29 21:15:38, Robert Liao wrote: > Comment on why this statement is necessary. Done. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { On 2013/08/29 21:15:38, Robert Liao wrote: > Use a more descriptive name instead of 1-7. 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? https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:509: Mock4JS.verifyAllMocks(); On 2013/08/29 21:15:38, Robert Liao wrote: > Why not just verify all mocks at the end? 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;
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { 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: > On 2013/08/29 21:15:38, Robert Liao wrote: > > Use a more descriptive name instead of 1-7. > > 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? https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:509: Mock4JS.verifyAllMocks(); 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: > On 2013/08/29 21:15:38, Robert Liao wrote: > > Why not just verify all mocks at the end? > > 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;
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { On 2013/08/30 17:34:09, Robert Liao wrote: > 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: > > On 2013/08/29 21:15:38, Robert Liao wrote: > > > Use a more descriptive name instead of 1-7. > > > > 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? > Please suggest names (say, for first 3 tests), and I'll convert the rest. https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:509: Mock4JS.verifyAllMocks(); On 2013/08/30 17:34:09, Robert Liao wrote: > 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: > > On 2013/08/29 21:15:38, Robert Liao wrote: > > > Why not just verify all mocks at the end? > > > > 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; > 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.
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { 1. TaskManager-2Sequential 2. TaskManager-Conflicting 3. TaskManager-NestedTaskEnqueue On 2013/08/30 20:18:52, vadimt wrote: > On 2013/08/30 17:34:09, Robert Liao wrote: > > 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: > > > On 2013/08/29 21:15:38, Robert Liao wrote: > > > > Use a more descriptive name instead of 1-7. > > > > > > 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? > > > > Please suggest names (say, for first 3 tests), and I'll convert the rest.
https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:464: TEST_F('GoogleNowUtilityUnitTest', 'TaskManager1', function() { On 2013/08/30 22:33:42, Robert Liao wrote: > 1. TaskManager-2Sequential > 2. TaskManager-Conflicting > 3. TaskManager-NestedTaskEnqueue > On 2013/08/30 20:18:52, vadimt wrote: > > On 2013/08/30 17:34:09, Robert Liao wrote: > > > 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: > > > > On 2013/08/29 21:15:38, Robert Liao wrote: > > > > > Use a more descriptive name instead of 1-7. > > > > > > > > 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? > > > > > > > Please suggest names (say, for first 3 tests), and I'll convert the rest. > Done.
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); It is worth a discussion as to what makes this test different form the one above. All that appears to be different is some calls to prologue, epilogue, and no mention of task 2 except for the addition. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:538: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); Why not taskNameB? https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:554: // branching chains of callbacks. Detail the tested branching pattern. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:572: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); taskNameB? https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:629: var onSuspendHandlerContainer = getMockHandlerContainer('runtime.onSuspend'); Where is getMockHandlerContainer defined? Code search doesn't seem to find it. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:670: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); taskNameB? https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:705: nonTaskPluginInstance = test.pluginFactory(); This variable is never read. Necessary?
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); On 2013/09/03 23:23:31, Robert Liao wrote: > It is worth a discussion as to what makes this test different form the one > above. > > All that appears to be different is some calls to prologue, epilogue, and no > mention of task 2 except for the addition. You mean what's the difference from TaskManager2Sequential? If so, the difference is that TaskManager2Sequential runs 2 tasks one after another. TaskManagerConflicting creates a task with a pending callback, then creates a conflicting task, then completes the pending callback, and the second task should not be executed. So, they are very different, but may be, I'm missing your point :) https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:538: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); On 2013/09/03 23:23:31, Robert Liao wrote: > Why not taskNameB? taskNameB conflicts with taskNameA, while taskNameC does not. We are checking the non-conflicting case here... https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:554: // branching chains of callbacks. On 2013/09/03 23:23:31, Robert Liao wrote: > Detail the tested branching pattern. Done. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:572: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); On 2013/09/03 23:23:31, Robert Liao wrote: > taskNameB? Need 'C' because 'B' is conflicting and would be ignored. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:629: var onSuspendHandlerContainer = getMockHandlerContainer('runtime.onSuspend'); On 2013/09/03 23:23:31, Robert Liao wrote: > Where is getMockHandlerContainer defined? Code search doesn't seem to find it. In https://codereview.chromium.org/23477006/diff/36001/chrome/browser/resources/... which is now in CQ. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:670: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); On 2013/09/03 23:23:31, Robert Liao wrote: > taskNameB? Se above about conflicts. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:705: nonTaskPluginInstance = test.pluginFactory(); On 2013/09/03 23:23:31, Robert Liao wrote: > This variable is never read. Necessary? Thanks!
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); That's the idea. A bit of the mechanics should be captured in comments here to future readers. On 2013/09/04 00:29:16, vadimt wrote: > On 2013/09/03 23:23:31, Robert Liao wrote: > > It is worth a discussion as to what makes this test different form the one > > above. > > > > All that appears to be different is some calls to prologue, epilogue, and no > > mention of task 2 except for the addition. > > You mean what's the difference from TaskManager2Sequential? > If so, the difference is that TaskManager2Sequential runs 2 tasks one after > another. > TaskManagerConflicting creates a task with a pending callback, then creates a > conflicting task, then completes the pending callback, and the second task > should not be executed. > > So, they are very different, but may be, I'm missing your point :) https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:538: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); Future readers will ask this question since the conflict code is way above. Refer to the conflict code through comments when necessary to capture This detail. On 2013/09/04 00:29:16, vadimt wrote: > On 2013/09/03 23:23:31, Robert Liao wrote: > > Why not taskNameB? > > taskNameB conflicts with taskNameA, while taskNameC does not. We are checking > the non-conflicting case here... https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:572: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); That's not obvious from a reading of the test case. Comment the conflict or rename the variables so that a conflict is clear. On 2013/09/04 00:29:16, vadimt wrote: > On 2013/09/03 23:23:31, Robert Liao wrote: > > taskNameB? > > Need 'C' because 'B' is conflicting and would be ignored.
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); On 2013/09/04 00:54:00, Robert Liao wrote: > That's the idea. A bit of the mechanics should be captured in comments here to > future readers. > > On 2013/09/04 00:29:16, vadimt wrote: > > On 2013/09/03 23:23:31, Robert Liao wrote: > > > It is worth a discussion as to what makes this test different form the one > > > above. > > > > > > All that appears to be different is some calls to prologue, epilogue, and no > > > mention of task 2 except for the addition. > > > > You mean what's the difference from TaskManager2Sequential? > > If so, the difference is that TaskManager2Sequential runs 2 tasks one after > > another. > > TaskManagerConflicting creates a task with a pending callback, then creates a > > conflicting task, then completes the pending callback, and the second task > > should not be executed. > > > > So, they are very different, but may be, I'm missing your point :) > Done. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:538: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); On 2013/09/04 00:54:00, Robert Liao wrote: > Future readers will ask this question since the conflict code is way above. > Refer to the conflict code through comments when necessary to capture > This detail. > > On 2013/09/04 00:29:16, vadimt wrote: > > On 2013/09/03 23:23:31, Robert Liao wrote: > > > Why not taskNameB? > > > > taskNameB conflicts with taskNameA, while taskNameC does not. We are checking > > the non-conflicting case here... > Done. https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:572: test.tasks.add(taskNameC, this.mockLocalFunctions.functions().task2); On 2013/09/04 00:54:00, Robert Liao wrote: > That's not obvious from a reading of the test case. Comment the conflict or > rename the variables so that a conflict is clear. > On 2013/09/04 00:29:16, vadimt wrote: > > On 2013/09/03 23:23:31, Robert Liao wrote: > > > taskNameB? > > > > Need 'C' because 'B' is conflicting and would be ignored. > Done.
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); Where was this done? On 2013/09/04 01:40:15, vadimt wrote: > On 2013/09/04 00:54:00, Robert Liao wrote: > > That's the idea. A bit of the mechanics should be captured in comments here to > > future readers. > > > > On 2013/09/04 00:29:16, vadimt wrote: > > > On 2013/09/03 23:23:31, Robert Liao wrote: > > > > It is worth a discussion as to what makes this test different form the one > > > > above. > > > > > > > > All that appears to be different is some calls to prologue, epilogue, and > no > > > > mention of task 2 except for the addition. > > > > > > You mean what's the difference from TaskManager2Sequential? > > > If so, the difference is that TaskManager2Sequential runs 2 tasks one after > > > another. > > > TaskManagerConflicting creates a task with a pending callback, then creates > a > > > conflicting task, then completes the pending callback, and the second task > > > should not be executed. > > > > > > So, they are very different, but may be, I'm missing your point :) > > > > Done.
https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:512: task1PluginInstance.prologue(); On 2013/09/04 17:02:03, Robert Liao wrote: > Where was this done? > On 2013/09/04 01:40:15, vadimt wrote: > > On 2013/09/04 00:54:00, Robert Liao wrote: > > > That's the idea. A bit of the mechanics should be captured in comments here > to > > > future readers. > > > > > > On 2013/09/04 00:29:16, vadimt wrote: > > > > On 2013/09/03 23:23:31, Robert Liao wrote: > > > > > It is worth a discussion as to what makes this test different form the > one > > > > > above. > > > > > > > > > > All that appears to be different is some calls to prologue, epilogue, > and > > no > > > > > mention of task 2 except for the addition. > > > > > > > > You mean what's the difference from TaskManager2Sequential? > > > > If so, the difference is that TaskManager2Sequential runs 2 tasks one > after > > > > another. > > > > TaskManagerConflicting creates a task with a pending callback, then > creates > > a > > > > conflicting task, then completes the pending callback, and the second task > > > > should not be executed. > > > > > > > > So, they are very different, but may be, I'm missing your point :) > > > > > > > Done. > CR tool may not show you the latest uploaded version of the source with the comment. Please see the same test here: https://codereview.chromium.org/23623010/diff/18001/chrome/browser/resources/...
lgtm
AUTO
On 2013/09/04 20:14:06, vadimt wrote: > AUTO Please ignore that 'AUTO' comment. I ran a wrong tool...
functionality looks good. here's some comment comments https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:474: // Tests that 2 tasks get successfully executed consequentially, even if the consequentially -> consecutively? https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:480: // Step 1. Adding 1st task that doesn't create pending callbacks. tiny nit, optional: 1st -> first, 2nd -> second below, and similar in the next function(s) [spelling out seems to be more common] https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:495: // Tests that adding a task while a conflicting task is being executed, causes tiny nit: no comma after "executed" https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:526: // Tests that adding a task while a non-conflicting task is being executed, no , after executed https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:568: // Step 1. Adding 1st task that creates a 2 pending callbacks. (through file) unify verb forms/moods. Adding -> add, entering->enter, leaving->leave etc. Then it will match the majority of them that are already that way (enter, exit) https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:633: // Tests that task manager's onSuspend method does not reports an error if all report an error
https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:474: // Tests that 2 tasks get successfully executed consequentially, even if the On 2013/09/05 19:50:34, Travis Skare wrote: > consequentially -> consecutively? Done. https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:480: // Step 1. Adding 1st task that doesn't create pending callbacks. On 2013/09/05 19:50:34, Travis Skare wrote: > tiny nit, optional: 1st -> first, 2nd -> second below, and similar in the next > function(s) [spelling out seems to be more common] Yeah, but since I use numbers in task names, I'd prefer to leave numbers in the comment... https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:495: // Tests that adding a task while a conflicting task is being executed, causes On 2013/09/05 19:50:34, Travis Skare wrote: > tiny nit: no comma after "executed" Done. https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:526: // Tests that adding a task while a non-conflicting task is being executed, On 2013/09/05 19:50:34, Travis Skare wrote: > no , after executed Done. https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:568: // Step 1. Adding 1st task that creates a 2 pending callbacks. On 2013/09/05 19:50:34, Travis Skare wrote: > (through file) unify verb forms/moods. Adding -> add, entering->enter, > leaving->leave etc. Then it will match the majority of them that are already > that way (enter, exit) Done. https://codereview.chromium.org/23623010/diff/20001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:633: // Tests that task manager's onSuspend method does not reports an error if all On 2013/09/05 19:50:34, Travis Skare wrote: > report an error Done.
lgtm
arv@, please provide OWNER's approval
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/23623010/33001
Message was sent while issue was closed.
Change committed as 222099 |
