Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1593313011:
Adding unittests for media casting sink observation and route handling (Closed)
|
Created:
4 years, 11 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 11 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, mcasas+watch_chromium.org, media-router+watch_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding unittests for media casting sink observation and route handling
This patch adds a mock MediaRouteProvider for unittest needs, as well as
unittests for sink observation and route handling.
BUG=578835
Committed: https://crrev.com/d20c259e2aabe9fbed12b1991c02e04eb431f1a8
Cr-Commit-Position: refs/heads/master@{#371247}
Patch Set 1 : #Patch Set 2 : fix compilation error #
Total comments: 68
Patch Set 3 : addressed comments of avayvod@ and mlamouri@ #Patch Set 4 : Using robolectric reflection to create mock RouteInfo #
Total comments: 26
Patch Set 5 : addressed comments of avayvod@ #
Total comments: 16
Patch Set 6 : addressed avayvod@'s comments #
Total comments: 2
Messages
Total messages: 22 (6 generated)
Patchset #1 (id:1) has been deleted
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org
PTAL
Patchset #2 (id:40001) has been deleted
some drive-by comments. I will let avayvod@ do the core of the review. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:34: // The pointer to the native object. Can be null only for testing. nit: s/for testing/during tests/ https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:45: public final Map<String, List<MediaSink>> mSinksPerSource = I would prefer to have accessors that test code would use. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:137: nativeMediaRouterAndroid, applicationContext); why this change? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:312: mNativeMediaRouterAndroid = nativeMediaRouter; nit: maybe you can initialize in the order of the arguments? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/MediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/MediaRouteProvider.java:92: void setMediaRouteManager(MediaRouteManager manager); If this is only used by the Mock, is it possible to cast the route provider as a MockRouteProvider in the tests and call the method directly on the mock so we don't add it to production code too? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:47: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); why this change?
Initial round of comments The set of test cases looks good. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:36: @VisibleForTesting I don't think having public data members for tests is a good idea. Rather have a getter method with a clear name like getRouteProvidersForTest() somewhere? Package visibility should also be enough. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:50: return mRouteProviders; hm... so you have the member public and also an accessor? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:310: ChromeMediaRouter(long nativeMediaRouter, MediaRouteProvider provider) { Perhaps, we should just have a ctor that takes the nativeMediaRouter argument and a @VisibleForTest method to add providers. Then in the create(nativeMR, applicationContext) you would add CastMediaRouteProvider.create() if it's not null, and in test you would call the addMediaRouteProvider method with the mock object. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:362: // This method is only implemented in {@link MockMediaRouteProvider}. Sounds like you could just pass it to the constructor of MockMediaRouteProvider? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:47: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); Agree. By the time this method is called, discovery delegate should no longer need to get updates for this source urn. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:66: public void onRouteAddedInternal(MediaSink sink) { Hm, can't you use Robolectric to create fake MediaRouter and RouteInfo to pass to the original methods instead of creating extra indirection level just for tests? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java:23: @Config(manifest = Config.NONE, This test is pretty small, should it be merged with the main sinks observing test? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java:27: public void setUp() { nit: doesn't seem like you need to override this just to call the super class https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:31: public void setUp() { nit: ditto https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:83: provider.createRouteFail( nit: should be createRouteFailure to match Success in the previous test. Fail is a verb https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:157: provider.joinRouteFail( nit: ditto https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:219: assertEquals(mChromeMediaRouter.mRouteIdsToProviders.size(), 2); I think that you don't need to repeatedly test what you cover in another test case. You already verified that creating two routes works, no need to do it in the detachRouteTest -- only verify something new in each test method. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:226: // TODO(zqzhang): should be 1? yes, perhaps this should be fixed in this or a separate earlier cl :) no need to add tests for bugs I think https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:236: doAnswer(new Answer() { this seems like a good candidate for a helper method? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:267: String routeId1 = getFirstElement(mRouteProvider.mRoutes).getKey(); IIRC, routeIds are not random. Maybe you could verify the returned route id in the createRoute/joinRouteTest-s and then just use the ROUTE_ID1, vs. obtaining the first element here and above? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:273: assertEquals(mChromeMediaRouter.mRouteIdsToProviders.size(), 2); ditto to fix this before landing the tests https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:32: private static class ListOfSize extends ArgumentMatcher<List<MediaSink>> { maybe this can be generalized and put somewhere in a common place? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:52: public void setUp() { nit: remove? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:59: // TODO(zqzhang): maybe we need to test for multiple AppIds? afaik, passing multiple app ids in a single source urn is not supported yet, but it is something that might be added in the future. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:71: assertEquals(mChromeMediaRouter.mSinksPerSourcePerProvider.get(SOURCE_ID1).size(), 1); do we have to check all the internal state here? imagine we create new entries in the maps on the fly when we actually receive sinks, we'd have to update the tests then https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:79: mRouteProvider.mDiscoveryCallback.onRouteAddedInternal( I'd rather prefer using Robolectric to fake onRouteAdded/onRouteRemoved callbacks. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jun... for inspiration :) https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:97: testObserveOneSink(); this looks very weird to me... is it a common thing in Chrome tests? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:190: testObserveTwoSinksOfTheSameSource(); I'd say we want test cases to be easily read on their own -- you don't have to copy all the asserts and verification but just the setup. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:251: // Remove a non-existing sink. nit: the comment is wrong https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:272: // Remove a non-existing sink. ditto https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:81: assertEquals(mChromeMediaRouter.getRouteProvidersForTest().size(), 1); If you assert these here, you don't need to assert those in the rest of the test cases. Also, what happens if the assertion fails? Maybe it should just be one separate test method like testInitialState() ? Or it might be too implementation specific as well... https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:90: return c.iterator().next(); this should ideally be somewhere else in the code base :) https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:115: } why don't you pass the manager here instead of adding a new method to the interface?
PTAL, see the notes and replies below. * Addressed comments of avayvod@ and mlamouri@ * Haven't completed shadowing MediaRouter.RouteInfo, working in progress. * Left a question DiscoveryCallback.removeSourceUrn(), need your reply. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:34: // The pointer to the native object. Can be null only for testing. On 2016/01/19 15:23:38, Mounir Lamouri wrote: > nit: s/for testing/during tests/ Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:36: @VisibleForTesting On 2016/01/20 12:11:30, whywhat wrote: > I don't think having public data members for tests is a good idea. Rather have a > getter method with a clear name like getRouteProvidersForTest() somewhere? > Package visibility should also be enough. Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:45: public final Map<String, List<MediaSink>> mSinksPerSource = On 2016/01/19 15:23:38, Mounir Lamouri wrote: > I would prefer to have accessors that test code would use. Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:50: return mRouteProviders; On 2016/01/20 12:11:30, whywhat wrote: > hm... so you have the member public and also an accessor? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:137: nativeMediaRouterAndroid, applicationContext); On 2016/01/19 15:23:38, Mounir Lamouri wrote: > why this change? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:310: ChromeMediaRouter(long nativeMediaRouter, MediaRouteProvider provider) { On 2016/01/20 12:11:30, whywhat wrote: > Perhaps, we should just have a ctor that takes the nativeMediaRouter argument > and a @VisibleForTest method to add providers. Then in the create(nativeMR, > applicationContext) you would add CastMediaRouteProvider.create() if it's not > null, and in test you would call the addMediaRouteProvider method with the mock > object. Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:312: mNativeMediaRouterAndroid = nativeMediaRouter; On 2016/01/19 15:23:38, Mounir Lamouri wrote: > nit: maybe you can initialize in the order of the arguments? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/MediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/MediaRouteProvider.java:92: void setMediaRouteManager(MediaRouteManager manager); On 2016/01/19 15:23:38, Mounir Lamouri wrote: > If this is only used by the Mock, is it possible to cast the route provider as a > MockRouteProvider in the tests and call the method directly on the mock so we > don't add it to production code too? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:362: // This method is only implemented in {@link MockMediaRouteProvider}. On 2016/01/20 12:11:30, whywhat wrote: > Sounds like you could just pass it to the constructor of MockMediaRouteProvider? We now cast the provider to the Mock provider, and sets the MRM. This method is no longer in production code. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:47: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); On 2016/01/20 12:11:30, whywhat wrote: > Agree. By the time this method is called, discovery delegate should no longer > need to get updates for this source urn. Shouldn't we clear the available sinks for the removed source? Maybe I misunderstood the logic. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:66: public void onRouteAddedInternal(MediaSink sink) { On 2016/01/20 12:11:30, whywhat wrote: > Hm, can't you use Robolectric to create fake MediaRouter and RouteInfo to pass > to the original methods instead of creating extra indirection level just for > tests? Working in progress. Haven't completed yet. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java:23: @Config(manifest = Config.NONE, On 2016/01/20 12:11:30, whywhat wrote: > This test is pretty small, should it be merged with the main sinks observing > test? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterLowEndDeviceTest.java:27: public void setUp() { On 2016/01/20 12:11:30, whywhat wrote: > nit: doesn't seem like you need to override this just to call the super class Won't fix since the tests are merged into sink observation tests https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:31: public void setUp() { On 2016/01/20 12:11:30, whywhat wrote: > nit: ditto This method cannot be removed, since we need the @Before annotation to make sure that setUp() is called before each test. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:83: provider.createRouteFail( On 2016/01/20 12:11:30, whywhat wrote: > nit: should be createRouteFailure to match Success in the previous test. Fail is > a verb Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:157: provider.joinRouteFail( On 2016/01/20 12:11:30, whywhat wrote: > nit: ditto Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:219: assertEquals(mChromeMediaRouter.mRouteIdsToProviders.size(), 2); On 2016/01/20 12:11:30, whywhat wrote: > I think that you don't need to repeatedly test what you cover in another test > case. You already verified that creating two routes works, no need to do it in > the detachRouteTest -- only verify something new in each test method. Done, now only create one route. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:226: // TODO(zqzhang): should be 1? On 2016/01/20 12:11:30, whywhat wrote: > yes, perhaps this should be fixed in this or a separate earlier cl :) no need to > add tests for bugs I think Done in this CL. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:236: doAnswer(new Answer() { On 2016/01/20 12:11:30, whywhat wrote: > this seems like a good candidate for a helper method? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:267: String routeId1 = getFirstElement(mRouteProvider.mRoutes).getKey(); On 2016/01/20 12:11:30, whywhat wrote: > IIRC, routeIds are not random. Maybe you could verify the returned route id in > the createRoute/joinRouteTest-s and then just use the ROUTE_ID1, vs. obtaining > the first element here and above? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:273: assertEquals(mChromeMediaRouter.mRouteIdsToProviders.size(), 2); On 2016/01/20 12:11:30, whywhat wrote: > ditto to fix this before landing the tests Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:32: private static class ListOfSize extends ArgumentMatcher<List<MediaSink>> { On 2016/01/20 12:11:30, whywhat wrote: > maybe this can be generalized and put somewhere in a common place? Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:52: public void setUp() { On 2016/01/20 12:11:30, whywhat wrote: > nit: remove? Won't fix (ditto) https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:59: // TODO(zqzhang): maybe we need to test for multiple AppIds? On 2016/01/20 12:11:30, whywhat wrote: > afaik, passing multiple app ids in a single source urn is not supported yet, but > it is something that might be added in the future. OK, removed this TODO https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:71: assertEquals(mChromeMediaRouter.mSinksPerSourcePerProvider.get(SOURCE_ID1).size(), 1); On 2016/01/20 12:11:31, whywhat wrote: > do we have to check all the internal state here? imagine we create new entries > in the maps on the fly when we actually receive sinks, we'd have to update the > tests then You are right, removed these checks now. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:79: mRouteProvider.mDiscoveryCallback.onRouteAddedInternal( On 2016/01/20 12:11:30, whywhat wrote: > I'd rather prefer using Robolectric to fake onRouteAdded/onRouteRemoved > callbacks. See > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jun... > for inspiration :) Working in progress. Haven't completed yet. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:97: testObserveOneSink(); On 2016/01/20 12:11:30, whywhat wrote: > this looks very weird to me... is it a common thing in Chrome tests? Fixed, now we only do these inits without check. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:190: testObserveTwoSinksOfTheSameSource(); On 2016/01/20 12:11:30, whywhat wrote: > I'd say we want test cases to be easily read on their own -- you don't have to > copy all the asserts and verification but just the setup. Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:251: // Remove a non-existing sink. On 2016/01/20 12:11:30, whywhat wrote: > nit: the comment is wrong Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:272: // Remove a non-existing sink. On 2016/01/20 12:11:30, whywhat wrote: > ditto Done. https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:81: assertEquals(mChromeMediaRouter.getRouteProvidersForTest().size(), 1); On 2016/01/20 12:11:31, whywhat wrote: > If you assert these here, you don't need to assert those in the rest of the test > cases. Also, what happens if the assertion fails? Maybe it should just be one > separate test method like testInitialState() ? Or it might be too implementation > specific as well... Removed this assert https://codereview.chromium.org/1593313011/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:90: return c.iterator().next(); On 2016/01/20 12:11:31, whywhat wrote: > this should ideally be somewhere else in the code base :) Done.
https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/CastMediaRouteProvider.java:362: // This method is only implemented in {@link MockMediaRouteProvider}. On 2016/01/21 at 12:28:37, Zhiqiang Zhang wrote: > On 2016/01/20 12:11:30, whywhat wrote: > > Sounds like you could just pass it to the constructor of MockMediaRouteProvider? > > We now cast the provider to the Mock provider, and sets the MRM. This method is no longer in production code. Couldn't we avoid casting by passing the MRM in the ctor? https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:47: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); On 2016/01/21 at 12:28:37, Zhiqiang Zhang wrote: > On 2016/01/20 12:11:30, whywhat wrote: > > Agree. By the time this method is called, discovery delegate should no longer > > need to get updates for this source urn. > > Shouldn't we clear the available sinks for the removed source? Maybe I misunderstood the logic. We should but this already happens in ChromeMediaRouter.stopObservingMediaSinks which calls removeSourceUrn via CastMediaRouterProvider.
PTAL. * Now use robolectric reflection to create mock RouteInfo (using the code of avayvod@'s old patch)
https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:45: public List<MediaRouteProvider> getRouteProvidersForTest() { does it have to be public or could be just package visible? ditto below https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:46: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); As I said, I don't think this is necessary, ChromeMediaRouter does cleanup its cache of the sinks. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:59: assertEquals(mRouteProvider.mRoutes.size(), 2); why testing two routes? isn't one sufficient? maybe have two methods? also, mRouteProvider is a mock, why do we test anything about it??? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:113: SOURCE_ID1, SINK_ID1, PRESENTATION_ID1, ORIGIN1, TAB_ID1, REQUEST_ID1); I don't think you need to verify createRoute and onRouteCreated for the first route anymore. this is covered by the first test Ditto the rest of the tests below. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:132: doAnswer(new Answer() { you don't seem to mock createRoute for join tests yet it still works? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:155: verify(mChromeMediaRouter).onRouteCreated(anyString(), eq(SINK_ID1), eq(REQUEST_ID1), it seems like you're testing the mock a lot here. if you're only testing mChromeMediaRouter you should verify that it calls mRouteProvider.createRoute with matching parameters and then you can call onRouteCreated or onRouteRequestFailed directly on mChromeMediaRouter, w/o the mock at all and just verify the state of the mChromeMediaRouter. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:226: provider.createRouteSuccess( doesn't seem like you need to do this since createRoute will call createRouteSuccess by default, no? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:51: verify(mChromeMediaRouter).onSinksReceived( Again, this is testing the code in your mock. Perhaps, you should test mChromeMediaRouter and then DiscoveryCallback separately? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:252: mRouteProvider.mDiscoveryCallback.removeSourceUrn(SOURCE_ID1); this will never happen in the production code. the source urn is removed when stopObservingMediaSinks is called for the source urn. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:79: ((MockMediaRouteProvider) mRouteProvider).setMediaRouteManager(mChromeMediaRouter); just pass it in the ctor! https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java:44: public static <T> T getFirstElement(Collection<T> c) { hm, is this used? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java:48: public static <K, V> Map.Entry<K, V> getFirstElement(Map<K, V> m) { since you take the key both times it's used, maybe make it a getFirstRoute() on the MockMediaRouteProvider class? https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:24: public MediaRouteManager mManager = null; make it final and initialize in the ctor?
PTAL. Addressed avayvod@'s comments, with notes and replies. * Instead of mocking the provider behavior, now I just verify the provider methods are called with correct arguments, and then directly call ChromeMediaRouter methods to update&check it's state. * Seperated sinks observation tests into two parts, one for ChromeMediaRouter, and one for DiscoveryCallback. * Because this change is big, maybe there are some redundant code now https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/router/ChromeMediaRouter.java:45: public List<MediaRouteProvider> getRouteProvidersForTest() { On 2016/01/21 17:56:45, whywhat wrote: > does it have to be public or could be just package visible? > ditto below Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/media/router/cast/DiscoveryCallback.java:46: mDiscoveryDelegate.onSinksReceived(sourceUrn, new ArrayList<MediaSink>()); On 2016/01/21 17:56:45, whywhat wrote: > As I said, I don't think this is necessary, ChromeMediaRouter does cleanup its > cache of the sinks. Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:59: assertEquals(mRouteProvider.mRoutes.size(), 2); On 2016/01/21 17:56:45, whywhat wrote: > why testing two routes? isn't one sufficient? maybe have two methods? > also, mRouteProvider is a mock, why do we test anything about it??? Split into testCreateOneRoute() and testCreateTwoRoutes(). Removed the asserts of mRouteProvider. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:113: SOURCE_ID1, SINK_ID1, PRESENTATION_ID1, ORIGIN1, TAB_ID1, REQUEST_ID1); On 2016/01/21 17:56:45, whywhat wrote: > I don't think you need to verify createRoute and onRouteCreated for the first > route anymore. this is covered by the first test > Ditto the rest of the tests below. Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:132: doAnswer(new Answer() { On 2016/01/21 17:56:45, whywhat wrote: > you don't seem to mock createRoute for join tests yet it still works? By default, it is mocked as CreateRouteSuccess. BTW, I removed all those mock set-ups for the CreateRouteSuccess and JoinRouteSuccess https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:155: verify(mChromeMediaRouter).onRouteCreated(anyString(), eq(SINK_ID1), eq(REQUEST_ID1), On 2016/01/21 17:56:45, whywhat wrote: > it seems like you're testing the mock a lot here. if you're only testing > mChromeMediaRouter you should verify that it calls mRouteProvider.createRoute > with matching parameters and then you can call onRouteCreated or > onRouteRequestFailed directly on mChromeMediaRouter, w/o the mock at all and > just verify the state of the mChromeMediaRouter. Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:226: provider.createRouteSuccess( On 2016/01/21 17:56:45, whywhat wrote: > doesn't seem like you need to do this since createRoute will call > createRouteSuccess by default, no? Won't fix since we don't have logic in MockMediaRouteProvider.createRoute() any more. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:51: verify(mChromeMediaRouter).onSinksReceived( On 2016/01/21 17:56:46, whywhat wrote: > Again, this is testing the code in your mock. Perhaps, you should test > mChromeMediaRouter and then DiscoveryCallback separately? Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:252: mRouteProvider.mDiscoveryCallback.removeSourceUrn(SOURCE_ID1); On 2016/01/21 17:56:46, whywhat wrote: > this will never happen in the production code. the source urn is removed when > stopObservingMediaSinks is called for the source urn. Now I still keep this test, but I just verify that it the callback does not notify mChromeMediaRouter https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:79: ((MockMediaRouteProvider) mRouteProvider).setMediaRouteManager(mChromeMediaRouter); On 2016/01/21 17:56:46, whywhat wrote: > just pass it in the ctor! Done. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java:44: public static <T> T getFirstElement(Collection<T> c) { On 2016/01/21 17:56:46, whywhat wrote: > hm, is this used? removed. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MediaRouterTestUtils.java:48: public static <K, V> Map.Entry<K, V> getFirstElement(Map<K, V> m) { On 2016/01/21 17:56:46, whywhat wrote: > since you take the key both times it's used, maybe make it a getFirstRoute() on > the MockMediaRouteProvider class? removed. https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:24: public MediaRouteManager mManager = null; On 2016/01/21 17:56:46, whywhat wrote: > make it final and initialize in the ctor? Done.
Looking much better now! Left some comments, mostly about removing more code. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:26: public void setUp() { remove this overload https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:38: super.setUp(); remove https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:113: public void testInitCallbackWithEmptyKnownSinks() { this seems to belong to a separate DiscoveryCallbackTest class https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:24: protected static final String TAG = "MediaRouter"; is this used? remove https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:15: public class MockMediaRouteProvider implements MediaRouteProvider, DiscoveryDelegate { seems like you can just create a mock(MediaRouteProvider) now, no? https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:17: private static final String TAG = "MediaRouter"; ditto https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:23: return MediaSource.from(sourceId) != null; doesn't seem like it's being used now? https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:62: mManager.onSinksReceived(sourceId, this, sinks); not being used now it seems?
PTAL. * addressed avayvod@'s comments. * moved tests for DiscoveryCallback into a separate class * mv MediaRouterTestUtils.java cast/TestUtils.java (seems to be a more appropriate place) https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterRouteTest.java:26: public void setUp() { On 2016/01/25 11:50:10, whywhat wrote: > remove this overload Done. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:38: super.setUp(); On 2016/01/25 11:50:10, whywhat wrote: > remove Done. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java:113: public void testInitCallbackWithEmptyKnownSinks() { On 2016/01/25 11:50:10, whywhat wrote: > this seems to belong to a separate DiscoveryCallbackTest class Done. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java:24: protected static final String TAG = "MediaRouter"; On 2016/01/25 11:50:11, whywhat wrote: > is this used? remove Done. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java (right): https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:15: public class MockMediaRouteProvider implements MediaRouteProvider, DiscoveryDelegate { On 2016/01/25 11:50:11, whywhat wrote: > seems like you can just create a mock(MediaRouteProvider) now, no? Yes, removed this mock class. Now I just use mock(MediaRouteProvider.class) and mock(DiscoveryDelegate.class) https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:17: private static final String TAG = "MediaRouter"; On 2016/01/25 11:50:11, whywhat wrote: > ditto Won't fix since removed. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:23: return MediaSource.from(sourceId) != null; On 2016/01/25 11:50:11, whywhat wrote: > doesn't seem like it's being used now? won't fix since removed. https://codereview.chromium.org/1593313011/diff/120001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/media/router/MockMediaRouteProvider.java:62: mManager.onSinksReceived(sourceId, this, sinks); On 2016/01/25 11:50:11, whywhat wrote: > not being used now it seems? won't fix since removed.
lgtm https://codereview.chromium.org/1593313011/diff/140001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1593313011/diff/140001/chrome/android/BUILD.g... chrome/android/BUILD.gn:273: "junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java", Don't you need the corresponding GYP change? Or is it a wildcard including all the files?
zqzhang@chromium.org changed reviewers: + miguelg@chromium.org
+miguelg@ for BUILD.gn, with the following replies to avayvod@ https://codereview.chromium.org/1593313011/diff/140001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/1593313011/diff/140001/chrome/android/BUILD.g... chrome/android/BUILD.gn:273: "junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterSinkObservationTest.java", On 2016/01/25 14:34:34, whywhat wrote: > Don't you need the corresponding GYP change? Or is it a wildcard including all > the files? The GYP file use "src_paths" which includes all source files in android/junit, which is like a wildcard.
chrome/android/BUILD.gn lgtm
The CQ bit was checked by zqzhang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1593313011/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1593313011/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Adding unittests for media casting sink observation and route handling This patch adds a mock MediaRouteProvider for unittest needs, as well as unittests for sink observation and route handling. BUG=578835 ========== to ========== Adding unittests for media casting sink observation and route handling This patch adds a mock MediaRouteProvider for unittest needs, as well as unittests for sink observation and route handling. BUG=578835 Committed: https://crrev.com/d20c259e2aabe9fbed12b1991c02e04eb431f1a8 Cr-Commit-Position: refs/heads/master@{#371247} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d20c259e2aabe9fbed12b1991c02e04eb431f1a8 Cr-Commit-Position: refs/heads/master@{#371247} |
