Add test code for UsbChooserDialog.java

Add test code for UsbChooserDialog.java

This patch added UsbChooserDialogTest.java to test the class
UsbChooserDialog.

BUG=591735
Committed: https://crrev.com/bc29520c1589b48edf5a3447bffb33a8761cc59b
Cr-Commit-Position: refs/heads/master@{#384832}

[juncai, 2016-03-28 23:32:12]

juncai@chromium.org changed reviewers:
+ newt@chromium.org

[juncai, 2016-03-28 23:32:12]

Please take a look.

[newt (away), 2016-03-28 23:48:58]

newt@chromium.org changed reviewers:
+ finnur@chromium.org

[newt (away), 2016-03-28 23:48:58]

finnur: Would you mind taking a look here?

[Finnur, 2016-03-30 13:18:54]

LGTM

[newt (away), 2016-04-01 17:42:48]

a few minor comments

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java
(right):

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:37:
UsbChooserDialogWithFakeNatives(long nativeUsbChooserDialogPtr) {
suggestion: I'd remove the nativeUsbChooserDialogPtr parameter and just call
super(42 /* nativeUsbChooserDialogPtr */).

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:79:
ActivityWindowAndroid windowAndroid =
You could remove lines 79-81 and change 82 to:

    dialog.show(getActivity(), ...);

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:102:
assertEquals("Not all items have a view; positions may be incorrect.",
So you're assuming that all the items fit on the screen? If there are more than
two or three items in the list, then this could fail on small devices and it'd
be better to scroll to the item that you want to click:

"mItemChooserDialog.setSelection(position)" will show the desired position at
the top of the ListView. ListView.getFirstVisiblePosition() might also be
useful.

Follow-up, since there are only three items this may be OK, if you're convinced
that this will succeed even on 320dp x 480dp screens.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:123:
assertFalse(chooserDialog.mSelectedDeviceId.equals(""));
This line isn't needed. If CriteriaHelper.pollUiThread() never succeeds, then it
throws an exception. Thus, at this point we already know that
chooserDialog.mSelectedDeviceId is non-empty.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:157:
assertEquals("", mChooserDialog.mSelectedDeviceId);
Likewise, this line isn't needed.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:189:
assertEquals("device_id_1", mChooserDialog.mSelectedDeviceId);
Maybe add "assertTrue(button.isEnabled());" here? and check the status of the
help message?

[juncai, 2016-04-01 23:06:50]

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java
(right):

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:37:
UsbChooserDialogWithFakeNatives(long nativeUsbChooserDialogPtr) {
On 2016/04/01 17:42:48, newt wrote:
> suggestion: I'd remove the nativeUsbChooserDialogPtr parameter and just call
> super(42 /* nativeUsbChooserDialogPtr */).

Done.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:79:
ActivityWindowAndroid windowAndroid =
On 2016/04/01 17:42:48, newt wrote:
> You could remove lines 79-81 and change 82 to:
> 
>     dialog.show(getActivity(), ...);

Done.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:102:
assertEquals("Not all items have a view; positions may be incorrect.",
On 2016/04/01 17:42:48, newt wrote:
> So you're assuming that all the items fit on the screen? If there are more
than
> two or three items in the list, then this could fail on small devices and it'd
> be better to scroll to the item that you want to click:
> 
> "mItemChooserDialog.setSelection(position)" will show the desired position at
> the top of the ListView. ListView.getFirstVisiblePosition() might also be
> useful.
> 
> Follow-up, since there are only three items this may be OK, if you're
convinced
> that this will succeed even on 320dp x 480dp screens.

Done.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:123:
assertFalse(chooserDialog.mSelectedDeviceId.equals(""));
On 2016/04/01 17:42:48, newt wrote:
> This line isn't needed. If CriteriaHelper.pollUiThread() never succeeds, then
it
> throws an exception. Thus, at this point we already know that
> chooserDialog.mSelectedDeviceId is non-empty.

Done.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:157:
assertEquals("", mChooserDialog.mSelectedDeviceId);
On 2016/04/01 17:42:48, newt wrote:
> Likewise, this line isn't needed.

Done.

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:189:
assertEquals("device_id_1", mChooserDialog.mSelectedDeviceId);
On 2016/04/01 17:42:48, newt wrote:
> Maybe add "assertTrue(button.isEnabled());" here? and check the status of the
> help message?

In the selectItem() function, after the 'CONNECT' button is clicked, the chooser
dialog is closed. The button is tested enabled inside that function, and in this
function, the help message is tested. So maybe not necessary to test them again
after the dialog is closed?

[newt (away), 2016-04-02 00:29:56]

lgtm after one last comment. Thanks for writing these tests :)

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java
(right):

https://codereview.chromium.org/1843483002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:189:
assertEquals("device_id_1", mChooserDialog.mSelectedDeviceId);
On 2016/04/01 23:06:50, juncai wrote:
> On 2016/04/01 17:42:48, newt wrote:
> > Maybe add "assertTrue(button.isEnabled());" here? and check the status of
the
> > help message?
> 
> In the selectItem() function, after the 'CONNECT' button is clicked, the
chooser
> dialog is closed. The button is tested enabled inside that function, and in
this
> function, the help message is tested. So maybe not necessary to test them
again
> after the dialog is closed?

Ah, right. This looks good then.

https://codereview.chromium.org/1843483002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java
(right):

https://codereview.chromium.org/1843483002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:92:
items.setSelection(position);
Modify UI objects only on the UI thread. (Technically, reading UI objects should
only happen on the UI thread, but it makes tests way less readable, so we don't
always do that.) This should be wrapped in ThreadUtils.runOnUiThreadBlocking().

[juncai, 2016-04-03 16:19:42]

https://codereview.chromium.org/1843483002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java
(right):

https://codereview.chromium.org/1843483002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/UsbChooserDialogTest.java:92:
items.setSelection(position);
On 2016/04/02 00:29:56, newt wrote:
> Modify UI objects only on the UI thread. (Technically, reading UI objects
should
> only happen on the UI thread, but it makes tests way less readable, so we
don't
> always do that.) This should be wrapped in
ThreadUtils.runOnUiThreadBlocking().

Done.

[juncai, 2016-04-03 16:19:54]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-04-03 16:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1843483002/#ps60001
(title: "address newt@'s comment")

[commit-bot: I haz the power, 2016-04-03 16:20:05]

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

[commit-bot: I haz the power, 2016-04-03 16:59:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-03 17:01:23]

Description was changed from

==========
Add test code for UsbChooserDialog.java

This patch added UsbChooserDialogTest.java to test the class
UsbChooserDialog.

BUG=591735
==========

to

==========
Add test code for UsbChooserDialog.java

This patch added UsbChooserDialogTest.java to test the class
UsbChooserDialog.

BUG=591735

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

[commit-bot: I haz the power, 2016-04-03 17:01:24]

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