Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1845253002:
Add unit test for Site Settings Handler. (Closed)
|
Created:
4 years, 8 months ago by Finnur Modified:
4 years, 8 months ago Reviewers:
michaelpg CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unit test for Site Settings Handler.
BUG=543635
Committed: https://crrev.com/f5ca600dbd28d6c3c8e528e35b0fb84efdcd309e
Cr-Commit-Position: refs/heads/master@{#385296}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address feedback #
Total comments: 1
Patch Set 3 : Address feedback #
Messages
Total messages: 15 (5 generated)
finnur@chromium.org changed reviewers: + michaelpg@chromium.org
Michael, mind taking this?
https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:38: EXPECT_EQ("test-callback-id", callback_id); opt. nit: make the callback id a member or static https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:39: should you check the call was successful via arg2()? https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:94: scoped_ptr<TestingProfile> profile_; can this just be a "TestingProfile profile_" member rather than a pointer? https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:99: scoped_ptr<SiteSettingsHandler> handler; SiteSettingsHandler handler(profile()); https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:118: EXPECT_EQ(2U, web_ui()->call_data().size()); is this the change event? why not verify the callback here too? https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:126: scoped_ptr<SiteSettingsHandler> handler; SiteSettingsHandler handler(profile());
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:38: EXPECT_EQ("test-callback-id", callback_id); On 2016/04/01 09:08:28, michaelpg wrote: > opt. nit: make the callback id a member or static Done. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:39: On 2016/04/01 09:08:28, michaelpg wrote: > should you check the call was successful via arg2()? Done. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:94: scoped_ptr<TestingProfile> profile_; On 2016/04/01 09:08:28, michaelpg wrote: > can this just be a "TestingProfile profile_" member rather than a pointer? Done. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:99: scoped_ptr<SiteSettingsHandler> handler; On 2016/04/01 09:08:28, michaelpg wrote: > SiteSettingsHandler handler(profile()); Done. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:118: EXPECT_EQ(2U, web_ui()->call_data().size()); Umm, I'm not sure which callback you are referring to... HandleSetDefaultValueForContentType does not call back into JavaScript. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:126: scoped_ptr<SiteSettingsHandler> handler; On 2016/04/01 09:08:28, michaelpg wrote: > SiteSettingsHandler handler(profile()); Done.
lgtm https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:118: EXPECT_EQ(2U, web_ui()->call_data().size()); On 2016/04/05 10:22:31, Finnur wrote: > Umm, I'm not sure which callback you are referring to... > HandleSetDefaultValueForContentType does not call back into JavaScript. so where does this 2nd call into webui come from? I'm just confused. https://codereview.chromium.org/1845253002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:18: const char kCallbackId[] = "test-callback-id"; nit: no indent
Updated and checking in. I'll respond to the callback question in a bit.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/1845253002/#ps60001 (title: "Address feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1845253002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1845253002/60001
Attempting to clarify: https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:110: ValidateDefault(true, 1U); This validates that we've had 1 call into web_ui (a call to get the default value) and that the callback back to JS returned true. https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:118: EXPECT_EQ(2U, web_ui()->call_data().size()); This validates that we've had 2 calls from JS into web_ui (first a call to get a default value and now another, to set the default value). If SetDefault were to result in a callback call back into to js (which it doesn't) then I would have created a helper that runs line 118 and then also validates the result of the callback. But since there is no callback, it suffices to check the number of calls into web_ui. To clarify: The term callback, that I use, is referring only to web_ui communicating back to js with what the result of the web_ui call was. Another way to think about it: If GetDefault on line 109 did not return a callback then I'd replace the ValidateDefault function on line 110 with: EXPECT_EQ(1U, web_ui()->call_data().size()); Does this answer your question? Happy to do a follow-up if you think something needs clarification/improvement.
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add unit test for Site Settings Handler. BUG=543635 ========== to ========== Add unit test for Site Settings Handler. BUG=543635 Committed: https://crrev.com/f5ca600dbd28d6c3c8e528e35b0fb84efdcd309e Cr-Commit-Position: refs/heads/master@{#385296} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f5ca600dbd28d6c3c8e528e35b0fb84efdcd309e Cr-Commit-Position: refs/heads/master@{#385296}
Message was sent while issue was closed.
https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc (right): https://codereview.chromium.org/1845253002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/site_settings_handler_unittest.cc:118: EXPECT_EQ(2U, web_ui()->call_data().size()); On 2016/04/05 21:24:37, Finnur wrote: > This validates that we've had 2 calls from JS into web_ui (first a call to get a > default value and now another, to set the default value). If SetDefault were to > result in a callback call back into to js (which it doesn't) then I would have > created a helper that runs line 118 and then also validates the result of the > callback. But since there is no callback, it suffices to check the number of > calls into web_ui. > > To clarify: The term callback, that I use, is referring only to web_ui > communicating back to js with what the result of the web_ui call was. > > Another way to think about it: If GetDefault on line 109 did not return a > callback then I'd replace the ValidateDefault function on line 110 with: > > EXPECT_EQ(1U, web_ui()->call_data().size()); > > > Does this answer your question? Happy to do a follow-up if you think something > needs clarification/improvement. So, you're verifying that Get calls WebUI (via the cr.webUIResponse callback) and that the callback itself is invoked correctly (the arguments to ResolveJavascriptCallback which get used in cr.webUIResponse). You're also verifying that Set results in a WebUI call (from the content observer), but you aren't verifying the arguments of that call (the webUIListenerCallback for contentSettingCategoryChanged). Is my understanding correct? If so, I wasn't misunderstanding what was happening, just don't know why you didn't test the listener callback when you did test the response callback. No need to update the test though, it's fine. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
