Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
257803004:
Add a test for toggling logging in PasswordAutofillAgent (Closed)
|
Created:
6 years, 8 months ago by vabr (Chromium) Modified:
6 years, 7 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a test for toggling logging in PasswordAutofillAgent
Test for functionality added in https://codereview.chromium.org/231283003/.
This is done as a browser test. Previous attempt to make it a unit test was scratched in https://codereview.chromium.org/258473005/.
R=isherman@chromium.org
BUG=347927
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266829
Patch Set 1 #
Total comments: 4
Patch Set 2 : Testing API rather than implementation #
Total comments: 9
Patch Set 3 : Comments addressed #Messages
Total messages: 20 (0 generated)
Hi Ilya, Please take a look, this replaces https://codereview.chromium.org/258473005/. NB: I did not run trybots just yet, as this depends on https://codereview.chromium.org/231283003/, which in turn depends on https://codereview.chromium.org/254573005/. Thanks, Vaclav
https://codereview.chromium.org/257803004/diff/1/components/autofill/content/... File components/autofill/content/renderer/test_password_autofill_agent.h (right): https://codereview.chromium.org/257803004/diff/1/components/autofill/content/... components/autofill/content/renderer/test_password_autofill_agent.h:17: using PasswordAutofillAgent::logging_state_active; just curious, what is this? I never saw we doing this before.
https://codereview.chromium.org/257803004/diff/1/components/autofill/content/... File components/autofill/content/renderer/test_password_autofill_agent.h (right): https://codereview.chromium.org/257803004/diff/1/components/autofill/content/... components/autofill/content/renderer/test_password_autofill_agent.h:17: using PasswordAutofillAgent::logging_state_active; On 2014/04/25 16:04:56, tfarina wrote: > just curious, what is this? I never saw we doing this before. That means that logging_state_active from PasswordAutofillAgent appears in the TestPasswordAutofillAgent "namespace". The reason is to change its visibility from protected (in PasswordAutofillAgent) to public here. Equivalent but longer notation would be to override logging_state_active and make it call the PasswordAutofillAgent version directly. See Effective C++, 3rd edition, Item 33, by Scott Meyers. (In my case it was: see the above, forget about it, and then have Ilya re-teach me that. :))
https://codereview.chromium.org/257803004/diff/1/chrome/renderer/autofill/pas... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/257803004/diff/1/chrome/renderer/autofill/pas... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1068: } Hmm, could you instead test that RPCs are or aren't sent, depending on the logging status? Conceivably, the code could correctly set |logging_state_active_|, but then incorrectly read from it. It's generally most useful to test externally observable behavior, rather than internal class details.
Thanks, Ilya. PTAL. Cheers, Vaclav https://codereview.chromium.org/257803004/diff/1/chrome/renderer/autofill/pas... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/257803004/diff/1/chrome/renderer/autofill/pas... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1068: } On 2014/04/25 18:25:29, Ilya Sherman wrote: > Hmm, could you instead test that RPCs are or aren't sent, depending on the > logging status? Conceivably, the code could correctly set > |logging_state_active_|, but then incorrectly read from it. It's generally most > useful to test externally observable behavior, rather than internal class > details. Absolutely, that's a very good point. Done. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:183: void SendVisiblePasswordForms() { Here I'm piggybacking on PasswordAutofillAgentTest being a friend of PasswordAutofillAgent to call the private SendPasswordForms. Alternatively I could also make SendPasswordForms protected in PasswordAutofillAgent, elevate to public in TestPasswordAutofillAgent, and call directly from the test cases. The former approach is less code and looks more in line with the extisting practice in this test, but I'm happy to switch if required.
LGTM, thanks. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:183: void SendVisiblePasswordForms() { On 2014/04/28 14:31:10, vabr (Chromium) wrote: > Here I'm piggybacking on PasswordAutofillAgentTest being a friend of > PasswordAutofillAgent to call the private SendPasswordForms. > > Alternatively I could also make SendPasswordForms protected in > PasswordAutofillAgent, elevate to public in TestPasswordAutofillAgent, and call > directly from the test cases. > > The former approach is less code and looks more in line with the extisting > practice in this test, but I'm happy to switch if required. It would be nice to someday get rid of the friend declarations, but not necessary to do as part of this CL. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:185: /*only_visible=*/true); Optional nit: I'd write this as "true /* only_visible */". https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1056: TEST_F(PasswordAutofillAgentTest, OnChangeLoggingStateNoMessage) { Optional nit: I'd name this "OnChangeLoggingState_NoMessage", so that it's easier to identify the subset of tests that are most closely related to one another. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1067: IPC::Listener* const agent_listener(password_autofill_); nit: Please use a static_cast for casting.
The CQ bit was checked by vabr@chromium.org
The CQ bit was unchecked by vabr@chromium.org
Thanks, Ilya! Comments addressed, sending to the CQ. Cheers, Vaclav https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:183: void SendVisiblePasswordForms() { On 2014/04/28 21:13:41, Ilya Sherman wrote: > On 2014/04/28 14:31:10, vabr (Chromium) wrote: > > Here I'm piggybacking on PasswordAutofillAgentTest being a friend of > > PasswordAutofillAgent to call the private SendPasswordForms. > > > > Alternatively I could also make SendPasswordForms protected in > > PasswordAutofillAgent, elevate to public in TestPasswordAutofillAgent, and > call > > directly from the test cases. > > > > The former approach is less code and looks more in line with the extisting > > practice in this test, but I'm happy to switch if required. > > It would be nice to someday get rid of the friend declarations, but not > necessary to do as part of this CL. I agree with both -- getting rid of the friend declarations, and also not doing it in this CL. :) I'll keep it in mind (http://crbug.com/368160) as a good filler, or onboarding, task. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:185: /*only_visible=*/true); On 2014/04/28 21:13:41, Ilya Sherman wrote: > Optional nit: I'd write this as "true /* only_visible */". Done. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1056: TEST_F(PasswordAutofillAgentTest, OnChangeLoggingStateNoMessage) { On 2014/04/28 21:13:41, Ilya Sherman wrote: > Optional nit: I'd name this "OnChangeLoggingState_NoMessage", so that it's > easier to identify the subset of tests that are most closely related to one > another. Done. https://codereview.chromium.org/257803004/diff/10001/chrome/renderer/autofill... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1067: IPC::Listener* const agent_listener(password_autofill_); On 2014/04/28 21:13:41, Ilya Sherman wrote: > nit: Please use a static_cast for casting. Done.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/257803004/30001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/257803004/30001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/257803004/30001
Message was sent while issue was closed.
Change committed as 266829 |
