Split functionality out of MinidumpUploaderTest into smaller components.

Split functionality out of MinidumpUploaderTest into smaller components.

Prepare to split MinidumpUploaderTest into WebView-specific tests, and
tests shared between WebView and Chrome, by splitting some
test-functionality out into smaller components and removing unnecessary
dependencies on AwMinidumpUploaderDelegate.

BUG=692448
Review-Url: https://codereview.chromium.org/2820683002
Cr-Commit-Position: refs/heads/master@{#465597}
Committed: https://chromium.googlesource.com/chromium/src/+/2be4a32532d91c71aaafdaa91ee3ee8278c32076

[gsennton, 2017-04-14 10:55:17]

gsennton@chromium.org changed reviewers:
+ isherman@chromium.org, tobiasjs@chromium.org

[gsennton, 2017-04-14 10:55:17]

Hi guys (Happy Easter! ;),

this is a new take on https://codereview.chromium.org/2709323004/

The goal is to be able to split MinidumpUploaderTest into WebView-specific and
non-WebView-specific tests (so the latter will live under
components/minidump_uploader).

The WebView-specific tests will be testing the PlatformServiceBridge and
AwMinidumpUploaderDelegate, while the non-WebView specific tests will be testing
MinidumpUploaderImpl.

ptal :)

[Ilya Sherman, 2017-04-14 16:35:45]

Thanks, Gustav.

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
File
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java
(right):

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:33:
public static void uploadAllMinidumpsOnUiThread(final MinidumpUploader
minidumpUploader,
nit: Please document.

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:46:
jobPostedLatch.await();
Should this also have a timeout specified?  Or is it fine to just rely on the
test harness here?  (I suppose it's not entirely clear to me why we specify a
custom timeout below.)

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:53:
public static void uploadMinidumpsSync(
nit: Please document.

[gsennton, 2017-04-17 11:38:57]

Thanks Ilya!

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
File
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java
(right):

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:33:
public static void uploadAllMinidumpsOnUiThread(final MinidumpUploader
minidumpUploader,
On 2017/04/14 16:35:45, Ilya Sherman wrote:
> nit: Please document.

Done.

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:46:
jobPostedLatch.await();
On 2017/04/14 16:35:45, Ilya Sherman wrote:
> Should this also have a timeout specified?  Or is it fine to just rely on the
> test harness here?  (I suppose it's not entirely clear to me why we specify a
> custom timeout below.)

Yep, I added a timeout here in PS2. Normally it is recommended to specify a
time-out (I think the test-harness time-out before interrupting a test case can
be very high).
As for the custom timeout, I don't think we have a general timeout for
instrumentation tests, there are lots of custom timeouts in both chrome/ and
android_webview/, e.g. in AwTestBase.java, PostMessageTest.java,
ArchiveTest.java, AwContentsClientOnFormResubmissionTest.java,
PopularUrlsTest.java and TabsTest.java.

https://codereview.chromium.org/2820683002/diff/1/components/minidump_uploade...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:53:
public static void uploadMinidumpsSync(
On 2017/04/14 16:35:45, Ilya Sherman wrote:
> nit: Please document.

Done.

[Ilya Sherman, 2017-04-17 16:27:51]

LGTM, thanks Gustav.

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
File
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java
(right):

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:63:
* @param minidumpUploader the implementation to use to upload minidump.
nit: s/minidump/minidumps

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:65:
* e.g. when uploading succeeds we should normally not expect to reschedule.
nit: Indent this line extra?

[gsennton, 2017-04-17 18:08:11]

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
File
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java
(right):

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:63:
* @param minidumpUploader the implementation to use to upload minidump.
On 2017/04/17 16:27:50, Ilya Sherman wrote:
> nit: s/minidump/minidumps

Done.

https://codereview.chromium.org/2820683002/diff/20001/components/minidump_upl...
components/minidump_uploader/android/javatests/src/org/chromium/components/minidump_uploader/MinidumpUploadTestUtility.java:65:
* e.g. when uploading succeeds we should normally not expect to reschedule.
On 2017/04/17 16:27:50, Ilya Sherman wrote:
> nit: Indent this line extra?

Done (same for blockUntilJobPosted further up in this file).

[Tobias Sargeant, 2017-04-19 13:05:39]

LGTM

[gsennton, 2017-04-19 13:19:29]

Thanks! Toby had a question offline about the use of AwMinidumpUploaderDelegate
in the tests - so I added a couple of comments on this (because the reason for
the different uses of AwMinidumpUploaderDelegate vs.
TestMinidumpUploaderDelegate is not obvious).

[gsennton, 2017-04-19 13:19:40]

The CQ bit was checked by gsennton@chromium.org

[gsennton, 2017-04-19 13:19:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, tobiasjs@chromium.org
Link to the patchset: https://codereview.chromium.org/2820683002/#ps60001
(title: "Add comments regarding the use of AwMinidumpUploaderDelegate in
MinidumpUploaderTest.")

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

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-04-19 14:34:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492607980967850,
"parent_rev": "c8717b5675bb7e85d6bfca8ce5bfb9afc55417ec", "commit_rev":
"2be4a32532d91c71aaafdaa91ee3ee8278c32076"}

[commit-bot: I haz the power, 2017-04-19 14:35:12]

Description was changed from

==========
Split functionality out of MinidumpUploaderTest into smaller components.

Prepare to split MinidumpUploaderTest into WebView-specific tests, and
tests shared between WebView and Chrome, by splitting some
test-functionality out into smaller components and removing unnecessary
dependencies on AwMinidumpUploaderDelegate.

BUG=692448
==========

to

==========
Split functionality out of MinidumpUploaderTest into smaller components.

Prepare to split MinidumpUploaderTest into WebView-specific tests, and
tests shared between WebView and Chrome, by splitting some
test-functionality out into smaller components and removing unnecessary
dependencies on AwMinidumpUploaderDelegate.

BUG=692448

Review-Url: https://codereview.chromium.org/2820683002
Cr-Commit-Position: refs/heads/master@{#465597}
Committed:
https://chromium.googlesource.com/chromium/src/+/2be4a32532d91c71aaafdaa91ee3...
==========

[commit-bot: I haz the power, 2017-04-19 14:35:13]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2be4a32532d91c71aaafdaa91ee3...