Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1025403002:
[Sync] PassphraseActivity: unregister for sync events when paused. (Closed)
|
Created:
5 years, 9 months ago by maxbogue Modified:
5 years, 8 months ago CC:
chromium-reviews, maniscalco+watch_chromium.org, maxbogue+watch_chromium.org, nyquist, plaree+watch_chromium.org, pvalenzuela+watch_chromium.org, tim+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] PassphraseActivity: unregister for sync events when paused.
TEST=For a manual test to exercise this code, see comment #2 in the bug.
BUG=469890
Committed: https://crrev.com/cf0c3c3522ed57d4922b0eb6e9d57ef4cf0cbeb8
Cr-Commit-Position: refs/heads/master@{#324316}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add a non-working test. #Patch Set 3 : Remove non-working test. #Patch Set 4 : Now with WORKING test. #Patch Set 5 : Try moving ChromeActivityTestBase to shell. #
Total comments: 16
Patch Set 6 : Address comments. #
Total comments: 7
Patch Set 7 : Address more comments + fix assert failure. #Patch Set 8 : Remove ChromeActivityTestBase and rename FakeProfileSyncService. #
Messages
Total messages: 27 (4 generated)
maxbogue@chromium.org changed reviewers: + maniscalco@chromium.org
PTAL!
How about the tests? Can this change be tested manually (TEST= line). How do we normally unit test android Activities? I'm looking at ChromeShellTestBase and wondering if you could use that to create a simple unit test for PassphraseActivity that repros the bug and verifies this fix. WDYT? https://codereview.chromium.org/1025403002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java (right): https://codereview.chromium.org/1025403002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java:69: addSyncStateChangedListener(); Here we add the SyncStateChangedListener. I'm thinking about cases where PhasepharaseActivity is resumed, but doesn't have a SSCL. If onResume is called and we don't reach this line (e.g. because we are showing the FRAGMENT_PASSWORD dialog) is it OK that we don't add the SSCL? I'm trying to wrap my head around when we should be listening for sync state changes and when we shouldn't.
Can't be manually tested AFAIK. I don't know how to trigger the activity if sync isn't initialized, so I think it's a rare race condition. I'll look into a test. https://codereview.chromium.org/1025403002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java (right): https://codereview.chromium.org/1025403002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java:69: addSyncStateChangedListener(); On 2015/03/24 at 16:07:35, maniscalco wrote: > Here we add the SyncStateChangedListener. I'm thinking about cases where PhasepharaseActivity is resumed, but doesn't have a SSCL. If onResume is called and we don't reach this line (e.g. because we are showing the FRAGMENT_PASSWORD dialog) is it OK that we don't add the SSCL? I'm trying to wrap my head around when we should be listening for sync state changes and when we shouldn't. If sync is not initialized it registers the SSCL and waits until it is so it can display the password dialog (which requires the backend). I'm not entirely sure why/how this activity would be started if sync was not already initialized; possibly it has something to do with being launched from a notification?
PTAL. I am punting on writing a test for this since it's been over a week and I'm still fighting Android crap trying to start an activity. Also relatively speaking this test is very low-value, so I'd like to spend my test-struggling time on more important ones.
On 2015/04/03 22:51:23, maxbogue wrote: > PTAL. I am punting on writing a test for this since it's been over a week and > I'm still fighting Android crap trying to start an activity. Also relatively > speaking this test is very low-value, so I'd like to spend my test-struggling > time on more important ones. Bummer. Next week let's talk about what kind of hurdles you ran into while trying to develop an unit test. I want to make sure we prioritize fixing any test framework problems/limitations that make it difficult to write unit tests. Putting automated testing aside for a minute. I understand that it may be difficult to repro the IllegalStateException mentioned in the bug report, but it seems like you should be able to at least trigger the new code manually, see that it behaves as expected, and see that no unexpected errors are logged. Maybe this is naive but could do something like this: 1. Trigger creation of the PassphraseActivity by visit sync settings, the account, then "Encryption". 2. Once the activity is displayed you'd then want to trigger onPause. Would pressing the home button cause PassphraseActivity's onPause to be invoked? 3. Return to the activity and continue entering a passphrase. See that syncing works as expected.
On 2015/04/03 23:56:51, maniscalco wrote: > On 2015/04/03 22:51:23, maxbogue wrote: > > PTAL. I am punting on writing a test for this since it's been over a week and > > I'm still fighting Android crap trying to start an activity. Also relatively > > speaking this test is very low-value, so I'd like to spend my test-struggling > > time on more important ones. > > Bummer. Next week let's talk about what kind of hurdles you ran into while > trying to develop an unit test. I want to make sure we prioritize fixing any > test framework problems/limitations that make it difficult to write unit tests. PTAL, I finally got a test working yesterday! > Putting automated testing aside for a minute. I understand that it may be > difficult to repro the IllegalStateException mentioned in the bug report, but it > seems like you should be able to at least trigger the new code manually, see > that it behaves as expected, and see that no unexpected errors are logged. > Maybe this is naive but could do something like this: > > 1. Trigger creation of the PassphraseActivity by visit sync settings, the > account, then "Encryption". > > 2. Once the activity is displayed you'd then want to trigger onPause. Would > pressing the home button cause PassphraseActivity's onPause to be invoked? > > 3. Return to the activity and continue entering a passphrase. See that syncing > works as expected. I didn't realize you were asking for a "hit the code path" test vs a "check the fix" test. The steps for that are: 1. Sign into an account with a passphrase. 2. Tap the generated notification (important: going through the settings page does not use PassphraseActivity). 3. Hit home to background the activity, then switch back and verify nothing crashed. Am I supposed to include those steps in a TEST= line somehow?
maxbogue@chromium.org changed reviewers: + pvalenzuela@chromium.org
Patrick, please take a look at the new testing code!
Thanks for investing the time in this work. This is an investment that will benefit us down the road. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:86: public static void overrideForTests(ProfileSyncService pss) { IMO calling this param "profileSyncService" (more verbose) would fit a little better with the Java style, but it's your call https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:87: sSyncSetupManager = pss; could you add a TODO at sSyncSetupManager's declaration to rename it? https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java:78: removeSyncStateChangedListener(); Add a comment explaining why this is here. (It's so that we don't process sync changes in the background, right?) https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java:12: public class ProfileSyncServiceMock extends ProfileSyncService { Some folks might not agree with the usage of the word mock in the name. See http://martinfowler.com/articles/mocksArentStubs.html for a pretty good summary. What are your thoughts here? IMO this is not a mock (more of a stub). https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java:31: return true; Why is this always true and not mutable by tests? My guess is that's the value we want for the PassphraseActivity tests. If so, add a comment explaining that. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:78: pss.syncStateChanged(); Is there some sort of state we can assert on here? If so, do that. If not, just mention that if the test method reaches this point without crashing, it's a success. :-) https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:88: return (ProfileSyncServiceMock) ProfileSyncService.get(context); Why make an extra call to PSS.get()? How about just initializing ProfileSyncServiceMock at the start of the method and return it here? https://codereview.chromium.org/1025403002/diff/80001/chrome/android/shell/ja... File chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/shell/ja... chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java:24: public class ChromeActivityTestBase<T extends Activity> I'm unsure if we want to introduce this additional base test class. I defer to an expert like Tommy or Yaron on this one. I believe you'll also need their approval to submit this.
https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:86: public static void overrideForTests(ProfileSyncService pss) { On 2015/04/07 19:55:51, pvalenzuela wrote: > IMO calling this param "profileSyncService" (more verbose) would fit a little > better with the Java style, but it's your call Done. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:87: sSyncSetupManager = pss; On 2015/04/07 19:55:51, pvalenzuela wrote: > could you add a TODO at sSyncSetupManager's declaration to rename it? Just did the change instead; it's only a few lines. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/sync/ui/PassphraseActivity.java:78: removeSyncStateChangedListener(); On 2015/04/07 19:55:51, pvalenzuela wrote: > Add a comment explaining why this is here. (It's so that we don't process sync > changes in the background, right?) Done. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java:12: public class ProfileSyncServiceMock extends ProfileSyncService { On 2015/04/07 19:55:51, pvalenzuela wrote: > Some folks might not agree with the usage of the word mock in the name. See > http://martinfowler.com/articles/mocksArentStubs.html for a pretty good summary. > > What are your thoughts here? IMO this is not a mock (more of a stub). Based on that article it seems like halfway between a stub and a fake. What do you think it should be? ProfileSyncServiceStub? https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ProfileSyncServiceMock.java:31: return true; On 2015/04/07 19:55:51, pvalenzuela wrote: > Why is this always true and not mutable by tests? My guess is that's the value > we want for the PassphraseActivity tests. If so, add a comment explaining that. It's because the value returned doesn't matter for my test, but the backend can't start without a server so the original call will crash. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:78: pss.syncStateChanged(); On 2015/04/07 19:55:51, pvalenzuela wrote: > Is there some sort of state we can assert on here? If so, do that. If not, just > mention that if the test method reaches this point without crashing, it's a > success. :-) Done. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:88: return (ProfileSyncServiceMock) ProfileSyncService.get(context); On 2015/04/07 19:55:51, pvalenzuela wrote: > Why make an extra call to PSS.get()? How about just initializing > ProfileSyncServiceMock at the start of the method and return it here? Can't. It has to be constructed on the UI thread. Added a comment. https://codereview.chromium.org/1025403002/diff/80001/chrome/android/shell/ja... File chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java (right): https://codereview.chromium.org/1025403002/diff/80001/chrome/android/shell/ja... chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java:24: public class ChromeActivityTestBase<T extends Activity> On 2015/04/07 19:55:51, pvalenzuela wrote: > I'm unsure if we want to introduce this additional base test class. > > I defer to an expert like Tommy or Yaron on this one. I believe you'll also need > their approval to submit this. It seemed like a better idea than copy/pasting the below code. I'll let one of them weigh in.
yfriedman@chromium.org changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... File chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java:24: public class ChromeActivityTestBase<T extends Activity> I'm not a big fan of this split (or at least the name) - simply because ChromeActivity isn't upstreamed and this causes the downstream one to shadow it.
On 2015/04/07 18:30:02, maxbogue wrote: > On 2015/04/03 23:56:51, maniscalco wrote: > > On 2015/04/03 22:51:23, maxbogue wrote: > > > PTAL. I am punting on writing a test for this since it's been over a week > and > > > I'm still fighting Android crap trying to start an activity. Also relatively > > > speaking this test is very low-value, so I'd like to spend my > test-struggling > > > time on more important ones. > > > > Bummer. Next week let's talk about what kind of hurdles you ran into while > > trying to develop an unit test. I want to make sure we prioritize fixing any > > test framework problems/limitations that make it difficult to write unit > tests. > > PTAL, I finally got a test working yesterday! Great! > > Putting automated testing aside for a minute. I understand that it may be > > difficult to repro the IllegalStateException mentioned in the bug report, but > it > > seems like you should be able to at least trigger the new code manually, see > > that it behaves as expected, and see that no unexpected errors are logged. > > Maybe this is naive but could do something like this: > > > > 1. Trigger creation of the PassphraseActivity by visit sync settings, the > > account, then "Encryption". > > > > 2. Once the activity is displayed you'd then want to trigger onPause. Would > > pressing the home button cause PassphraseActivity's onPause to be invoked? > > > > 3. Return to the activity and continue entering a passphrase. See that > syncing > > works as expected. > > I didn't realize you were asking for a "hit the code path" test vs a "check the > fix" > test. The steps for that are: > > 1. Sign into an account with a passphrase. > 2. Tap the generated notification (important: going through the settings page > does > not use PassphraseActivity). > 3. Hit home to background the activity, then switch back and verify nothing > crashed. > > Am I supposed to include those steps in a TEST= line somehow? Regarding the TEST= line, I suggest putting comment in the linked bug that describes manual steps to repro the issue (like the 1-3 you just mentioned). Then in the TEST= line I'd put: TEST=See comment #XX of linked bug. where XX is the comment number on the bug where you described the repro/test steps.
https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... File chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java:24: public class ChromeActivityTestBase<T extends Activity> On 2015/04/07 21:41:30, Yaron wrote: > I'm not a big fan of this split (or at least the name) - simply because > ChromeActivity isn't upstreamed and this causes the downstream one to shadow it. Hmm. Perhaps the simplest way is to just have ChromeShellTestBase as the parent, start the browser (startChromeBrowserProcessSync) and then add a new helper method which takes an intent/activity to start. I'm not sure if that still works but worth a try. Sorry for the rough edges - it looks like with the current state of upstreaming, we're in an odd place where ChromeShell mostly supports one activity at the moment.
https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:23: public class PassphraseActivityTest extends ChromeActivityTestBase<PassphraseActivity> { Really glad to see a test for this bug! https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:78: pss.setSyncInitialized(true); Since ProfileSyncService lives on the UI thread and this test doesn't execute on the UI thread, we should be careful to ensure we don't call methods on pss here. Can you move the calls on pss inside the runOnUiThreadBlocking call above? That way we're not accessing pss from anything other than the UI thread.
https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:78: pss.setSyncInitialized(true); On 2015/04/07 22:22:11, maniscalco wrote: > Since ProfileSyncService lives on the UI thread and this test doesn't execute on > the UI thread, we should be careful to ensure we don't call methods on pss here. > Can you move the calls on pss inside the runOnUiThreadBlocking call above? > That way we're not accessing pss from anything other than the UI thread. Done. Turns out that PSS.get() at least DOES have an assert, so I had to move that here too. I'm not sure why the assert triggered on the trybot but not on my local run :/ https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... File chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/shell/j... chrome/android/shell/javatests/src/org/chromium/chrome/shell/ChromeActivityTestBase.java:24: public class ChromeActivityTestBase<T extends Activity> On 2015/04/07 21:47:52, Yaron wrote: > On 2015/04/07 21:41:30, Yaron wrote: > > I'm not a big fan of this split (or at least the name) - simply because > > ChromeActivity isn't upstreamed and this causes the downstream one to shadow > it. > > Hmm. Perhaps the simplest way is to just have ChromeShellTestBase as the parent, > start the browser (startChromeBrowserProcessSync) and then add a new helper > method which takes an intent/activity to start. I'm not sure if that still works > but worth a try. > > Sorry for the rough edges - it looks like with the current state of upstreaming, > we're in an odd place where ChromeShell mostly supports one activity at the > moment. I agree the class could be named better (it has nothing to do with ChromeActivity, I just mean activities in Chrome), but I'm not sure I see why it would be better to involve ChromeShellTestBase, since the test doesn't require that activity.
I think this stems from single-inheritance in java. You need to extend FragmentActivity but most activities in chrome for android use ChromeActivity for library loading. Introducing another ChromeActivityTestBase will just lead to more confusion. My suggestion was because I think you can use the existing ContentSHellTestBase scaffolding to get the application going but then send your intent for the PassphraseActivity. Another alternative is to just copy startChromeBrowserProcessSync into PassphraseActivityTest since that's all your using https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java (right): https://codereview.chromium.org/1025403002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/sync/ui/PassphraseActivityTest.java:78: pss.setSyncInitialized(true); On 2015/04/08 04:46:40, maxbogue wrote: > On 2015/04/07 22:22:11, maniscalco wrote: > > Since ProfileSyncService lives on the UI thread and this test doesn't execute > on > > the UI thread, we should be careful to ensure we don't call methods on pss > here. > > Can you move the calls on pss inside the runOnUiThreadBlocking call above? > > That way we're not accessing pss from anything other than the UI thread. > > Done. Turns out that PSS.get() at least DOES have an assert, so I had to move > that here too. I'm not sure why the assert triggered on the trybot but not on my > local run :/ Hmm. Is your device rooted? asserts should fire locally when run through test_runner.py but maybe we can only enable for a rooted device.
PTAL, I believe all stated concerns have been addressed.
LGTM My only remaining concern is that this test will live as part of ChromeShellTest.apk instead of ChromeSyncShellTest.apk. However, we (I) haven't fully defined what's supposed to go there. Unless someone like Yaron or Tommy objects, this location works for me. We can always move it in the future.
lgtm
lgtm
The CQ bit was checked by maxbogue@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1025403002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/cf0c3c3522ed57d4922b0eb6e9d57ef4cf0cbeb8 Cr-Commit-Position: refs/heads/master@{#324316} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
