[Password Manager] Make expected the calls of FormSaver in password_form_manager_unittests.cc

[Password Manager] Make expected the calls of FormSaver in password_form_manager_unittests.cc

Otherwise, unittests will show warnings 'unexpected calls'.

BUG=621355
Committed: https://crrev.com/d0297390345d61d57a39736f898362888a9aaf30
Cr-Commit-Position: refs/heads/master@{#406839}

[kolos1, 2016-07-21 11:31:25]

kolos@chromium.org changed reviewers:
+ vabr@chromium.org

[kolos1, 2016-07-21 11:31:26]

Hi Vaclav,

I fixed 'unexpected calls' of FormSaver in password_form_manager_unittests.cc.
Please review.

Regards,
Maxim

[vabr (Chromium), 2016-07-21 11:57:50]

Hi Maxim,

Sorry for the log spam.

However, adding EXPECT_CALLS is not the right solution here. The tests do not
actually care whether the mentioned methods are called in the situations you
modified.

The right solution here is to create a NiceMock<MockFormSaver> instead of just
MockFormSaver in the places where that MockFormSaver would generate the log
spam. See more at, e.g.,
https://groups.google.com/forum/#!topic/googlemock/pRyZwyWmrRE (Zhanyong Wan is
the author of gMock).

Also, when you set up EXPECT_CALL with Times(1), you can as well leave that
Times(1) out, because it is the default behaviour. But that no longer applies
here if you follow my advice from above.

Cheers,
Vaclav

[kolos1, 2016-07-21 13:03:27]

Patchset #2 (id:20001) has been deleted

[kolos1, 2016-07-21 13:08:42]

On 2016/07/21 11:57:50, vabr (Chromium) wrote:
> Hi Maxim,
> 
> Sorry for the log spam.
> 
> However, adding EXPECT_CALLS is not the right solution here. The tests do not
> actually care whether the mentioned methods are called in the situations you
> modified.
> 
> The right solution here is to create a NiceMock<MockFormSaver> instead of just
> MockFormSaver in the places where that MockFormSaver would generate the log
> spam. See more at, e.g.,
> https://groups.google.com/forum/#!topic/googlemock/pRyZwyWmrRE (Zhanyong Wan
is
> the author of gMock).
> 
> Also, when you set up EXPECT_CALL with Times(1), you can as well leave that
> Times(1) out, because it is the default behaviour. But that no longer applies
> here if you follow my advice from above.
> 
> Cheers,
> Vaclav

Thanks, Vaclav. It is good to know. Changed to NiceMock. See my comment.

Regards,
Maxim

[kolos1, 2016-07-21 13:08:50]

https://codereview.chromium.org/2167003003/diff/40001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2167003003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:419:
observed_form_, base::WrapUnique(new NiceMock<MockFormSaver>())));
Is it ok if the default behavior for FormSaver is to ignore unexpected calls?
FYI, the change in this line produces spam just in one test. Other tests create
MockFormSaver withing a test.

[vabr (Chromium), 2016-07-21 13:26:15]

LGTM, thanks!
Vaclav

https://codereview.chromium.org/2167003003/diff/40001/components/password_man...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/2167003003/diff/40001/components/password_man...
components/password_manager/core/browser/password_form_manager_unittest.cc:419:
observed_form_, base::WrapUnique(new NiceMock<MockFormSaver>())));
On 2016/07/21 13:08:50, kolos1 wrote:
> Is it ok if the default behavior for FormSaver is to ignore unexpected calls?
> FYI, the change in this line produces spam just in one test. Other tests
create
> MockFormSaver withing a test.

Thanks for checking. It is OK. MockFormSaver has a very low number of mocked
methods, so if any test is interested in checking that a method is not called,
adding EXPECT_CALL().Times(0) is a much better way to express that then relying
on gMock warnings (or changing the mock to a StrictMock to get real failures).

[kolos1, 2016-07-21 13:29:42]

The CQ bit was checked by kolos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 13:29:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 14:06:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:06:16]

Dry run: This issue passed the CQ dry run.

[kolos1, 2016-07-21 14:06:54]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-07-21 14:07:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 14:10:38]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-21 14:12:13]

Description was changed from

==========
[Password Manager] Make expected the calls of FormSaver in
password_form_manager_unittests.cc

Otherwise, unittests will show warnings 'unexpected calls'.

BUG=621355
==========

to

==========
[Password Manager] Make expected the calls of FormSaver in
password_form_manager_unittests.cc

Otherwise, unittests will show warnings 'unexpected calls'.

BUG=621355

Committed: https://crrev.com/d0297390345d61d57a39736f898362888a9aaf30
Cr-Commit-Position: refs/heads/master@{#406839}
==========

[commit-bot: I haz the power, 2016-07-21 14:12:14]

Patchset 2 (id:??) landed as
https://crrev.com/d0297390345d61d57a39736f898362888a9aaf30
Cr-Commit-Position: refs/heads/master@{#406839}