Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23751004:
Unit tests for attempt 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 attempt manager.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222189
Patch Set 1 #
Total comments: 20
Patch Set 2 : robliao@ comments #
Total comments: 2
Patch Set 3 : More comments. #
Total comments: 5
Patch Set 4 : skare@ comments #
Total comments: 2
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:722: var testAttemptStorageKey = 'current-delay-testAttempts'; This literal (5 uses) is used more than the variable (4 uses). Your choice: All var access (structuring the object creation to use the variable) or all literal access (remove the variable). https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:725: var testRandomValue = 0.31415926; Comment why this is called random value and why we can't use random seeding. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:778: var extectedRetryDelaySeconds = Typo: expectedRetryDelaySeconds https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:820: var testFirstDelaySeconds = 1039; Why not use testInitialDelaySeconds? https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:837: // Tests that retry time grows exponentially. It may be useful to have a test that grows this more than once (x^3 in addition to x^2) https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:846: var extectedRetryDelaySeconds = Typo (see above) https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:879: var extectedRetryDelaySeconds = testMaximumDelaySeconds; Typo (see above) https://codereview.chromium.org/23751004/diff/1/chrome/test/data/webui/test_a... File chrome/test/data/webui/test_api.js (left): https://codereview.chromium.org/23751004/diff/1/chrome/test/data/webui/test_a... chrome/test/data/webui/test_api.js:575: assertEquals(undefined, namespace[fieldName]); Why is this getting removed?
https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:722: var testAttemptStorageKey = 'current-delay-testAttempts'; On 2013/09/04 20:17:25, Robert Liao wrote: > This literal (5 uses) is used more than the variable (4 uses). > Your choice: All var access (structuring the object creation to use the > variable) or all literal access (remove the variable). Done. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:725: var testRandomValue = 0.31415926; On 2013/09/04 20:17:25, Robert Liao wrote: > Comment why this is called random value and why we can't use random seeding. Done. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:778: var extectedRetryDelaySeconds = On 2013/09/04 20:17:25, Robert Liao wrote: > Typo: expectedRetryDelaySeconds Done. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:820: var testFirstDelaySeconds = 1039; On 2013/09/04 20:17:25, Robert Liao wrote: > Why not use testInitialDelaySeconds? To verify that the implementation doesn't use testInitialDelaySeconds by mistake. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:837: // Tests that retry time grows exponentially. On 2013/09/04 20:17:25, Robert Liao wrote: > It may be useful to have a test that grows this more than once (x^3 in addition > to x^2) Given that the object doesn't have state, and we check all its inputs and outputs, no. Please let me know if you need more detailed explanation. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:846: var extectedRetryDelaySeconds = On 2013/09/04 20:17:25, Robert Liao wrote: > Typo (see above) Done. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:879: var extectedRetryDelaySeconds = testMaximumDelaySeconds; On 2013/09/04 20:17:25, Robert Liao wrote: > Typo (see above) Done. https://codereview.chromium.org/23751004/diff/1/chrome/test/data/webui/test_a... File chrome/test/data/webui/test_api.js (left): https://codereview.chromium.org/23751004/diff/1/chrome/test/data/webui/test_a... chrome/test/data/webui/test_api.js:575: assertEquals(undefined, namespace[fieldName]); On 2013/09/04 20:17:25, Robert Liao wrote: > Why is this getting removed? It became too restrictive. Sometimes (I have 2 cases), the API already exists, and you really want to override the existing call. Removing this assert allows to avoid writing something like "instrumented.api.function = undefined;" before mocking.
https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:820: var testFirstDelaySeconds = 1039; Change the test name to AttemptManagerStartWithDelayParam and update the comment as well s/parameter/delay parameter/ On 2013/09/04 22:34:40, vadimt wrote: > On 2013/09/04 20:17:25, Robert Liao wrote: > > Why not use testInitialDelaySeconds? > > To verify that the implementation doesn't use testInitialDelaySeconds by > mistake. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:837: // Tests that retry time grows exponentially. Makes sense. Consider updating the comment with this fact. On 2013/09/04 22:34:40, vadimt wrote: > On 2013/09/04 20:17:25, Robert Liao wrote: > > It may be useful to have a test that grows this more than once (x^3 in > addition > > to x^2) > > Given that the object doesn't have state, and we check all its inputs and > outputs, no. > Please let me know if you need more detailed explanation. https://codereview.chromium.org/23751004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:728: // result of Math.random(). Not exactly true. A fixed seed would be okay, but fixed seeding isn't possible in JS at the moment.
https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:820: var testFirstDelaySeconds = 1039; On 2013/09/05 00:09:39, Robert Liao wrote: > Change the test name to AttemptManagerStartWithDelayParam and update the comment > as well s/parameter/delay parameter/ > On 2013/09/04 22:34:40, vadimt wrote: > > On 2013/09/04 20:17:25, Robert Liao wrote: > > > Why not use testInitialDelaySeconds? > > > > To verify that the implementation doesn't use testInitialDelaySeconds by > > mistake. > Done. https://codereview.chromium.org/23751004/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility_unittest.gtestjs:837: // Tests that retry time grows exponentially. On 2013/09/05 00:09:39, Robert Liao wrote: > Makes sense. Consider updating the comment with this fact. > On 2013/09/04 22:34:40, vadimt wrote: > > On 2013/09/04 20:17:25, Robert Liao wrote: > > > It may be useful to have a test that grows this more than once (x^3 in > > addition > > > to x^2) > > > > Given that the object doesn't have state, and we check all its inputs and > > outputs, no. > > Please let me know if you need more detailed explanation. > Done. https://codereview.chromium.org/23751004/diff/6001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/6001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility_unittest.gtestjs:728: // result of Math.random(). On 2013/09/05 00:09:39, Robert Liao wrote: > Not exactly true. A fixed seed would be okay, but fixed seeding isn't possible > in JS at the moment. Done.
lgtm https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:831: // Tests starting an attempt manager with a delay parameter. The indentation is technically insufficient here, but I think this works better...
thanks for tests https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:731: function createTestAttempStorageEntry(delaySeconds) { attemp -> attempt https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:774: // Step 1. Checking that attempt manager is not running. (through file) checking -> check, etc as in other CL
https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:731: function createTestAttempStorageEntry(delaySeconds) { On 2013/09/05 19:59:09, Travis Skare wrote: > attemp -> attempt Done. https://codereview.chromium.org/23751004/diff/12001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:774: // Step 1. Checking that attempt manager is not running. On 2013/09/05 19:59:09, Travis Skare wrote: > (through file) > checking -> check, etc as in other CL Done.
lgtm
arv@, please provide OWNER's approval
LGTM https://codereview.chromium.org/23751004/diff/18001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/18001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:729: var testRandomValue = 0.31415926; FYI, we have a bunch of these already: https://code.google.com/p/chromium/codesearch#search/&q=Math.random%5Cs=&sq=p...
https://codereview.chromium.org/23751004/diff/18001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility_unittest.gtestjs (right): https://codereview.chromium.org/23751004/diff/18001/chrome/browser/resources/... chrome/browser/resources/google_now/utility_unittest.gtestjs:729: var testRandomValue = 0.31415926; On 2013/09/09 13:55:10, arv wrote: > FYI, we have a bunch of these already: > > https://code.google.com/p/chromium/codesearch#search/&q=Math.random%255Cs=&sq... Thanks, that's nice to know! However, I don't see how they immediately could be useful here.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/23751004/18001
Message was sent while issue was closed.
Change committed as 222189 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
