Add unit tests for media.remote.AbstractMediaRouteController

Add unit tests for media.remote.AbstractMediaRouteController

BUG=472067
Committed: https://crrev.com/63b65fd1f43a9e2bc0f187567ca031643bd8c166
Cr-Commit-Position: refs/heads/master@{#327254}

[dgn, 2015-04-24 15:51:50]

dgn@chromium.org changed reviewers:
+ aberent@chromium.org, bauerb@chromium.org

[Bernhard Bauer, 2015-04-24 16:03:47]

LGTM w/ nits:

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:165:
Matchers.any(MediaRouteSelector.class), Matchers.anyInt())).thenReturn(true);
Indent 8 spaces

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:187:
Matchers.any(MediaRouteSelector.class), Matchers.anyInt())).thenReturn(false);
Indent 8 spaces

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:249:
* A dummy class used here to be able to instanciate the abstract class while
calling its
Nit: "instantiate"

[dgn, 2015-04-24 16:11:43]

Thanks, fixed! I'll see if Anthony has comments before submitting.

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:165:
Matchers.any(MediaRouteSelector.class), Matchers.anyInt())).thenReturn(true);
On 2015/04/24 16:03:46, Bernhard Bauer wrote:
> Indent 8 spaces

Done.

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:187:
Matchers.any(MediaRouteSelector.class), Matchers.anyInt())).thenReturn(false);
On 2015/04/24 16:03:46, Bernhard Bauer wrote:
> Indent 8 spaces

Done.

https://codereview.chromium.org/1066863004/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:249:
* A dummy class used here to be able to instanciate the abstract class while
calling its
On 2015/04/24 16:03:46, Bernhard Bauer wrote:
> Nit: "instantiate"

Done.

[aberent, 2015-04-27 11:06:10]

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
(right):

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:225:
startWatchingRouteSelection();
You can't use this here! It uses a different callback.

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:43:
/** Tests for {@link AbstractMediaRouteController}. */
This doesn't seem to be a complete set of tests. Where are the tests for, for
example UiListeners, or for isRemotePlaybackAvailable? If the intention is to
add these in a separate CL, fine, but please say so in the commit comment and CL
description.

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:66:
DummyMediaRouteController.sMediaRouteSelector = mock(MediaRouteSelector.class);
Move to beforeTest? You seem to do this in every test.

[dgn, 2015-04-27 16:35:23]

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java
(right):

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteController.java:225:
startWatchingRouteSelection();
On 2015/04/27 11:06:10, aberent wrote:
> You can't use this here! It uses a different callback.

Done.

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:43:
/** Tests for {@link AbstractMediaRouteController}. */
On 2015/04/27 11:06:10, aberent wrote:
> This doesn't seem to be a complete set of tests. Where are the tests for, for
> example UiListeners, or for isRemotePlaybackAvailable? If the intention is to
> add these in a separate CL, fine, but please say so in the commit comment and
CL
> description.

The coverage should be better now. I had issues with mocking RouteInfo but I
managed to go around it using reflection. But I'm not very fond of the amount of
hacking that goes in those tests and I'm afraid that they are testing
implementation details.

https://codereview.chromium.org/1066863004/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:66:
DummyMediaRouteController.sMediaRouteSelector = mock(MediaRouteSelector.class);
On 2015/04/27 11:06:10, aberent wrote:
> Move to beforeTest? You seem to do this in every test.

Done.

[aberent, 2015-04-27 17:09:21]

lgtm

https://codereview.chromium.org/1066863004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:219:
// The listener should not be notified of changes anymore.
You should possibly also test the case of multiple listeners. In particular
removing one listener should not affect the other listeners.

This seems like it might go wrong, and hence is worth testing, because there is
special handling of removal of the last listener.

[dgn, 2015-04-28 09:11:58]

Thanks for the review!

https://codereview.chromium.org/1066863004/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java
(right):

https://codereview.chromium.org/1066863004/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java:219:
// The listener should not be notified of changes anymore.
On 2015/04/27 at 17:09:20, aberent wrote:
> You should possibly also test the case of multiple listeners. In particular
removing one listener should not affect the other listeners.
> 
> This seems like it might go wrong, and hence is worth testing, because there
is special handling of removal of the last listener.

Done

[dgn, 2015-04-28 09:12:00]

The CQ bit was checked by dgn@chromium.org

[dgn, 2015-04-28 09:12:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
aberent@chromium.org
Link to the patchset: https://codereview.chromium.org/1066863004/#ps60001
(title: "Fix comments, test name, better add/removeMediaStateListener testing")

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

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

[commit-bot: I haz the power, 2015-04-28 09:42:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-28 09:43:20]

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