Allow an entire InProcessBrowserTest subclass to configure the host resolver ...

Add SetUpTestFixtureTest/TearDownTestFixtureTest methods to InProcessBrowserTest that let tests do 
what is normally done by overriding SetUp/TearDown methods.  This special method is needed because
of the way SetUp winds up at the bottom of the stack that invokes your test fixture methods.

Make the LiveBookmarksSyncTest class allow direct lookup for google.com domain as part of setup,
which these tests need to access GAIA and sync servers.

[Paweł Hajdan Jr., 2009-09-22 00:20:15]

NACK. I don't like making the method virtual, and overriding it.

Please override the configuration with AllowDirectLookup on top of it. It will
redirect to the system resolver. And it's more healthy long-term for the test
infrastructure.

If that still isn't as close to real-world test case as possible, I can think
more how to do that...

[tim (not reviewing), 2009-09-22 00:22:08]

On 2009/09/22 00:20:15, Paweł Hajdan Jr. wrote:
> NACK. I don't like making the method virtual, and overriding it.
> 
> Please override the configuration with AllowDirectLookup on top of it. It will
> redirect to the system resolver. And it's more healthy long-term for the test
> infrastructure.
> 
> If that still isn't as close to real-world test case as possible, I can think
> more how to do that...

Could you explain what you mean?  I don't quite follow what I'm supposed to do,
sorry :(

[tim (not reviewing), 2009-09-22 00:44:57]

On 2009/09/22 00:22:08, timsteele wrote:
> On 2009/09/22 00:20:15, Paweł Hajdan Jr. wrote:
> > NACK. I don't like making the method virtual, and overriding it.
> > 
> > Please override the configuration with AllowDirectLookup on top of it. It
will
> > redirect to the system resolver. And it's more healthy long-term for the
test
> > infrastructure.
> > 
> > If that still isn't as close to real-world test case as possible, I can
think
> > more how to do that...
> 
> Could you explain what you mean?  I don't quite follow what I'm supposed to
do,
> sorry :(

Basically any LiveBookmarkSyncTest needs to access external URLs, so it seems
intuitive to have something in LiveBookmarkSyncTest (the parent class of all
such tests) configure this.  It can't be done by overriding SetUp, because the
HostResolver is only instantiated in InProcessBrowserTest::SetUp, so it is null
when overriding SetUp.  And calling AddSimulatedFailure for domain A (which
InProcessBrowserTest::SetUp does now) will trump any future rule additions for
domain A, such as AllowDirectLookup. So AddSimulatedFailure(*.google.com) can't
happen for LiveBookmarkSyncTests.

[tim (not reviewing), 2009-09-22 20:59:11]

I spoke with Pawel over IM and re-implemented with proposed changes.  Please
have another look.

[chron_chromium.org, 2009-09-22 23:25:06]

lgtm

http://codereview.chromium.org/214047/diff/7001/7005
File chrome/test/live_sync/live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/214047/diff/7001/7005#newcode87
Line 87: new net::RuleBasedHostResolverProc(host_resolver());
+2 indent

http://codereview.chromium.org/214047/diff/7001/7005#newcode88
Line 88: h->AllowDirectLookup("*.google.com");
s/h/resolver

[Paweł Hajdan Jr., 2009-09-22 23:26:22]

+jcampan for in_process_* stuff

http://codereview.chromium.org/214047/diff/7001/7002
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/214047/diff/7001/7002#newcode75
Line 75: virtual void SetUpInProcessBrowserTestFixture() {}
nit: please add an empty line after that

http://codereview.chromium.org/214047/diff/7001/7005
File chrome/test/live_sync/live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/214047/diff/7001/7005#newcode86
Line 86: net::RuleBasedHostResolverProc* h =
This should be a scoped_refptr member of the fixture, otherwise it's going to be
leaked.

[tim (not reviewing), 2009-09-22 23:37:16]

http://codereview.chromium.org/214047/diff/7001/7002
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/214047/diff/7001/7002#newcode75
Line 75: virtual void SetUpInProcessBrowserTestFixture() {}
On 2009/09/22 23:26:22, Paweł Hajdan Jr. wrote:
> nit: please add an empty line after that

Done.

http://codereview.chromium.org/214047/diff/7001/7005
File chrome/test/live_sync/live_bookmarks_sync_test.cc (right):

http://codereview.chromium.org/214047/diff/7001/7005#newcode86
Line 86: net::RuleBasedHostResolverProc* h =
On 2009/09/22 23:26:22, Paweł Hajdan Jr. wrote:
> This should be a scoped_refptr member of the fixture, otherwise it's going to
be
> leaked.

It's not leaked because I don't AddRef, and ScopedDefaultHostResolverProc will
stuff |resolver| into a scoped_refptr.  Added comment for clarity.

http://codereview.chromium.org/214047/diff/7001/7005#newcode87
Line 87: new net::RuleBasedHostResolverProc(host_resolver());
On 2009/09/22 23:25:06, chron_chromium.org wrote:
> +2 indent

Done.

http://codereview.chromium.org/214047/diff/7001/7005#newcode88
Line 88: h->AllowDirectLookup("*.google.com");
On 2009/09/22 23:25:06, chron_chromium.org wrote:
> s/h/resolver

Done.

[Paweł Hajdan Jr., 2009-09-22 23:39:41]

LGTM

[jcampan, 2009-09-23 16:43:47]

LGTM (in_process_test part)