Skip to content

GitLab

T
tangled
Commit b683772d authored by Victor Hugo Vianna Silva's avatar Victor Hugo Vianna Silva Committed by Commit Bot
Browse files
Options

Allow lazy construction of AndroidSyncSettings in tests 

The motivation is that AndroidSyncSettings will soon have different
behaviors depending on Chrome's startup state, so we need to be able
to fake the appropriate state in tests before actually constructing the
object.

We also take the time to update some documentation of this class that
became inaccurate with crrev.com/c/2366895.

Bug: 1125622
Change-Id: I7fd0399bcb49024f151b78c7a234f99601481948
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2396335
Commit-Queue: Victor Vianna <victorvianna@google.com>
Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804866}
parent fbcc81e4
Hide whitespace changes
Inline Side-by-side
Showing with 70 additions and 46 deletions
chrome/android/java/src/org/chromium/chrome/browser/sync/AndroidSyncSettings.java
View file @ b683772d
......@@ -31,6 +31,10 @@ import org.chromium.components.sync.SyncContentResolverDelegate;
import org.chromium.components.sync.SystemSyncContentResolverDelegate;
/**
* WARNING: Chrome will be decoupled from Android auto-sync (crbug.com/1105795).
* Some documentation in this class may be outdated or may not be coherent when
* DecoupleSyncFromAndroidMasterSync is enabled.
*
* A helper class to handle the current status of sync for Chrome in Android settings.
* It also provides an observer to be used whenever Android sync settings change.
*
......@@ -44,6 +48,7 @@ public class AndroidSyncSettings {
private final Object mLock = new Object();
// Cached value of the static |getContractAuthority()|.
private final String mContractAuthority;
private final SyncContentResolverDelegate mSyncContentResolverDelegate;
......@@ -105,7 +110,7 @@ public class AndroidSyncSettings {
@VisibleForTesting
public AndroidSyncSettings(SyncContentResolverDelegate syncContentResolverDelegate,
@Nullable Callback<Boolean> callback, @Nullable Account account) {
mContractAuthority = ContextUtils.getApplicationContext().getPackageName();
mContractAuthority = getContractAuthority();
mSyncContentResolverDelegate = syncContentResolverDelegate;
mAccount = account;
......@@ -152,14 +157,14 @@ public class AndroidSyncSettings {
}
/**
* Make sure Chrome is syncable, and enable sync.
* Enables Chrome sync for |mAccount| if it's non-null.
*/
public void enableChromeSync() {
setChromeSyncEnabled(true);
}
/**
* Disables Android Chrome sync
* Disables Chrome sync for |mAccount| if it's non-null.
*/
public void disableChromeSync() {
setChromeSyncEnabled(false);
......@@ -189,10 +194,13 @@ public class AndroidSyncSettings {
}
/**
* Returns the contract authority to use when requesting sync.
* Returns the contract authority used by Chrome when talking to auto-sync.
* Exposed only to tests, so they can fake user interaction with the
* auto-sync UI.
*/
public String getContractAuthority() {
return mContractAuthority;
@VisibleForTesting
public static String getContractAuthority() {
return ContextUtils.getApplicationContext().getPackageName();
}
/**
......@@ -227,7 +235,7 @@ public class AndroidSyncSettings {
}
/**
* Ensure Chrome is registered with the Android Sync Manager iff signed in.
* Updates whether Chrome is registered with the Android Auto-Sync Manager.
*
* This is what causes the "Chrome" option to appear in Settings -> Accounts -> Sync .
* This function must be called within a synchronized block.
......
chrome/android/javatests/src/org/chromium/chrome/browser/password_manager/settings/PasswordViewingTypeTest.java
View file @ b683772d
......@@ -60,7 +60,7 @@ public class PasswordViewingTypeTest {
TestThreadUtils.runOnUiThreadBlocking(() -> {
AndroidSyncSettings.overrideForTests(
new AndroidSyncSettings(mSyncContentResolverDelegate));
mAuthority = AndroidSyncSettings.get().getContractAuthority();
mAuthority = AndroidSyncSettings.getContractAuthority();
AndroidSyncSettings.get().updateAccount(mAccount);
});
}
......
chrome/android/javatests/src/org/chromium/chrome/browser/sync/AndroidSyncSettingsTest.java
View file @ b683772d
......@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.sync;
import android.accounts.Account;
import android.os.Bundle;
import android.support.test.InstrumentationRegistry;
import androidx.test.filters.SmallTest;
......@@ -129,6 +128,10 @@ public class AndroidSyncSettingsTest {
setupTestAccounts();
mSyncContentResolverDelegate = new CountingMockSyncContentResolverDelegate();
mAuthority = AndroidSyncSettings.getContractAuthority();
}
private void createAndroidSyncSettings() throws TimeoutException {
TestThreadUtils.runOnUiThreadBlocking(() -> {
mAndroidSyncSettings = new AndroidSyncSettings(mSyncContentResolverDelegate,
(Boolean result) -> mCallbackHelper.notifyCalled(), mAccount);
......@@ -136,16 +139,12 @@ public class AndroidSyncSettingsTest {
mNumberOfCallsToWait++;
mCallbackHelper.waitForCallback(0, mNumberOfCallsToWait);
mAuthority = mAndroidSyncSettings.getContractAuthority();
if (ChromeFeatureList.isEnabled(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)) {
Assert.assertTrue(
mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) <= 0);
} else {
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
}
mSyncSettingsObserver = new MockSyncSettingsObserver();
mAndroidSyncSettings.registerObserver(mSyncSettingsObserver);
}
private void setupTestAccounts() {
......@@ -211,6 +210,8 @@ public class AndroidSyncSettingsTest {
@SmallTest
@Feature({"Sync"})
public void testAccountInitialization() throws TimeoutException {
createAndroidSyncSettings();
// mAccount was set to be syncable and not have periodic syncs.
Assert.assertEquals(1, mSyncContentResolverDelegate.mSetIsSyncableCalls.get());
Assert.assertEquals(1, mSyncContentResolverDelegate.mRemovePeriodicSyncCalls.get());
......@@ -229,7 +230,9 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testToggleMasterSyncFromSettings() throws InterruptedException {
public void testToggleMasterSyncFromSettings() throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
Assert.assertTrue(mAndroidSyncSettings.doesMasterSyncSettingAllowChromeSync());
......@@ -252,7 +255,9 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testToggleChromeSyncFromSettings() throws InterruptedException {
public void testToggleChromeSyncFromSettings() throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
......@@ -300,7 +305,10 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testToggleAccountSyncFromApplication() throws InterruptedException {
public void testToggleAccountSyncFromApplication()
throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
setMasterSyncAllowsChromeSync();
enableChromeSyncOnUiThread();
......@@ -315,7 +323,10 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testToggleSyncabilityForMultipleAccounts() throws InterruptedException {
public void testToggleSyncabilityForMultipleAccounts()
throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
setMasterSyncAllowsChromeSync();
enableChromeSyncOnUiThread();
......@@ -344,7 +355,9 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testSyncSettingsCaching() throws InterruptedException {
public void testSyncSettingsCaching() throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
setMasterSyncAllowsChromeSync();
enableChromeSyncOnUiThread();
......@@ -388,47 +401,42 @@ public class AndroidSyncSettingsTest {
@Test
@SmallTest
@Feature({"Sync"})
public void testGetContractAuthority() {
Assert.assertEquals("The contract authority should be the package name.",
InstrumentationRegistry.getTargetContext().getPackageName(),
mAndroidSyncSettings.getContractAuthority());
}
public void testAndroidSyncSettingsPostsNotifications()
throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
@Test
@SmallTest
@Feature({"Sync"})
public void testAndroidSyncSettingsPostsNotifications() throws InterruptedException {
setMasterSyncAllowsChromeSync();
MockSyncSettingsObserver syncSettingsObserver = new MockSyncSettingsObserver();
mAndroidSyncSettings.registerObserver(syncSettingsObserver);
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
mAndroidSyncSettings.enableChromeSync();
Assert.assertTrue("enableChromeSync should trigger observers",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
updateAccount(mAlternateAccount);
Assert.assertTrue("switching to account with different settings should notify",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
updateAccount(mAccount);
Assert.assertTrue("switching to account with different settings should notify",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
mAndroidSyncSettings.enableChromeSync();
Assert.assertFalse("enableChromeSync shouldn't trigger observers",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
mAndroidSyncSettings.disableChromeSync();
Assert.assertTrue("disableChromeSync should trigger observers",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
mSyncSettingsObserver.clearNotification();
syncSettingsObserver.clearNotification();
mAndroidSyncSettings.disableChromeSync();
Assert.assertFalse("disableChromeSync shouldn't observers",
mSyncSettingsObserver.receivedNotification());
syncSettingsObserver.receivedNotification());
}
@Test
......@@ -436,6 +444,8 @@ public class AndroidSyncSettingsTest {
@Feature({"Sync"})
@Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
public void testIsSyncableOnSigninAndNotOnSignout() throws TimeoutException {
createAndroidSyncSettings();
Assert.assertTrue(mSyncContentResolverDelegate.getIsSyncable(mAccount, mAuthority) > 0);
updateAccountWithCallback(null, (Boolean result) -> {
......@@ -456,7 +466,10 @@ public class AndroidSyncSettingsTest {
@SmallTest
@Feature({"Sync"})
@Features.DisableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
public void testSyncableIsAlwaysSetWhenEnablingSync() throws InterruptedException {
public void testSyncableIsAlwaysSetWhenEnablingSync()
throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
// Setup bad state.
mSyncContentResolverDelegate.setMasterSyncAutomatically(true);
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
......@@ -482,7 +495,10 @@ public class AndroidSyncSettingsTest {
@Features.EnableFeatures(ChromeFeatureList.DECOUPLE_SYNC_FROM_ANDROID_MASTER_SYNC)
// TODO(crbug.com/1105795): Remove this test after DecoupleSyncFromAndroidMasterSync has
// launched, since testToggleChromeSyncFromSettings() covers the same functionality.
public void testSyncStateDoesNotDependOnMasterSync() throws InterruptedException {
public void testSyncStateDoesNotDependOnMasterSync()
throws InterruptedException, TimeoutException {
createAndroidSyncSettings();
mSyncContentResolverDelegate.setSyncAutomatically(mAccount, mAuthority, true);
mSyncContentResolverDelegate.setMasterSyncAutomatically(false);
mSyncContentResolverDelegate.waitForLastNotificationCompleted();
......
chrome/android/javatests/src/org/chromium/chrome/browser/sync/SyncTest.java
View file @ b683772d
......@@ -110,7 +110,7 @@ public class SyncTest {
// settings. This would normally be done by the
// SystemSyncTestRule.getSyncContentResolver().
mSyncTestRule.getSyncContentResolver().renameAccounts(
oldAccount, newAccount, getAndroidSyncSettings().getContractAuthority());
oldAccount, newAccount, AndroidSyncSettings.getContractAuthority());
// Starts the rename process. Normally, this is triggered by the broadcast
// listener as well.
......@@ -141,7 +141,7 @@ public class SyncTest {
@Feature({"Sync"})
public void testStopAndStartSyncThroughAndroidChromeSync() {
Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
String authority = getAndroidSyncSettings().getContractAuthority();
String authority = AndroidSyncSettings.getContractAuthority();
Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
Assert.assertTrue(SyncTestUtil.isSyncRequested());
......@@ -182,7 +182,7 @@ public class SyncTest {
@DisabledTest(message = "Test is flaky crbug.com/1100890")
public void testReenableMasterSyncFirst() {
Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
String authority = getAndroidSyncSettings().getContractAuthority();
String authority = AndroidSyncSettings.getContractAuthority();
Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
Assert.assertTrue(SyncTestUtil.isSyncRequested());
......@@ -215,7 +215,7 @@ public class SyncTest {
@Feature({"Sync"})
public void testReenableChromeSyncFirst() {
Account account = mSyncTestRule.setUpAccountAndSignInForTesting();
String authority = getAndroidSyncSettings().getContractAuthority();
String authority = AndroidSyncSettings.getContractAuthority();
Assert.assertTrue(getAndroidSyncSettings().isSyncEnabled());
Assert.assertTrue(SyncTestUtil.isSyncRequested());
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment