Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
121983002:
Restoring recently deleted unit tests (Closed)
|
Created:
6 years, 12 months ago by vadimt Modified:
6 years, 11 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRestoring recently deleted unit tests.
Some unit tests were deleted during switching from merging to combining. Restoring them.
Other changes:
1. Switching server response parameter from string to a typed one in parseAndShowNotificationCards(). This way tests don’t have to stringify this parameter before passing to the tested function.
2. Changes in cards.js to match ‘actionUrls’ field definitions in annotation types. They can’t be null.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244624
Patch Set 1 #
Total comments: 38
Patch Set 2 : robliao@ comments #Patch Set 3 : More robliao@ comments #
Total comments: 4
Patch Set 4 : rgustafson's notes #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); To reduce test churn do this refactor first: https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:480: 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 ? https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:481: TEST_F( Comment targeted test behavior and expectations. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:502: var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION'; Is this necessary? This parameter used for the functions used below is optional. The expectation is that this is either a function or undefined. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:566: // Tests combineGroup function. Comment targeted test behavior and expectations. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:631: var fakeOnSuccessFunction = 'FAKE ON SUCCESS FUNCTION'; Should use a fake function to simulate the correct function signature. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:632: var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION'; See optional argument above. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:723: cards: [{notificationId: 'c44'}], c4? https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:810: 'GoogleNowBackgroundUnitTest', Possible to generalize with the above? https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:31: var testNotificationA = { Including the priority in the name would be useful when using these notifications in priority tests. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:97: notificationId: testNotificationIdA, Add a few tests that vary the optional arguments in the received notification. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:117: will(invokeCallback(chromeNotificationsCreateSavedArgs, 2, true)); Useful comment here: the true here indicates that the card exists. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:350: // showTime in future, and it doesn't get dismissed. What do you mean in the third line? https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:417: // showTime in future, and it doesn't get dismissed. should not be dismissed.
Thanks for so detailed comments! https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); On 2013/12/30 01:59:49, robliao wrote: > To reduce test churn do this refactor first: > > https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:480: On 2013/12/30 01:59:49, robliao wrote: > 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 ? 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:481: TEST_F( On 2013/12/30 01:59:49, robliao wrote: > Comment targeted test behavior and expectations. Done. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:502: var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION'; On 2013/12/30 01:59:49, robliao wrote: > Is this necessary? This parameter used for the functions used below is optional. > The expectation is that this is either a function or undefined. 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:566: // Tests combineGroup function. On 2013/12/30 01:59:49, robliao wrote: > Comment targeted test behavior and expectations. Done. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:631: var fakeOnSuccessFunction = 'FAKE ON SUCCESS FUNCTION'; On 2013/12/30 01:59:49, robliao wrote: > Should use a fake function to simulate the correct function signature. 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:632: var fakeOnCardShownFunction = 'FAKE ON CARD SHOWN FUNCTION'; On 2013/12/30 01:59:49, robliao wrote: > See optional argument above. Se above. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:723: cards: [{notificationId: 'c44'}], On 2013/12/30 01:59:49, robliao wrote: > c4? No, I wanted it to be different from c4. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:810: 'GoogleNowBackgroundUnitTest', On 2013/12/30 01:59:49, robliao wrote: > Possible to generalize with the above? 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:31: var testNotificationA = { On 2013/12/30 01:59:49, robliao wrote: > Including the priority in the name would be useful when using these > notifications in priority tests. There are not so many tests that care about priority, and given that I'd have to accordingly change all other names (like testActionUrlsGroupPrio1), which would make the code less clear, we probably shouldn't do this. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:97: notificationId: testNotificationIdA, On 2013/12/30 01:59:49, robliao wrote: > Add a few tests that vary the optional arguments in the received notification. See the above comment about detailness of testing. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:117: will(invokeCallback(chromeNotificationsCreateSavedArgs, 2, true)); On 2013/12/30 01:59:49, robliao wrote: > Useful comment here: the true here indicates that the card exists. I added the constant for 'true', hope this will work for you. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:350: // showTime in future, and it doesn't get dismissed. On 2013/12/30 01:59:49, robliao wrote: > What do you mean in the third line? Nothing ;-) https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:417: // showTime in future, and it doesn't get dismissed. On 2013/12/30 01:59:49, robliao wrote: > should not be dismissed. Done.
https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); 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: > On 2013/12/30 01:59:49, robliao wrote: > > To reduce test churn do this refactor first: > > > > > https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... > > 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:480: 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: > On 2013/12/30 01:59:49, robliao wrote: > > 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 ? > > 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. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:723: cards: [{notificationId: 'c44'}], 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: > On 2013/12/30 01:59:49, robliao wrote: > > c4? > > No, I wanted it to be different from c4. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/cards_unittest.gtestjs:117: will(invokeCallback(chromeNotificationsCreateSavedArgs, 2, true)); Even better! On 2014/01/02 21:46:04, vadimt wrote: > On 2013/12/30 01:59:49, robliao wrote: > > Useful comment here: the true here indicates that the card exists. > > I added the constant for 'true', hope this will work for you.
https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); I wonder what other reviewers think. The proposal is essentially about the style; you can do this either way. Thoughts? https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:480: 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... https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:723: cards: [{notificationId: 'c44'}], On 2014/01/03 09:09:34, robliao wrote: > 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: > > On 2013/12/30 01:59:49, robliao wrote: > > > c4? > > > > No, I wanted it to be different from c4. > Renamed.
https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); On 2014/01/03 18:50:34, vadimt wrote: > I wonder what other reviewers think. The proposal is essentially about the > style; you can do this either way. Thoughts? Let me look at this more in depth after lunch. https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:480: On 2014/01/03 18:50:34, vadimt wrote: > 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... 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. https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:445: * Based on a response response from the notification server, shows response x2 https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:198: /** @type {?UncombinedNotification} */ |undefined rather than ?nullable
https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:445: * Based on a response response from the notification server, shows On 2014/01/07 20:36:45, rgustafson wrote: > response x2 Done. https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/121983002/diff/150001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:198: /** @type {?UncombinedNotification} */ On 2014/01/07 20:36:45, rgustafson wrote: > |undefined rather than ?nullable Done.
lgtm https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/121983002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background_unittest.gtestjs:478: testNotificationId, this.mockLocalFunctions.functions().selector); 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: > On 2014/01/03 18:50:34, vadimt wrote: > > I wonder what other reviewers think. The proposal is essentially about the > > style; you can do this either way. Thoughts? > > Let me look at this more in depth after lunch.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/121983002/220001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/121983002/220001
Message was sent while issue was closed.
Change committed as 244624 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
