Stop using TemplateURLServiceTestUtil to initialize TemplateURLServiceFactory

Stop using TemplateURLServiceTestUtil to initialize TemplateURLServiceFactory

To drop all dependencies on chrome, TemplateURLServiceTestUtil is going to stop interact with TemplateURLServiceFactory.

All tests which have used the util to initialize the factory are rewritten to do it with TemplateURLServiceFactoryTestUtil.

BUG=387985
TEST=unit_tests
TBR=sky@chromium.org for an include removal in ui_test_utils.cc

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

[Peter Kasting, 2014-07-10 23:15:48]

I don't like the way this adds noticeably more boilerplate code in multiple
places, especially since that code is somewhat nontrivial.

Why does this test util file have to be componentized?  Can't we leave it in
chrome/ somewhere, or at least split it into two pieces based on which parts of
it depend on chrome/?  Then the chrome/ side will still be available for tests
in chrome/ to use (which seems to be "all of them" right now).

[hashimoto, 2014-07-11 16:36:14]

On 2014/07/10 23:15:48, Peter Kasting wrote:
> I don't like the way this adds noticeably more boilerplate code in multiple
> places, especially since that code is somewhat nontrivial.
> 
> Why does this test util file have to be componentized?  Can't we leave it in
This test util should be componentized to move TemplateURLServiceTest to
components/search_engines.
> chrome/ somewhere, or at least split it into two pieces based on which parts
of
> it depend on chrome/?  Then the chrome/ side will still be available for tests
> in chrome/ to use (which seems to be "all of them" right now).
Currently, TemplateURLServiceTestUtil is doing the following things:
 1. Initialize TemplateURLServiceFactory to return non-NULL TemplateURLService
for classes which depend on TemplateURLServiceFactory.
 2. Initialize TestingTemplateURLService and counts
OnTemplateURLServiceChanged() invocations.
What all users of TemplateURLServiceTestUtil outside of
chrome/browser/search_engines need is #1 and they don't care about #2 at all.
To move tests under chrome/browser/search_engines to components,
TemplateURLServiceTestUtil should be chrome-free to provide the feature #2 for
TemplateURLServiceTest.

In the new patch set, a new class TemplateURLServiceFactoryTestUtil is added to
replace TemplateURLServiceTestUtil when the test needs only #1.
Also, TemplateURLServiceTestUtil is fixed to stop using
TemplateURLServiceFactory.
In the coming changes, all chrome/ dependencies will be removed from
TemplateURLServiceTestUtil.

[Peter Kasting, 2014-07-11 17:55:54]

More questions.

(1) Is it possible to do what's currently in
TemplateURLService[Factory]TestUtil::SetUp() and TearDown() in its constructor
and destructor instead?  That would ensure that no one forgets to call these.
(2) It looks like most things in TemplateURLServiceFactoryTestUtil are also in
TemplateURLServiceTestUtil.  What if we made TemplateURLServiceFactoryTestUtil a
subclass of TemplateURLServiceTestUtil, which additionally mucked with
TemplateURLServiceFactory?  If this doesn't make sense, would it make sense to
have a common base class for both of these classes?

[hashimoto, 2014-07-14 09:27:48]

On 2014/07/11 17:55:54, Peter Kasting wrote:
> More questions.
> 
> (1) Is it possible to do what's currently in
> TemplateURLService[Factory]TestUtil::SetUp() and TearDown() in its constructor
> and destructor instead?  That would ensure that no one forgets to call these.
Done.
> (2) It looks like most things in TemplateURLServiceFactoryTestUtil are also in
> TemplateURLServiceTestUtil.  What if we made TemplateURLServiceFactoryTestUtil
a
> subclass of TemplateURLServiceTestUtil, which additionally mucked with
> TemplateURLServiceFactory?  If this doesn't make sense, would it make sense to
> have a common base class for both of these classes?

Code duplicated between TemplateURLServiceFactoryTestUtil and
TemplateURLServiceTestUtil is:
 1. TemplateURLServiceObserver implementation to count changes
  Removed. As long as Load() is tested well in TemplateURLService's own test,
there is no need to verify the observer related behavior in tests of user code.
 2. SetManagedDefaultSearchPreferences/RemoveManagedDefaultSearchPreferences
  Removed. These methods were doing almost nothing, just inlining them with the
new function TemplateURLServiceTestUtil::CreateDefaultSearchPreferenceValue()
should be fine.
  Also, since TemplateURLServiceTestUtil is going to replace chrome/test/base's
TestingPrefServiceSyncable with user_prefs::TestingPrefServiceSyncable and these
two pref classes inherit different template instances of TestingPrefServiceBase,
sub-classing doesn't work here.

Now TemplateURLSerivceFactoryTestUtil is 32+33=65 lines and it shares almost no
code with TemplateURLServiceTestUtil.
BTW, how about inlining all code in TemplateURLServiceFactoryTestUtil as the
patch set #1 did?
I'm not sure if it makes sense to have a class to just call
TestingProfile::CreateWebDataService() and
TemplateURLServiceFactory::SetTestingFactoryAndUse() in 3 unittests.

[Peter Kasting, 2014-07-14 21:32:53]

On 2014/07/14 09:27:48, hashimoto wrote:
>  2. SetManagedDefaultSearchPreferences/RemoveManagedDefaultSearchPreferences
>   Removed. These methods were doing almost nothing, just inlining them with
the
> new function TemplateURLServiceTestUtil::CreateDefaultSearchPreferenceValue()
> should be fine.

No, this is one of the things I didn't like about patch set 1.  The inlined code
is noticeably longer and less readable.  I don't want to land this.

>   Also, since TemplateURLServiceTestUtil is going to replace
chrome/test/base's
> TestingPrefServiceSyncable with user_prefs::TestingPrefServiceSyncable and
these
> two pref classes inherit different template instances of
TestingPrefServiceBase,
> sub-classing doesn't work here.

Why are the two incompatible?  If both are named TestingPrefServiceSyncable, it
seems like a bug if they're actually two completely different, non-coercible
types.  Maybe that should be fixed?  If not, maybe we can use
user_prefs::TestingPrefServiceSyncable in both cases?  Why do these actually
have to be different?

In the limit we can come up with some kind of template-based solution, but
that's ugly.

[hashimoto, 2014-07-15 06:24:21]

On 2014/07/14 21:32:53, Peter Kasting wrote:
> On 2014/07/14 09:27:48, hashimoto wrote:
> >  2. SetManagedDefaultSearchPreferences/RemoveManagedDefaultSearchPreferences
> >   Removed. These methods were doing almost nothing, just inlining them with
> the
> > new function
TemplateURLServiceTestUtil::CreateDefaultSearchPreferenceValue()
> > should be fine.
> 
> No, this is one of the things I didn't like about patch set 1.  The inlined
code
> is noticeably longer and less readable.  I don't want to land this.
> 
> >   Also, since TemplateURLServiceTestUtil is going to replace
> chrome/test/base's
> > TestingPrefServiceSyncable with user_prefs::TestingPrefServiceSyncable and
> these
> > two pref classes inherit different template instances of
> TestingPrefServiceBase,
> > sub-classing doesn't work here.
> 
> Why are the two incompatible?  If both are named TestingPrefServiceSyncable,
it
> seems like a bug if they're actually two completely different, non-coercible
> types.  Maybe that should be fixed?  If not, maybe we can use
chrome/test/base's TestingPrefServiceSyncable is a TestingPrefService which
inherits PrefServiceSyncable defined in
chrome/browser/prefs/pref_service_syncable.h
user_prefs::TestingPrefServiceSyncable is a TestingPrefService with
PrefRegistrySyncable which inherits PrefService defined in
base/prefs/pref_service.h
If there is a better name for one of them, we can do rename to make it less
ambiguous.
> user_prefs::TestingPrefServiceSyncable in both cases?  Why do these actually
> have to be different?
When TestingProfile is used, ::TestingPrefServiceSyncable is the only choice
because TestingProfile depends on PrefServiceSynable's Chrome specific features.
For code being componentized, user_prefs::TestingPrefServiceSyncable should be
used because it shouldn't include Chrome specific parts.
> 
> In the limit we can come up with some kind of template-based solution, but
> that's ugly.
Added a new utility DefaultSearchPrefTestUtil under components/search_engines.
Both TemplateURLServiceTestUtil and TemplateURLServiceFactoryTestUtil have
wrapper methods to make user code a bit shorter.

[Peter Kasting, 2014-07-15 18:48:59]

LGTM

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_test_util.h (right):

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_test_util.h:29: void SetUp();
Can these also be moved into the constructor/destructor?  Or do they have to
remain as separate SetUp()/TearDown() functions?

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_test_util.h:31: // Cleans up
data structures for this class  (mirroring gtest standard
Nit: Extra space

[hashimoto, 2014-07-16 03:20:59]

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_test_util.h (right):

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_test_util.h:29: void SetUp();
On 2014/07/15 18:48:58, Peter Kasting wrote:
> Can these also be moved into the constructor/destructor?  Or do they have to
> remain as separate SetUp()/TearDown() functions?

ASSERT_TRUE cannot be moved into the ctor so at least SetUp has a reason to
exist.

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_test_util.h:31: // Cleans up
data structures for this class  (mirroring gtest standard
On 2014/07/15 18:48:58, Peter Kasting wrote:
> Nit: Extra space

Done.

[hashimoto, 2014-07-16 05:37:19]

battre@, could you review automatic_profile_resetter_delegate_unittest.cc as an
owner?
haitaol@, could you review search_engine_data_type_controller_unittest.cc as an
owner?

TestURLServiceFactoryTestUtil is replacing TestURLServiceTestUtil.

[Peter Kasting, 2014-07-16 05:38:12]

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_test_util.h (right):

https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_test_util.h:29: void SetUp();
On 2014/07/16 03:20:59, hashimoto wrote:
> On 2014/07/15 18:48:58, Peter Kasting wrote:
> > Can these also be moved into the constructor/destructor?  Or do they have to
> > remain as separate SetUp()/TearDown() functions?
> 
> ASSERT_TRUE cannot be moved into the ctor so at least SetUp has a reason to
> exist.

Shrug, you could just nuke the ASSERT_TRUE.

[hashimoto, 2014-07-16 05:56:02]

On 2014/07/16 05:38:12, Peter Kasting wrote:
>
https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
> File chrome/browser/search_engines/template_url_service_test_util.h (right):
> 
>
https://codereview.chromium.org/376413002/diff/220001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_test_util.h:29: void
SetUp();
> On 2014/07/16 03:20:59, hashimoto wrote:
> > On 2014/07/15 18:48:58, Peter Kasting wrote:
> > > Can these also be moved into the constructor/destructor?  Or do they have
to
> > > remain as separate SetUp()/TearDown() functions?
> > 
> > ASSERT_TRUE cannot be moved into the ctor so at least SetUp has a reason to
> > exist.
> 
> Shrug, you could just nuke the ASSERT_TRUE.

OK, removed SetUp() and TearDown().

[battre, 2014-07-16 07:13:51]

Delegating to engedy@ who wrote the code.

[engedy, 2014-07-16 08:21:10]

LGTM on c/b/profile_resetter/, with one tiny nit.

https://codereview.chromium.org/376413002/diff/300001/chrome/browser/profile_...
File
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc
(right):

https://codereview.chromium.org/376413002/diff/300001/chrome/browser/profile_...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:239:
scoped_ptr<TemplateURLServiceFactoryTestUtil> util_;
nit: Could you please rename this to be a bit more specific, e.g.,
|template_url_service_test_util|?

[hashimoto, 2014-07-16 09:04:41]

https://codereview.chromium.org/376413002/diff/300001/chrome/browser/profile_...
File
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc
(right):

https://codereview.chromium.org/376413002/diff/300001/chrome/browser/profile_...
chrome/browser/profile_resetter/automatic_profile_resetter_delegate_unittest.cc:239:
scoped_ptr<TemplateURLServiceFactoryTestUtil> util_;
On 2014/07/16 08:21:09, engedy wrote:
> nit: Could you please rename this to be a bit more specific, e.g.,
> |template_url_service_test_util|?

Done.

[haitaol1, 2014-07-16 16:46:40]

lgtm

[hashimoto, 2014-07-17 03:44:42]

TBRing sky@chromium.org for an include removal in ui_test_utils.cc

[hashimoto, 2014-07-17 03:44:49]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-17 03:51:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/376413002/350001

[commit-bot: I haz the power, 2014-07-17 07:39:09]

Change committed as 283695