Remove MockFormFetcherImpl from FormFetcherImplTest

Remove MockFormFetcherImpl from FormFetcherImplTest

MockFormFetcherImpl currently appears in FormFetcherImpl unittest. Its purpose
is to ensure that ProcessMigratedForms is called on a FormFetcher if an
associated HttpPasswordMigrator receives results from PasswordStore.

This is not necessary. It is sufficient to check that (A) HttpPasswordMigrator
receiving results from PasswordStore calls ProcessMigratedForms on its
consumer, and (B), that forms which the FormFetcher ends up with are the
migrated ones. (A) is tested in HttpPasswordMigratorTest::TestFullStore, (B) is
tested in FormFetcherImplTest.TryToMigrateHTTPPasswordsOnHTTPSSites through
EXPECT_CALL(consumer_, ProcessMatches(...https_form...)).

Moreover, having a test double for the tested class is a bad practice, which
could result in the test bypassing the production code when it should not.

Therefore, this CL removes MockFormFetcherImpl.

BUG=571580
Review-Url: https://codereview.chromium.org/2756543004
Cr-Commit-Position: refs/heads/master@{#457415}
Committed: https://chromium.googlesource.com/chromium/src/+/ff26a148d43106a3154d412c7f57902fb5a33fde

[vabr (Chromium), 2017-03-16 11:37:31]

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

[commit-bot: I haz the power, 2017-03-16 11:37:39]

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

[vabr (Chromium), 2017-03-16 11:55:57]

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

[vabr (Chromium), 2017-03-16 11:55:57]

Hi Jan,

Could you PTAL?

Thanks,
Vaclav

[commit-bot: I haz the power, 2017-03-16 12:15:07]

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

[commit-bot: I haz the power, 2017-03-16 12:15:08]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-03-16 12:20:13]

lgtm

[vabr (Chromium), 2017-03-16 13:15:40]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-03-16 13:15:58]

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, 2017-03-16 13:19:41]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489670140299830, "parent_rev":
"6986d62692bad0e2d790d61ea4e2ac201dc7efd5", "commit_rev":
"ff26a148d43106a3154d412c7f57902fb5a33fde"}

[commit-bot: I haz the power, 2017-03-16 13:20:16]

Description was changed from

==========
Remove MockFormFetcherImpl from FormFetcherImplTest

MockFormFetcherImpl currently appears in FormFetcherImpl unittest. Its purpose
is to ensure that ProcessMigratedForms is called on a FormFetcher if an
associated HttpPasswordMigrator receives results from PasswordStore.

This is not necessary. It is sufficient to check that (A) HttpPasswordMigrator
receiving results from PasswordStore calls ProcessMigratedForms on its
consumer, and (B), that forms which the FormFetcher ends up with are the
migrated ones. (A) is tested in HttpPasswordMigratorTest::TestFullStore, (B) is
tested in FormFetcherImplTest.TryToMigrateHTTPPasswordsOnHTTPSSites through
EXPECT_CALL(consumer_, ProcessMatches(...https_form...)).

Moreover, having a test double for the tested class is a bad practice, which
could result in the test bypassing the production code when it should not.

Therefore, this CL removes MockFormFetcherImpl.

BUG=571580
==========

to

==========
Remove MockFormFetcherImpl from FormFetcherImplTest

MockFormFetcherImpl currently appears in FormFetcherImpl unittest. Its purpose
is to ensure that ProcessMigratedForms is called on a FormFetcher if an
associated HttpPasswordMigrator receives results from PasswordStore.

This is not necessary. It is sufficient to check that (A) HttpPasswordMigrator
receiving results from PasswordStore calls ProcessMigratedForms on its
consumer, and (B), that forms which the FormFetcher ends up with are the
migrated ones. (A) is tested in HttpPasswordMigratorTest::TestFullStore, (B) is
tested in FormFetcherImplTest.TryToMigrateHTTPPasswordsOnHTTPSSites through
EXPECT_CALL(consumer_, ProcessMatches(...https_form...)).

Moreover, having a test double for the tested class is a bad practice, which
could result in the test bypassing the production code when it should not.

Therefore, this CL removes MockFormFetcherImpl.

BUG=571580

Review-Url: https://codereview.chromium.org/2756543004
Cr-Commit-Position: refs/heads/master@{#457415}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff26a148d43106a3154d412c7f57...
==========

[commit-bot: I haz the power, 2017-03-16 13:20:18]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/ff26a148d43106a3154d412c7f57...