[Cronet] Unit test refactoring and fixes

[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…() resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365
Committed: https://crrev.com/b0293a3aa2ce09755ed6fab5838b4f978d56f313
Cr-Commit-Position: refs/heads/master@{#363876}

[kapishnikov, 2015-12-07 19:41:21]

kapishnikov@chromium.org changed reviewers:
+ mef@chromium.org, pauljensen@chromium.org, xunjieli@chromium.org

[xunjieli, 2015-12-07 23:09:10]

This is great. Glad that you are working on making the tests better! A few
comments.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:37:
* <code>URL.setURLStreamHandlerFactory()</code> method can only be called once.
This flag
nit: maybe use {@code true} ? That's how it was used elsewhere, so we should
probably stick to one style.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:132:
} else {
I'd prefer to keep CronetHttpURLConnection logic all in one place here. It's
slightly cleaner and easier to reason about.  We can avoid having to call
switchToCronetHttpURLConnectionTestingIfNeeded, and set
mTestingCronetHttpURLConnection.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:151:
public static void prepareTestStorage(Context context) {
I think moving CronetTestFramework into CronetTestBase.java might be another
cleanup that might worth considering.

That way we can consolidate some of the setup/initialization logic, and some of
the member variables.

[kapishnikov, 2015-12-08 01:09:37]

Helen, thanks for the review. The comments are below.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:37:
* <code>URL.setURLStreamHandlerFactory()</code> method can only be called once.
This flag
On 2015/12/07 23:09:10, xunjieli wrote:
> nit: maybe use {@code true} ? That's how it was used elsewhere, so we should
> probably stick to one style.

Will fix it.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:132:
} else {
On 2015/12/07 23:09:10, xunjieli wrote:
> I'd prefer to keep CronetHttpURLConnection logic all in one place here. It's
> slightly cleaner and easier to reason about.  We can avoid having to call
> switchToCronetHttpURLConnectionTestingIfNeeded, and set
> mTestingCronetHttpURLConnection. 

The problem with keeping all logic inside runTest() method is that the method is
called before the actual test is run; therefore, CronetTestFramework must be
created in the setUp() method. Otherwise, mCronetTestFramework will be null and
URL.setURLStreamHandlerFactory(.mStreamHandlerFactory) will fail with
NullPointerException. Instantiating CronetEngin in setUp() imposes limitation
that all tests have to share the same Cronet configuration. Also, two engines
cannot be instantiated in the same test, e.g. for persistence testing.

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:151:
public static void prepareTestStorage(Context context) {
On 2015/12/07 23:09:10, xunjieli wrote:
> I think moving CronetTestFramework into CronetTestBase.java might be another
> cleanup that might worth considering.
> 
> That way we can consolidate some of the setup/initialization logic, and some
of
> the member variables. 

Yes, I agree that we could move some of the CronetTestFramework logic into the
CronetTestBase class and the remaining parts to some utility class/classes. Lets
do the refactoring in a new CL since this one already addresses couple of
issues.

[xunjieli, 2015-12-08 13:40:25]

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:132:
} else {
On 2015/12/08 01:09:37, kapishnikov wrote:
> On 2015/12/07 23:09:10, xunjieli wrote:
> > I'd prefer to keep CronetHttpURLConnection logic all in one place here. It's
> > slightly cleaner and easier to reason about.  We can avoid having to call
> > switchToCronetHttpURLConnectionTestingIfNeeded, and set
> > mTestingCronetHttpURLConnection. 
> 
> The problem with keeping all logic inside runTest() method is that the method
is
> called before the actual test is run; therefore, CronetTestFramework must be
> created in the setUp() method. Otherwise, mCronetTestFramework will be null
and
> URL.setURLStreamHandlerFactory(.mStreamHandlerFactory) will fail with
> NullPointerException. Instantiating CronetEngin in setUp() imposes limitation
> that all tests have to share the same Cronet configuration. Also, two engines
> cannot be instantiated in the same test, e.g. for persistence testing.

I don't really like this. The @CompareDefaultWithCronet or
@OnlyRunCronetHttpURLConnection will only ever need one CronetEngine instance
anyway. The tests don't share CronetEngines, since the default stack doesn't use
CronetEngine.

I think this CL tries to do too many things at once, which makes reviewing hard.
Could you do a change list that's only enough to solve the host resolver
refactoring first? And do other changes in small chunks in different CLs?

[kapishnikov, 2015-12-08 19:10:27]

Description was changed from

==========
[Cronet] Unit test refactoring and fixes

1. Bug fix: if a test annotated with @CompareDefaultWithCronet or
   @OnlyRunCronetHttpURLConnection tried  to instantiate CronetEngine
   inside the test instead of the setUp() method, it failed with
   NullPointerException. As the result of the change, the above
   mentioned tests do not need to share a single CronetEngine
   configuration but instead may create their own configuration inside
   the tests (multiple times if needed).
2. Bug fix: got rid of CronetTestBase.mCronetTestFramework member field.
   That field kept the reference to the last instantiated
   CronetTestFramework; thus, instantiating the framework twice,
   resulted in the loss of the previous state.
3. Bug fix: Tests that were annotated with CompareDefaultWithCronet
   didn’t call setUp() & tearDown() methods between the test runs. That
   potentially introduced dependency between two runs.
4. Refactoring: introduced registerHostResolver(...) method in the base
   class that allow registering the resolver for the legacy and new
   APIs. Refactored QuicTest, SdchTest and PkpTest to use it.
5. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
   resulted in deletion of QUIC cache. It is not desirable in some cases,
   e.g. in tests that check persistence. As the result of the change,
   the cache is always cleared on setUp() method. That eliminates
   dependencies between tests. If a test needs to clear the cache
   between multiple engine instantiation within a single test, it can
   call CronetTestFramework.prepareTestStorage() method.

BUG=565365
==========

to

==========
[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class
that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and
PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check
persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates
dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation
within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and
the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365
==========

[kapishnikov, 2015-12-08 20:05:33]

On 2015/12/08 13:40:25, xunjieli wrote:
>
https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
> File
>
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
> (right):
> 
>
https://codereview.chromium.org/1503943003/diff/1/components/cronet/android/t...
>
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:132:
> } else {
> On 2015/12/08 01:09:37, kapishnikov wrote:
> > On 2015/12/07 23:09:10, xunjieli wrote:
> > > I'd prefer to keep CronetHttpURLConnection logic all in one place here.
It's
> > > slightly cleaner and easier to reason about.  We can avoid having to call
> > > switchToCronetHttpURLConnectionTestingIfNeeded, and set
> > > mTestingCronetHttpURLConnection. 
> > 
> > The problem with keeping all logic inside runTest() method is that the
method
> is
> > called before the actual test is run; therefore, CronetTestFramework must be
> > created in the setUp() method. Otherwise, mCronetTestFramework will be null
> and
> > URL.setURLStreamHandlerFactory(.mStreamHandlerFactory) will fail with
> > NullPointerException. Instantiating CronetEngin in setUp() imposes
limitation
> > that all tests have to share the same Cronet configuration. Also, two
engines
> > cannot be instantiated in the same test, e.g. for persistence testing.
> 
> I don't really like this. The @CompareDefaultWithCronet or
> @OnlyRunCronetHttpURLConnection will only ever need one CronetEngine instance
> anyway. The tests don't share CronetEngines, since the default stack doesn't
use
> CronetEngine.
> 
> I think this CL tries to do too many things at once, which makes reviewing
hard.
> Could you do a change list that's only enough to solve the host resolver
> refactoring first? And do other changes in small chunks in different CLs?

I have remove everything non-related to the host resolver from the CL.

[pauljensen, 2015-12-08 20:07:13]

I took a quick glace at this now that it's been broken into two CLs and it's
looking good.  Helen can take a more thorough look tonight, or I'll do so
tomorrow morning.

[xunjieli, 2015-12-08 20:11:57]

Looks great. Thanks for doing this. I just have one comment.

https://codereview.chromium.org/1503943003/diff/20001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1503943003/diff/20001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:151:
public static void prepareTestStorage(Context context) {
Tests should not have access to this method. Maybe move this to CronetTestBase,
and make it private?

[xunjieli, 2015-12-08 20:47:29]

lgtm. thanks!

https://codereview.chromium.org/1503943003/diff/20001/components/cronet/andro...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/1503943003/diff/20001/components/cronet/andro...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:151:
public static void prepareTestStorage(Context context) {
On 2015/12/08 20:11:57, xunjieli wrote:
> Tests should not have access to this method. Maybe move this to
CronetTestBase,
> and make it private?

Talked to Andrei offline. I think we are gonna leave this as it is, since Andrei
convinced me that they should really be a part of a utility class instead of
CronetTestBase.

[kapishnikov, 2015-12-08 21:06:34]

The CQ bit was checked by kapishnikov@chromium.org

[commit-bot: I haz the power, 2015-12-08 21:09:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1503943003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1503943003/20001

[commit-bot: I haz the power, 2015-12-09 01:00:12]

Description was changed from

==========
[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class
that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and
PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check
persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates
dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation
within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and
the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365
==========

to

==========
[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class
that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and
PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check
persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates
dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation
within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and
the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365
==========

[commit-bot: I haz the power, 2015-12-09 01:00:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-09 01:01:01]

Description was changed from

==========
[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class
that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and
PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check
persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates
dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation
within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and
the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365
==========

to

==========
[Cronet] Unit test refactoring and fixes

1. Refactoring: introduced registerHostResolver(...) method in the base class
that allow registering
   the resolver for the legacy and new APIs. Refactored QuicTest, SdchTest and
PkpTest to use it.

2. Bug fix: Every call to start CronetTestBase.startCronetTestFramework…()
resulted in deletion of QUIC
   cache. It is not desirable in some cases, e.g. in tests that check
persistence. As the result of the
   change, the cache is always cleared on setUp() method. That eliminates
dependencies between tests.
   If a test needs to clear the cache between multiple engine instantiation
within a single test, it
   can call CronetTestFramework.prepareTestStorage() method.

To simplify the CL, the changes related to the CronetHttpURLConnection tests and
the CronetTestBase.mCronetTestFramework member are excluded from this CL.

BUG=565365

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

[commit-bot: I haz the power, 2015-12-09 01:01:01]

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