Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2816603002:
Adding JUnit tests for MediaNotificationManager (Closed)
|
Created:
3 years, 8 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 8 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, feature-media-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding JUnit tests for MediaNotificationManager
This CL adds a number of JUnit tests for testing
MediaNotificationManager in the following aspects:
* Check if MediaNotificationManager handles the ListenerService correctly.
* Check if ListenerService can correctly process Intents.
* Check if the Notification presrented to Android NotificationManager is
correct.
Many MediaNotificationManager members are changed to package scope access
to avoid too many "ForTesting" names.
BUG=705919
Review-Url: https://codereview.chromium.org/2816603002
Cr-Commit-Position: refs/heads/master@{#464375}
Committed: https://chromium.googlesource.com/chromium/src/+/a98c2132ee5335e39d6d83f70ed37678237637e9
Patch Set 1 #Patch Set 2 : nits #Patch Set 3 : added missing file #
Total comments: 57
Patch Set 4 : addressed avayvod's comments #Patch Set 5 : addressed nits #Patch Set 6 : rebased #Patch Set 7 : rebased #Messages
Total messages: 34 (24 generated)
Description was changed from ========== Adding JUnit tests for MediaNotificationManager This CL adds a number of JUnit tests for testing MediaNotificationManager in the following aspects: * Check if MediaNotificationManager handles the ListenerService correctly. * Check if ListenerService can correctly process Intents. * Check if the Notification presrented to Android NotificationManager is correct. BUG=705919 ========== to ========== Adding JUnit tests for MediaNotificationManager This CL adds a number of JUnit tests for testing MediaNotificationManager in the following aspects: * Check if MediaNotificationManager handles the ListenerService correctly. * Check if ListenerService can correctly process Intents. * Check if the Notification presrented to Android NotificationManager is correct. Many MediaNotificationManager members are changed to package scope access to avoid too many "ForTesting" names. BUG=705919 ==========
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
PTAL
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:84: static Boolean sOverrideNForTesting; nit: the name is not very easy to read, maybe sOverrideIsRunningNForTesting? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:89: static boolean sDisableSdkIntCheckForTesting; I would just remove the assertion, tbh. I don't think it's of any use. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:231: if (!processIntent(intent)) stopSelfForTesting(); nit: I hope you meant to leave stopSelf() here? I don't think any ForTesting code should ever be called in production code, at least for clarity. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:241: stopSelf(); nit: Just make stopSelf() package visible? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:315: stopSelfForTesting(); ditto. I think according to the docs stopSelf is a public method so I'm not sure why stopSelfForTesting even needed. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:726: // TODO(zqzhang): merge this call to the if statement above? nit: how do you mean to merge it? Could you add more context, please? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:811: // TODO(zqzhang): It's weird that setShowWhen() don't work on K. Calling setWhen() to force nit: s/don't/doesn't Do you know if there's an Android bug filed? If it's in the support library it could be fixed eventually. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:930: // TODO(zqzhang): this should be wrong. nit: more context/clarity in the comment please? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:38: public class MediaNotificationManagerAndroidNotificationTest nit: remove Android from the name? unless we have another platform we write in Java for :) https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:43: super.setUp(); nit: Hm, if all this does is call the parent's method, it shouldn't be written? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:50: // Not testing against pre-N due to Robolectric restrictions. nit: clarify comment? not testing what? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:57: // This is the fake implementation to ensure |mMediaSession| is non-null. why do you need a non null mMediaSession? updateNotificationBuilder() doesn't use it as far as I can see, neither does builder.build(). https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:66: assertEquals(info.metadata.getTitle(), shadowNotification.getContentTitle()); nit: I believe it'd be easier to read if it was the actual string: assertEquals("title", shadowNotification.getContentTitle()); assertEquals("artist - album", shadowNotification.getContentText()); assertEquals("https://example.com", shadowNotification.getSubText()); https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:70: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); nit: we're testing pre-N here, no? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:78: // Not testing against pre-N due to Robolectric restrictions. nit: ditto - seems like you are not testing N+ for non-empty artist and album - maybe just put a comment before the test cases explaining that and why and having a TODO with a bug for when we will be able to test that? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:88: getManager().updateNotificationBuilder(); the code in these three test cases seems to repeat the same template - can it be extracted into a private method of this or base class? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:97: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); can you enforce this via some annotation? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:102: public void updateNotificationBuilderDisplaysCorrectMetadata_PreN_EmptyArtistAndAlbum() { nit: I'd expect this be next to the other preN test https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:45: * JUnit tests for checking MediaNotificationManager handles the listener service correctly. nit: expand the comment a bit to make it easier to read? For example "Tests for checking that {@link MediaNotificationManager} handles the listener service life cycle correctly"? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:54: public void setUp() { nit: remove? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:126: public void testProcessMediaButtonAction() { nit: so you created a separate test case for each edge case for service start but then combine all actions into a huge test case of 50+ lines of code. could you split this one into separate ones too? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:223: // Called in |setUpService()|. you could perhaps reset mock expectations or something after setUpService() to "forget" about it and make the verify statement below use never() to be more explicit? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerTestBase.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerTestBase.java:147: void startService(Intent intent) { nit: this and other methods don't appear to be used by the derived classes? mark them private? also if something is only used by the service test (the notification one doesn't appear to use much), it should only be in the service test unless it makes this base class to complex. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestShadowNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestShadowNotificationManager.java:5: package org.chromium.chrome.browser.media.ui; nit: this and the class below don't seem to have anything media specific? can these be reused for other notification tests?
https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:84: static Boolean sOverrideNForTesting; On 2017/04/11 at 23:57:43, whywhat wrote: > nit: the name is not very easy to read, maybe sOverrideIsRunningNForTesting? Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:89: static boolean sDisableSdkIntCheckForTesting; On 2017/04/11 at 23:57:43, whywhat wrote: > I would just remove the assertion, tbh. I don't think it's of any use. Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:231: if (!processIntent(intent)) stopSelfForTesting(); On 2017/04/11 at 23:57:43, whywhat wrote: > nit: I hope you meant to leave stopSelf() here? I don't think any ForTesting code should ever be called in production code, at least for clarity. No, Service.stopSelf() is final and I cannot override it. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:241: stopSelf(); On 2017/04/11 at 23:57:43, whywhat wrote: > nit: Just make stopSelf() package visible? (See the reply above). https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:315: stopSelfForTesting(); On 2017/04/11 at 23:57:43, whywhat wrote: > ditto. I think according to the docs stopSelf is a public method so I'm not sure why stopSelfForTesting even needed. (See reply above) https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:726: // TODO(zqzhang): merge this call to the if statement above? On 2017/04/11 at 23:57:43, whywhat wrote: > nit: how do you mean to merge it? Could you add more context, please? I mean to move updateNotification() above to the else branch, and remove the startService() call. For the if-branch, it will start the service, and the service will call updateNotification() when it's started. For the else-branch, calling startService() will be no-op as the service has already started. Do you prefer I do it in this patch or a follow-up? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:811: // TODO(zqzhang): It's weird that setShowWhen() don't work on K. Calling setWhen() to force On 2017/04/11 at 23:57:43, whywhat wrote: > nit: s/don't/doesn't > Do you know if there's an Android bug filed? If it's in the support library it could be fixed eventually. Don't know yet. I'll file one. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:930: // TODO(zqzhang): this should be wrong. On 2017/04/11 at 23:57:43, whywhat wrote: > nit: more context/clarity in the comment please? Done. I mean on pre-N, the notification looks bad when the large icon is not set. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:38: public class MediaNotificationManagerAndroidNotificationTest On 2017/04/11 at 23:57:43, whywhat wrote: > nit: remove Android from the name? unless we have another platform we write in Java for :) Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:43: super.setUp(); On 2017/04/11 at 23:57:43, whywhat wrote: > nit: Hm, if all this does is call the parent's method, it shouldn't be written? Ah, just realized that JUnit runs the @Before methods of all superclasses before the child class. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:50: // Not testing against pre-N due to Robolectric restrictions. On 2017/04/11 at 23:57:43, whywhat wrote: > nit: clarify comment? not testing what? This no longer applies, as we have |sOverrideIsRunningNForTesting|. Removed this comment. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:57: // This is the fake implementation to ensure |mMediaSession| is non-null. On 2017/04/11 at 23:57:43, whywhat wrote: > why do you need a non null mMediaSession? updateNotificationBuilder() doesn't use it as far as I can see, neither does builder.build(). Actually it is called in addNotificationButtons(). setMediaStyle() uses |mMediaSession|. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:66: assertEquals(info.metadata.getTitle(), shadowNotification.getContentTitle()); On 2017/04/11 at 23:57:43, whywhat wrote: > nit: I believe it'd be easier to read if it was the actual string: > > assertEquals("title", shadowNotification.getContentTitle()); > assertEquals("artist - album", shadowNotification.getContentText()); > assertEquals("https://example.com", shadowNotification.getSubText()); Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:70: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); On 2017/04/11 at 23:57:43, whywhat wrote: > nit: we're testing pre-N here, no? Yes, we are "assuming" pre-N in MediaNotificationManager logic. However, we need to access the notification subtext using N APIs (because it is not implemented in ShadowNotification). https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:78: // Not testing against pre-N due to Robolectric restrictions. On 2017/04/11 at 23:57:43, whywhat wrote: > nit: ditto - seems like you are not testing N+ for non-empty artist and album - maybe just put a comment before the test cases explaining that and why and having a TODO with a bug for when we will be able to test that? Removed comments. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:88: getManager().updateNotificationBuilder(); On 2017/04/11 at 23:57:43, whywhat wrote: > the code in these three test cases seems to repeat the same template - can it be extracted into a private method of this or base class? Moved to a utility function called "updateNotificationBuilderAndBuild()" https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:97: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); On 2017/04/11 at 23:57:43, whywhat wrote: > can you enforce this via some annotation? There is a @RequiresApi in support library v25 but we haven't upgraded it yet. Now temporarily using a test to check the API level. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:102: public void updateNotificationBuilderDisplaysCorrectMetadata_PreN_EmptyArtistAndAlbum() { On 2017/04/11 at 23:57:43, whywhat wrote: > nit: I'd expect this be next to the other preN test Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:45: * JUnit tests for checking MediaNotificationManager handles the listener service correctly. On 2017/04/11 at 23:57:44, whywhat wrote: > nit: expand the comment a bit to make it easier to read? > For example "Tests for checking that {@link MediaNotificationManager} handles the listener service life cycle correctly"? Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:54: public void setUp() { On 2017/04/11 at 23:57:44, whywhat wrote: > nit: remove? Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:126: public void testProcessMediaButtonAction() { On 2017/04/11 at 23:57:44, whywhat wrote: > nit: so you created a separate test case for each edge case for service start but then combine all actions into a huge test case of 50+ lines of code. could you split this one into separate ones too? Done. Also I split them into a separate test class. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:223: // Called in |setUpService()|. On 2017/04/11 at 23:57:44, whywhat wrote: > you could perhaps reset mock expectations or something after setUpService() to "forget" about it and make the verify statement below use never() to be more explicit? There is Mockito.clearInvocations(). However the Mockito document says "avoid at all costs": https://static.javadoc.io/org.mockito/mockito-core/2.7.22/org/mockito/Mockito... But I agree with you this could be a reasonable case for using it. Shall we change it? https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerTestBase.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerTestBase.java:147: void startService(Intent intent) { On 2017/04/11 at 23:57:44, whywhat wrote: > nit: this and other methods don't appear to be used by the derived classes? mark them private? Done > also if something is only used by the service test (the notification one doesn't appear to use much), it should only be in the service test unless it makes this base class to complex. Some non-service tests actually require the service to be set up. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestShadowNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationTestShadowNotificationManager.java:5: package org.chromium.chrome.browser.media.ui; On 2017/04/11 at 23:57:44, whywhat wrote: > nit: this and the class below don't seem to have anything media specific? can these be reused for other notification tests? Maybe not at the moment. We are customizing the shadow for the purpose of testing MediaNotificationManager. For other purposes, the shadow could be different.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:231: if (!processIntent(intent)) stopSelfForTesting(); On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > On 2017/04/11 at 23:57:43, whywhat wrote: > > nit: I hope you meant to leave stopSelf() here? I don't think any ForTesting code should ever be called in production code, at least for clarity. > > No, Service.stopSelf() is final and I cannot override it. Can you at least use stopSelf() in the non-test code? Or name it something like stopSelfInternal()? Calling xxxForTesting() in non-test code looks very weird :) I wish we could use Mockito 2: https://github.com/mockito/mockito/wiki/What's-new-in-Mockito-2#mock-the-unmo... https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:726: // TODO(zqzhang): merge this call to the if statement above? On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > On 2017/04/11 at 23:57:43, whywhat wrote: > > nit: how do you mean to merge it? Could you add more context, please? > > I mean to move updateNotification() above to the else branch, and remove the startService() call. > > For the if-branch, it will start the service, and the service will call updateNotification() when it's started. > For the else-branch, calling startService() will be no-op as the service has already started. > > Do you prefer I do it in this patch or a follow-up? A follow up is fine. Just curious :) https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:57: // This is the fake implementation to ensure |mMediaSession| is non-null. On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > On 2017/04/11 at 23:57:43, whywhat wrote: > > why do you need a non null mMediaSession? updateNotificationBuilder() doesn't use it as far as I can see, neither does builder.build(). > > Actually it is called in addNotificationButtons(). setMediaStyle() uses |mMediaSession|. Oh, do you think we could avoid this dependency somehow (in the future)? Not in this CL though. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:70: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > On 2017/04/11 at 23:57:43, whywhat wrote: > > nit: we're testing pre-N here, no? > > Yes, we are "assuming" pre-N in MediaNotificationManager logic. However, we need to access the notification subtext using N APIs (because it is not implemented in ShadowNotification). Can the tests be run somehow that the N APIs won't be available? I'd rather have something like: // Can only check subtext if built against N SDK if (have N APIs) { assert subtext is correct } Although then the test won't fail if only run in M- environments ... https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:223: // Called in |setUpService()|. On 2017/04/12 at 14:45:11, Zhiqiang Zhang wrote: > On 2017/04/11 at 23:57:44, whywhat wrote: > > you could perhaps reset mock expectations or something after setUpService() to "forget" about it and make the verify statement below use never() to be more explicit? > > There is Mockito.clearInvocations(). However the Mockito document says "avoid at all costs": https://static.javadoc.io/org.mockito/mockito-core/2.7.22/org/mockito/Mockito... > > But I agree with you this could be a reasonable case for using it. Shall we change it? I don't mind :) As long as we do it right after the setup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
zqzhang@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor: AppHooks.java https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/media/ui/MediaNotificationManager.java:231: if (!processIntent(intent)) stopSelfForTesting(); On 2017/04/12 at 15:00:40, whywhat wrote: > On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > > On 2017/04/11 at 23:57:43, whywhat wrote: > > > nit: I hope you meant to leave stopSelf() here? I don't think any ForTesting code should ever be called in production code, at least for clarity. > > > > No, Service.stopSelf() is final and I cannot override it. > > Can you at least use stopSelf() in the non-test code? > Or name it something like stopSelfInternal()? > > Calling xxxForTesting() in non-test code looks very weird :) > > I wish we could use Mockito 2: https://github.com/mockito/mockito/wiki/What's-new-in-Mockito-2#mock-the-unmo... Renamed as "stopListenerService()". "stopSelfInternal()" is improper as it's actually a wrapper function. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:57: // This is the fake implementation to ensure |mMediaSession| is non-null. On 2017/04/12 at 15:00:40, whywhat wrote: > On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > > On 2017/04/11 at 23:57:43, whywhat wrote: > > > why do you need a non null mMediaSession? updateNotificationBuilder() doesn't use it as far as I can see, neither does builder.build(). > > > > Actually it is called in addNotificationButtons(). setMediaStyle() uses |mMediaSession|. > > Oh, do you think we could avoid this dependency somehow (in the future)? Not in this CL though. Yes, it's possible. We can move statements around to achieve this. Added TODO https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerAndroidNotificationTest.java:70: assertTrue(RuntimeEnvironment.getApiLevel() >= Build.VERSION_CODES.N); On 2017/04/12 at 15:00:40, whywhat wrote: > On 2017/04/12 at 14:45:10, Zhiqiang Zhang wrote: > > On 2017/04/11 at 23:57:43, whywhat wrote: > > > nit: we're testing pre-N here, no? > > > > Yes, we are "assuming" pre-N in MediaNotificationManager logic. However, we need to access the notification subtext using N APIs (because it is not implemented in ShadowNotification). > > Can the tests be run somehow that the N APIs won't be available? > I'd rather have something like: > > // Can only check subtext if built against N SDK > if (have N APIs) { > assert subtext is correct > } > > Although then the test won't fail if only run in M- environments ... Done. https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java (right): https://codereview.chromium.org/2816603002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/media/ui/MediaNotificationManagerServiceTest.java:223: // Called in |setUpService()|. On 2017/04/12 at 15:00:40, whywhat wrote: > On 2017/04/12 at 14:45:11, Zhiqiang Zhang wrote: > > On 2017/04/11 at 23:57:44, whywhat wrote: > > > you could perhaps reset mock expectations or something after setUpService() to "forget" about it and make the verify statement below use never() to be more explicit? > > > > There is Mockito.clearInvocations(). However the Mockito document says "avoid at all costs": https://static.javadoc.io/org.mockito/mockito-core/2.7.22/org/mockito/Mockito... > > > > But I agree with you this could be a reasonable case for using it. Shall we change it? > > I don't mind :) As long as we do it right after the setup. Done. Added method setUpServiceAndClearInvocations().
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
apphooks lgtm. I've heard we can use Shadows to mock statics, but I haven't them yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
drive-by lgtm, very excited \o/ :)
On 2017/04/12 at 21:01:49, dtrainor wrote: > apphooks lgtm. I've heard we can use Shadows to mock statics, but I haven't them yet. Yeah, I did some search on mocking static methods. It seems like we need PowerMock on top of Mockito to make it work. However we don't have PowerMock yet.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, dtrainor@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2816603002/#ps120001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1492080392540460,
"parent_rev": "d4bcb119316cfccf5c1ddcebe3b6d65a7fec09e4", "commit_rev":
"a98c2132ee5335e39d6d83f70ed37678237637e9"}
Message was sent while issue was closed.
Description was changed from ========== Adding JUnit tests for MediaNotificationManager This CL adds a number of JUnit tests for testing MediaNotificationManager in the following aspects: * Check if MediaNotificationManager handles the ListenerService correctly. * Check if ListenerService can correctly process Intents. * Check if the Notification presrented to Android NotificationManager is correct. Many MediaNotificationManager members are changed to package scope access to avoid too many "ForTesting" names. BUG=705919 ========== to ========== Adding JUnit tests for MediaNotificationManager This CL adds a number of JUnit tests for testing MediaNotificationManager in the following aspects: * Check if MediaNotificationManager handles the ListenerService correctly. * Check if ListenerService can correctly process Intents. * Check if the Notification presrented to Android NotificationManager is correct. Many MediaNotificationManager members are changed to package scope access to avoid too many "ForTesting" names. BUG=705919 Review-Url: https://codereview.chromium.org/2816603002 Cr-Commit-Position: refs/heads/master@{#464375} Committed: https://chromium.googlesource.com/chromium/src/+/a98c2132ee5335e39d6d83f70ed3... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a98c2132ee5335e39d6d83f70ed3... |
