[Cronet] Clean up tests

[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

BUG=547160
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2892013002
Cr-Commit-Position: refs/heads/master@{#474894}
Committed: https://chromium.googlesource.com/chromium/src/+/a669662d38b1c4a8071ea4cde7a904b2703e0e71

[pauljensen, 2017-05-18 18:02:54]

Description was changed from

==========
[Cronet] Clean up tests
==========

to

==========
[Cronet] Clean up tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[pauljensen, 2017-05-22 13:47:45]

Description was changed from

==========
[Cronet] Clean up tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  The command-line-args options were not 
saving lines of code (evident in the net reduction in lines of code in this
CL) and were adding a layer of abstraction that complicated tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[pauljensen, 2017-05-22 14:22:51]

Description was changed from

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  The command-line-args options were not 
saving lines of code (evident in the net reduction in lines of code in this
CL) and were adding a layer of abstraction that complicated tests.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[pauljensen, 2017-05-22 16:02:07]

Description was changed from

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

BUG=547160

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-05-22 18:56:40]

mgersh@chromium.org changed reviewers:
+ mgersh@chromium.org

[mgersh, 2017-05-23 18:12:13]

Thanks for doing this!

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:372:
startCronetTestFrameworkWithCronetEngineBuilder(builder);
Why does this one still need the framework?

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:177:
createJavaEngineBuilder().build().newBidirectionalStreamBuilder(
It looks like the point of this method is that it's called after the
CronetEngine is already created, so using mCronetEngine here should work, and if
it doesn't that's a bug.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:179:
* Sets the {@link UrlStreamHandlerFactory} from {@code cronetEngine}.  This
should be called
nit: Url... -> URL...

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:180:
* during setup() and is installed by {@link runTest()} as the default when
Cronet is tested.
nit: setup() -> setUp()

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:893:
private CronetEngine createCronetEngine(int cacheType) {
Since this is now used in a test earlier in the file too, it would make more
sense near the beginning of the file, and might want a more descriptive name
like createCronetEngineWithCache or something so it's clear why it's used only
in a few tests.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:896:
enableDiskCache(builder);
This way of separating things is a little strange. Instead of having an
enableDiskCache() that sets the storage path and then does the same thing as the
else case, I'd prefer doing just the setStoragePath() in the if.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:406:
private void startCronetFramework() {
It's not using a Framework anymore, so this could be startCronetEngine instead,
or just delete it.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:90:
public void startNetLog() {
This class is kind of a strange combination of things: it can create a
CronetEngine with some nice default settings, it can start a netlog but only if
you used it for the default CronetEngine settings, and it has some static
methods related to storage. In my ideal world it would all live in
CronetTestBase. Most of that could be a future cleanup, but I think it's worth
moving the netlog stuff now since this refactor makes those methods not usable
for a lot of tests where you could previously use them.

[pauljensen, 2017-05-25 15:15:15]

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:372:
startCronetTestFrameworkWithCronetEngineBuilder(builder);
On 2017/05/23 18:12:12, mgersh wrote:
> Why does this one still need the framework?

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:177:
createJavaEngineBuilder().build().newBidirectionalStreamBuilder(
On 2017/05/23 18:12:12, mgersh wrote:
> It looks like the point of this method is that it's called after the
> CronetEngine is already created, so using mCronetEngine here should work, and
if
> it doesn't that's a bug.

Previously startCronetTestFrameworkWithUrlAndCronetEngineBuilder() was setting
mCronetTestFramework to mTestFramework, and CronetTestBase.runTest() was
swapping mTestFramework.mCronetEngine for a JavaCronetEngine.  I removed the
call to startCronetTestFrameworkWithUrlAndCronetEngineBuilder() so this swap is
no longer happening, so mCronetEngine will be a CronetUrlRequestContext not a
JavaCronetEngine instance.  To keep this test testing what it was designed to
test I manually create a JavaCronetEngine here.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:179:
* Sets the {@link UrlStreamHandlerFactory} from {@code cronetEngine}.  This
should be called
On 2017/05/23 18:12:13, mgersh wrote:
> nit: Url... -> URL...

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:180:
* during setup() and is installed by {@link runTest()} as the default when
Cronet is tested.
On 2017/05/23 18:12:13, mgersh wrote:
> nit: setup() -> setUp()

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:893:
private CronetEngine createCronetEngine(int cacheType) {
On 2017/05/23 18:12:13, mgersh wrote:
> Since this is now used in a test earlier in the file too, it would make more
> sense near the beginning of the file, and might want a more descriptive name
> like createCronetEngineWithCache or something so it's clear why it's used only
> in a few tests.

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:896:
enableDiskCache(builder);
On 2017/05/23 18:12:13, mgersh wrote:
> This way of separating things is a little strange. Instead of having an
> enableDiskCache() that sets the storage path and then does the same thing as
the
> else case, I'd prefer doing just the setStoragePath() in the if.

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:406:
private void startCronetFramework() {
On 2017/05/23 18:12:13, mgersh wrote:
> It's not using a Framework anymore, so this could be startCronetEngine
instead,
> or just delete it.

Done.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/src/org/chromium/net/CronetTestFramework.java:90:
public void startNetLog() {
On 2017/05/23 18:12:13, mgersh wrote:
> This class is kind of a strange combination of things: it can create a
> CronetEngine with some nice default settings, it can start a netlog but only
if
> you used it for the default CronetEngine settings, and it has some static
> methods related to storage. In my ideal world it would all live in
> CronetTestBase. Most of that could be a future cleanup, but I think it's worth
> moving the netlog stuff now since this refactor makes those methods not usable
> for a lot of tests where you could previously use them.

Done, netlog stuff was actually dead, so I just moved the rest into
CronetTestBase.

[mgersh, 2017-05-25 18:11:17]

lgtm once the last few things are fixed. Looks like the trybot failure is just a
stray import.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java:177:
createJavaEngineBuilder().build().newBidirectionalStreamBuilder(
On 2017/05/25 15:15:14, pauljensen wrote:
> On 2017/05/23 18:12:12, mgersh wrote:
> > It looks like the point of this method is that it's called after the
> > CronetEngine is already created, so using mCronetEngine here should work,
and
> if
> > it doesn't that's a bug.
> 
> Previously startCronetTestFrameworkWithUrlAndCronetEngineBuilder() was setting
> mCronetTestFramework to mTestFramework, and CronetTestBase.runTest() was
> swapping mTestFramework.mCronetEngine for a JavaCronetEngine.  I removed the
> call to startCronetTestFrameworkWithUrlAndCronetEngineBuilder() so this swap
is
> no longer happening, so mCronetEngine will be a CronetUrlRequestContext not a
> JavaCronetEngine instance.  To keep this test testing what it was designed to
> test I manually create a JavaCronetEngine here.

Acknowledged.

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:179:
* Sets the {@link UrlStreamHandlerFactory} from {@code cronetEngine}.  This
should be called
On 2017/05/25 15:15:14, pauljensen wrote:
> On 2017/05/23 18:12:13, mgersh wrote:
> > nit: Url... -> URL...
> 
> Done.

This and the one below aren't actually done?

[pauljensen, 2017-05-26 00:14:03]

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java
(right):

https://codereview.chromium.org/2892013002/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetTestBase.java:179:
* Sets the {@link UrlStreamHandlerFactory} from {@code cronetEngine}.  This
should be called
On 2017/05/25 18:11:16, mgersh wrote:
> On 2017/05/25 15:15:14, pauljensen wrote:
> > On 2017/05/23 18:12:13, mgersh wrote:
> > > nit: Url... -> URL...
> > 
> > Done.
> 
> This and the one below aren't actually done?

Done.

[pauljensen, 2017-05-26 00:23:28]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2017-05-26 00:23:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mgersh@chromium.org
Link to the patchset: https://codereview.chromium.org/2892013002/#ps180001
(title: "address comments")

[commit-bot: I haz the power, 2017-05-26 00:24:19]

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-05-26 03:12:08]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495758208699900,
"parent_rev": "c08b94e9b264f92b5478462b4846fa46ae1e1b9d", "commit_rev":
"a669662d38b1c4a8071ea4cde7a904b2703e0e71"}

[commit-bot: I haz the power, 2017-05-26 03:12:20]

Description was changed from

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

BUG=547160

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
[Cronet] Clean up tests

Remove most of CronetTestFramework.  Cronet tests haven't passing options
on the command line for a long time so remove the command-line-args 
abstraction layer.  Saves 250+ lines of code.

BUG=547160

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

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

[commit-bot: I haz the power, 2017-05-26 03:12:21]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a669662d38b1c4a8071ea4cde7a9...