autofill: Add tests for the account chooser.

autofill: Add tests for the account chooser.

BUG=180419
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186991

[Ilya Sherman, 2013-03-06 00:43:01]

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc (right):

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
nit: 2013, no (c)

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:21:
MOCK_METHOD0(AccountChoiceChanged, void());
I thought we weren't supposed to use Gmock in ui/ test code?

[Evan Stade, 2013-03-07 04:06:09]

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc (right):

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2013/03/06 00:43:01, Ilya Sherman wrote:
> nit: 2013, no (c)

Done.

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:21:
MOCK_METHOD0(AccountChoiceChanged, void());
On 2013/03/06 00:43:01, Ilya Sherman wrote:
> I thought we weren't supposed to use Gmock in ui/ test code?

I spent a bit of time tracking down an answer to this. In the end, no one told
me there was an absolute moratorium. Ben deferred to jam, and I got jam's
sign-off on this usage because it is a pure virtual interface and the usage is
very straightforward (easy to understand or refactor if necessary).

[Ilya Sherman, 2013-03-07 04:57:15]

LGTM.

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc (right):

https://codereview.chromium.org/12502004/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:21:
MOCK_METHOD0(AccountChoiceChanged, void());
On 2013/03/07 04:06:09, Evan Stade wrote:
> On 2013/03/06 00:43:01, Ilya Sherman wrote:
> > I thought we weren't supposed to use Gmock in ui/ test code?
> 
> I spent a bit of time tracking down an answer to this. In the end, no one told
> me there was an absolute moratorium. Ben deferred to jam, and I got jam's
> sign-off on this usage because it is a pure virtual interface and the usage is
> very straightforward (easy to understand or refactor if necessary).

Awesome!  I agree, I find Gmock to be the clearest way to write tests like
these.  Thanks for chasing that down.

[Dan Beam, 2013-03-07 05:18:31]

lgtm (though an integration test that verifies AutofillDialogControllerImpl
actually calls AccountChooserModel::SetHadWalletError(true) would be nice)

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc (right):

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:41:
TEST_F(AccountChooserModelTest, ObeysPref) {
nit: I'd arguably alphabetize these tests by name in this file, but I don't know
if there's a precedent for this.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:46:
EXPECT_TRUE(model()->WalletIsSelected());
\n

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:57:
EXPECT_TRUE(model()->WalletIsSelected());
\n

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:66:
model()->ExecuteCommand(0);
// Comment explaining what this does.

I assume it selects a suggestions menu item. If it does, that's a really poor
name, IMO.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:67:
EXPECT_TRUE(model()->WalletIsSelected());
\n

[Evan Stade, 2013-03-07 05:39:15]

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc (right):

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:41:
TEST_F(AccountChooserModelTest, ObeysPref) {
On 2013/03/07 05:18:31, Dan Beam wrote:
> nit: I'd arguably alphabetize these tests by name in this file, but I don't
know
> if there's a precedent for this.

generally tests are ordered from simplest to more complex. Related tests are
near each other.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:46:
EXPECT_TRUE(model()->WalletIsSelected());
On 2013/03/07 05:18:31, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:57:
EXPECT_TRUE(model()->WalletIsSelected());
On 2013/03/07 05:18:31, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:66:
model()->ExecuteCommand(0);
On 2013/03/07 05:18:31, Dan Beam wrote:
> // Comment explaining what this does.
> 
> I assume it selects a suggestions menu item. If it does, that's a really poor
> name, IMO.

it's documented in ui/base/models/menu_model.h. I don't want to duplicate that
documentation here for reasons we've discussed before.

https://codereview.chromium.org/12502004/diff/18013/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_models_unittest.cc:67:
EXPECT_TRUE(model()->WalletIsSelected());
On 2013/03/07 05:18:31, Dan Beam wrote:
> \n

Done.

[commit-bot: I haz the power, 2013-03-07 05:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12502004/33001

[commit-bot: I haz the power, 2013-03-07 06:26:13]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-03-08 00:02:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12502004/48001

[commit-bot: I haz the power, 2013-03-08 04:47:29]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-03-08 17:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/12502004/48001

[commit-bot: I haz the power, 2013-03-08 18:16:52]

Change committed as 186991