[Feedback] Add simple test for FeedbackCollector

[Feedback] Add simple test for FeedbackCollector

Up until now the FeedbackCollector has been untested. This CL adds a simple
test to ensure that the class works as it should in the default case.
This also paves the way for making it simpler to write more tests once the
callback-based approach is used.

BUG=386395
Committed: https://crrev.com/327e41961e883621a5c9d34b5f203d28455e94d5
Cr-Commit-Position: refs/heads/master@{#337114}

[nyquist, 2015-06-30 21:02:50]

nyquist@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[nyquist, 2015-06-30 21:02:51]

dfalcantara: PTAL

[gone, 2015-07-01 16:01:10]

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java:45:
private static final int CONNECTIVITY_CHECK_TIMEOUT_MS = 5000;
Did this get bumped up because there's more data to collect?

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:25:
public class FeedbackCollectorTest extends ChromeShellTestBase {
Don't add any more ChromeShell tests.  Newton's nuking them.  Can this be made
into a ChromePublic test instead?

[nyquist, 2015-07-01 18:10:59]

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java:45:
private static final int CONNECTIVITY_CHECK_TIMEOUT_MS = 5000;
On 2015/07/01 16:01:10, dfalcantara wrote:
> Did this get bumped up because there's more data to collect?

No. Instead of being the user-visible timeout that it was planned being, it is
not just an upper bound for how long the connectivity checks will stick around.

The timeout in FeedbackCollector will still be important timeout. Since the
other long-running task is screenshots, and the timeout is 500 ms in
FeedbackCollector, it would require the ScreenshotTask to take ~ 5s for this
timeout to matter.

So yeah, when used for feedback purposes, this timeout isn't as important
anymore.

I guess we could set it to 500ms or so to ensure we get the TIMEOUT value
instead of UNKNOWN in case things are slow.

Any thoughts?

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:25:
public class FeedbackCollectorTest extends ChromeShellTestBase {
On 2015/07/01 16:01:10, dfalcantara wrote:
> Don't add any more ChromeShell tests.  Newton's nuking them.  Can this be made
> into a ChromePublic test instead?

Thanks for noticing! Doing this in https://codereview.chromium.org/1216403003/

Hope that's OK.

[nyquist, 2015-07-01 18:14:04]

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java:45:
private static final int CONNECTIVITY_CHECK_TIMEOUT_MS = 5000;
On 2015/07/01 18:10:58, nyquist wrote:
> On 2015/07/01 16:01:10, dfalcantara wrote:
> > Did this get bumped up because there's more data to collect?
> 
> No. Instead of being the user-visible timeout that it was planned being, it is
> not just an upper bound for how long the connectivity checks will stick
around.
> 
> The timeout in FeedbackCollector will still be important timeout. Since the
> other long-running task is screenshots, and the timeout is 500 ms in
> FeedbackCollector, it would require the ScreenshotTask to take ~ 5s for this
> timeout to matter.
> 
> So yeah, when used for feedback purposes, this timeout isn't as important
> anymore.
> 
> I guess we could set it to 500ms or so to ensure we get the TIMEOUT value
> instead of UNKNOWN in case things are slow.
> 
> Any thoughts?

Also, it feels like the value-change should be in the next CL, not in this one.
It makes way more sense there at least.

[gone, 2015-07-01 18:28:15]

lgtm % comments

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java
(right):

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java:45:
private static final int CONNECTIVITY_CHECK_TIMEOUT_MS = 5000;
On 2015/07/01 at 18:14:04, nyquist wrote:
> On 2015/07/01 18:10:58, nyquist wrote:
> > On 2015/07/01 16:01:10, dfalcantara wrote:
> > > Did this get bumped up because there's more data to collect?
> > 
> > No. Instead of being the user-visible timeout that it was planned being, it
is
> > not just an upper bound for how long the connectivity checks will stick
around.
> > 
> > The timeout in FeedbackCollector will still be important timeout. Since the
> > other long-running task is screenshots, and the timeout is 500 ms in
> > FeedbackCollector, it would require the ScreenshotTask to take ~ 5s for this
> > timeout to matter.
> > 
> > So yeah, when used for feedback purposes, this timeout isn't as important
> > anymore.
> > 
> > I guess we could set it to 500ms or so to ensure we get the TIMEOUT value
> > instead of UNKNOWN in case things are slow.
> > 
> > Any thoughts?
> 
> Also, it feels like the value-change should be in the next CL, not in this
one. It makes way more sense there at least.

Eh, it's fine.  Was just confused about it.

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java
(right):

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:37:
* In addition, the {@link FeedbackCollector#init} method is intentionally empty
to ensure
Neither FeedbackCollor#init() nor TestFeedbackCollector#init() look empty here.

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:75:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
Isn't there an equivalent for Callables, which actually return values instead of
a void run() for Runnables?  Would avoid this AtomicReference stuff.

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:96:
final AtomicReference<Bitmap> bitmapRef = new AtomicReference<>();
Same as comment above.

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:108:
final AtomicReference<Bundle> bundleRef = new AtomicReference<>();
Yep.

https://chromiumcodereview.appspot.com/1211283005/diff/20001/chrome/android/j...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:185:
final AtomicReference<TestFeedbackCollector> collectorRef = new
AtomicReference<>();
same as above.

[nyquist, 2015-07-01 20:19:59]

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/feedback/FeedbackCollector.java:45:
private static final int CONNECTIVITY_CHECK_TIMEOUT_MS = 5000;
On 2015/07/01 18:28:14, dfalcantara wrote:
> On 2015/07/01 at 18:14:04, nyquist wrote:
> > On 2015/07/01 18:10:58, nyquist wrote:
> > > On 2015/07/01 16:01:10, dfalcantara wrote:
> > > > Did this get bumped up because there's more data to collect?
> > > 
> > > No. Instead of being the user-visible timeout that it was planned being,
it
> is
> > > not just an upper bound for how long the connectivity checks will stick
> around.
> > > 
> > > The timeout in FeedbackCollector will still be important timeout. Since
the
> > > other long-running task is screenshots, and the timeout is 500 ms in
> > > FeedbackCollector, it would require the ScreenshotTask to take ~ 5s for
this
> > > timeout to matter.
> > > 
> > > So yeah, when used for feedback purposes, this timeout isn't as important
> > > anymore.
> > > 
> > > I guess we could set it to 500ms or so to ensure we get the TIMEOUT value
> > > instead of UNKNOWN in case things are slow.
> > > 
> > > Any thoughts?
> > 
> > Also, it feels like the value-change should be in the next CL, not in this
> one. It makes way more sense there at least.
> 
> Eh, it's fine.  Was just confused about it.

Okay. I'll still move the number-change to the next CL though, to make it
clearer why the change happened.

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
File
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java
(right):

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:37:
* In addition, the {@link FeedbackCollector#init} method is intentionally empty
to ensure
On 2015/07/01 18:28:14, dfalcantara wrote:
> Neither FeedbackCollor#init() nor TestFeedbackCollector#init() look empty
here.

Done.

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:75:
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
On 2015/07/01 18:28:14, dfalcantara wrote:
> Isn't there an equivalent for Callables, which actually return values instead
of
> a void run() for Runnables?  Would avoid this AtomicReference stuff.

Wow. Yeah. Sorry. Thanks! Done.

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:96:
final AtomicReference<Bitmap> bitmapRef = new AtomicReference<>();
On 2015/07/01 18:28:14, dfalcantara wrote:
> Same as comment above.

Done.

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:108:
final AtomicReference<Bundle> bundleRef = new AtomicReference<>();
On 2015/07/01 18:28:14, dfalcantara wrote:
> Yep.

Done.

https://codereview.chromium.org/1211283005/diff/20001/chrome/android/javatest...
chrome/android/javatests_shell/src/org/chromium/chrome/browser/feedback/FeedbackCollectorTest.java:185:
final AtomicReference<TestFeedbackCollector> collectorRef = new
AtomicReference<>();
On 2015/07/01 18:28:14, dfalcantara wrote:
> same as above.

Done.

[nyquist, 2015-07-01 20:43:06]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2015-07-01 20:43:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1211283005/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2015-07-01 20:44:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211283005/60001

[commit-bot: I haz the power, 2015-07-01 21:24:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-01 21:25:06]

Patchset 4 (id:??) landed as
https://crrev.com/327e41961e883621a5c9d34b5f203d28455e94d5
Cr-Commit-Position: refs/heads/master@{#337114}