[Android][Test] Enable flaky NTP test with FakeAccountManagerFacade

This CL fixes one flaky NTP test with the new FakeAccountManagerFacade
implementation and removes unused code in AccountManagerTestRule.

Bug: 996716
Change-Id: Ifb6662bcc572ad8dab959d6b8aa18d4dae8578c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2220050Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Alice Wang <aliceywang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774564}

chrome/android/feed/core/javatests/src/org/chromium/chrome/browser/feed/FeedNewTabPageTest.java

@@ -51,7 +51,6 @@ import org.chromium.base.test.params.ParameterizedRunner;
 import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
-import org.chromium.base.test.util.FlakyTest;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
@@ -141,7 +140,15 @@ public class FeedNewTabPageTest {
         mMostVisitedSites = new FakeMostVisitedSites();
         mMostVisitedSites.setTileSuggestions(mSiteSuggestions);
         mSuggestionsDeps.getFactory().mostVisitedSites = mMostVisitedSites;
+    }
+
+    @After
+    public void tearDown() {
+        mTestServer.stopAndDestroyServer();
+        FeedProcessScopeFactory.setTestNetworkClient(null);
+    }
 
+    private void openNewTabPage() {
         mActivityTestRule.loadUrl(UrlConstants.NTP_URL);
         mTab = mActivityTestRule.getActivity().getActivityTab();
         NewTabPageTestUtils.waitForNtpLoaded(mTab);
@@ -152,12 +159,6 @@ public class FeedNewTabPageTest {
         Assert.assertEquals(mSiteSuggestions.size(), mTileGridLayout.getChildCount());
     }
 
-    @After
-    public void tearDown() {
-        mTestServer.stopAndDestroyServer();
-        FeedProcessScopeFactory.setTestNetworkClient(null);
-    }
-
     private void waitForPopup(Matcher<View> matcher) {
         View mainDecorView = mActivityTestRule.getActivity().getWindow().getDecorView();
         onView(isRoot())
@@ -169,6 +170,7 @@ public class FeedNewTabPageTest {
     @MediumTest
     @Feature({"FeedNewTabPage"})
     public void testSignInPromo() {
+        openNewTabPage();
         SignInPromo.SigninObserver signinObserver = mNtp.getCoordinatorForTesting()
                                                             .getMediatorForTesting()
                                                             .getSignInPromoForTesting()
@@ -220,6 +222,7 @@ public class FeedNewTabPageTest {
     @Feature({"FeedNewTabPage"})
     @DisabledTest(message = "https://crbug.com/1046822")
     public void testSignInPromo_DismissBySwipe() {
+        openNewTabPage();
         boolean dismissed = SharedPreferencesManager.getInstance().readBoolean(
                 ChromePreferenceKeys.SIGNIN_PROMO_NTP_PROMO_DISMISSED, false);
         if (dismissed) {
@@ -255,18 +258,17 @@ public class FeedNewTabPageTest {
     @Test
     @MediumTest
     @Feature({"FeedNewTabPage"})
-    @FlakyTest(message = "https://crbug.com/996716")
-    @AccountManagerTestRule.BlockGetAccounts
     public void testSignInPromo_AccountsNotReady() {
+        mAccountManagerTestRule.setIsCachePopulated(false);
+        openNewTabPage();
         // Check that the sign-in promo is not shown if accounts are not ready.
         onView(instanceOf(RecyclerView.class))
                 .perform(RecyclerViewActions.scrollToPosition(SIGNIN_PROMO_POSITION));
         onView(withId(R.id.signin_promo_view_container)).check(doesNotExist());
 
-        // Wait for accounts cache population to finish and reload ntp.
-        mAccountManagerTestRule.unblockGetAccountsAndWaitForAccountsPopulated();
-        TestThreadUtils.runOnUiThreadBlocking(() -> mTab.reload());
-        NewTabPageTestUtils.waitForNtpLoaded(mTab);
+        mAccountManagerTestRule.setIsCachePopulated(true);
+        TestThreadUtils.runOnUiThreadBlocking(mTab::reload);
+        ChromeTabUtils.waitForTabPageLoaded(mTab, mTab.getUrlString());
 
         // Check that the sign-in promo is displayed this time.
         onView(instanceOf(RecyclerView.class))
@@ -280,6 +282,7 @@ public class FeedNewTabPageTest {
     @Feature({"NewTabPage", "FeedNewTabPage"})
     @ParameterAnnotations.UseMethodParameter(SigninPromoParams.class)
     public void testArticleSectionHeaderWithMenu(boolean disableSigninPromoCard) throws Exception {
+        openNewTabPage();
         // Scroll to the article section header in case it is not visible.
         onView(instanceOf(RecyclerView.class))
                 .perform(RecyclerViewActions.scrollToPosition(ARTICLE_SECTION_HEADER_POSITION));
@@ -318,6 +321,7 @@ public class FeedNewTabPageTest {
     @Feature({"FeedNewTabPage"})
     @DisabledTest(message = "https://crbug.com/914068")
     public void testArticleSectionHeader() throws Exception {
+        openNewTabPage();
         final int expectedCountWhenCollapsed = 2;
         final int expectedCountWhenExpanded = 4; // 3 header views and the empty view.
 
@@ -383,6 +387,7 @@ public class FeedNewTabPageTest {
     @Feature({"FeedNewTabPage"})
     @DisabledTest(message = "crbug.com/1064388")
     public void testFeedDisabledByPolicy() throws Exception {
+        openNewTabPage();
         final boolean pref = TestThreadUtils.runOnUiThreadBlocking(
                 ()
                         -> PrefServiceBridge.getInstance().getBoolean(

chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java

@@ -21,6 +21,7 @@ import org.chromium.chrome.browser.signin.SigninManager.SignInAllowedObserver;
 import org.chromium.chrome.browser.signin.SigninManager.SignInStateObserver;
 import org.chromium.chrome.browser.signin.SigninPreferencesManager;
 import org.chromium.chrome.browser.signin.SigninPromoController;
+import org.chromium.components.signin.AccountManagerFacade;
 import org.chromium.components.signin.AccountManagerFacadeProvider;
 import org.chromium.components.signin.AccountsChangeObserver;
 import org.chromium.components.signin.metrics.SigninAccessPoint;
@@ -188,17 +189,20 @@ public abstract class SignInPromo extends OptionalLeaf {
     public class SigninObserver implements SignInStateObserver, SignInAllowedObserver,
                                            ProfileDataCache.Observer, AccountsChangeObserver {
         private final SigninManager mSigninManager;
+        private final AccountManagerFacade mAccountManagerFacade;
 
         /** Guards {@link #unregister()}, which can be called multiple times. */
         private boolean mUnregistered;
 
         private SigninObserver(SigninManager signinManager) {
             mSigninManager = signinManager;
+            mAccountManagerFacade = AccountManagerFacadeProvider.getInstance();
+
             mSigninManager.addSignInAllowedObserver(this);
             mSigninManager.addSignInStateObserver(this);
 
             mProfileDataCache.addObserver(this);
-            AccountManagerFacadeProvider.getInstance().addObserver(this);
+            mAccountManagerFacade.addObserver(this);
         }
 
         private void unregister() {
@@ -208,7 +212,7 @@ public abstract class SignInPromo extends OptionalLeaf {
             mSigninManager.removeSignInAllowedObserver(this);
             mSigninManager.removeSignInStateObserver(this);
             mProfileDataCache.removeObserver(this);
-            AccountManagerFacadeProvider.getInstance().removeObserver(this);
+            mAccountManagerFacade.removeObserver(this);
         }
 
         // SignInAllowedObserver implementation.
@@ -237,7 +241,7 @@ public abstract class SignInPromo extends OptionalLeaf {
         // AccountsChangeObserver implementation.
         @Override
         public void onAccountsChanged() {
-            mAccountsReady = AccountManagerFacadeProvider.getInstance().isCachePopulated();
+            mAccountsReady = mAccountManagerFacade.isCachePopulated();
             // We don't change the visibility here to avoid the promo popping up in the feed
             // unexpectedly. If accounts are ready, the promo will be shown up on the next reload.
             notifyDataChanged();

components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/AccountManagerTestRule.java

@@ -16,28 +16,12 @@ import org.chromium.components.signin.AccountManagerFacadeProvider;
 import org.chromium.components.signin.AccountUtils;
 import org.chromium.components.signin.ProfileDataSource;
 
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-
 /**
  * JUnit4 rule for overriding behaviour of {@link AccountManagerFacade} for tests.
  */
 public class AccountManagerTestRule implements TestRule {
     private final FakeAccountManagerFacade mFakeAccountManagerFacade;
 
-    /**
-     * Test method annotation signaling that the account population should be blocked until {@link
-     * #unblockGetAccountsAndWaitForAccountsPopulated()} is called.
-     *
-     * TODO(https://crbug.com/1078342) Cleanup this interface
-     * We should be able to mock isCachePopulated to simulate signin promo shown status.
-     */
-    @Target(ElementType.METHOD)
-    @Retention(RetentionPolicy.RUNTIME)
-    public @interface BlockGetAccounts {}
-
     public AccountManagerTestRule(@Nullable FakeProfileDataSource fakeProfileDataSource) {
         mFakeAccountManagerFacade = new FakeAccountManagerFacade(fakeProfileDataSource);
     }
@@ -62,13 +46,10 @@ public class AccountManagerTestRule implements TestRule {
     }
 
     /**
-     * Unblocks all get accounts requests to {@link AccountManagerFacade}.
-     * Has no effect in tests that are not annotated with {@link BlockGetAccounts}.
-     *
-     * TODO(https://crbug.com/1078342) Cleanup this method.
+     * Sets the boolean for whether the account cache has already been populated.
      */
-    public void unblockGetAccountsAndWaitForAccountsPopulated() {
-        AccountManagerFacadeProvider.getInstance().tryGetGoogleAccounts();
+    public void setIsCachePopulated(boolean isCachePopulated) {
+        mFakeAccountManagerFacade.setIsCachePopulated(isCachePopulated);
     }
 
     /**

components/signin/core/browser/android/javatests/src/org/chromium/components/signin/test/util/FakeAccountManagerFacade.java

@@ -33,6 +33,9 @@ public class FakeAccountManagerFacade implements AccountManagerFacade {
     @GuardedBy("mLock")
     private final Set<AccountHolder> mAccountHolders = new LinkedHashSet<>();
 
+    @GuardedBy("mLock")
+    private boolean mIsCachePopulated = true;
+
     private final @Nullable FakeProfileDataSource mFakeProfileDataSource;
 
     /**
@@ -63,7 +66,9 @@ public class FakeAccountManagerFacade implements AccountManagerFacade {
 
     @Override
     public boolean isCachePopulated() {
-        return true;
+        synchronized (mLock) {
+            return mIsCachePopulated;
+        }
     }
 
     @Override
@@ -113,6 +118,15 @@ public class FakeAccountManagerFacade implements AccountManagerFacade {
         return true;
     }
 
+    /**
+     * Sets the boolean for whether the account cache has already been populated.
+     */
+    void setIsCachePopulated(boolean isCachePopulated) {
+        synchronized (mLock) {
+            mIsCachePopulated = isCachePopulated;
+        }
+    }
+
     void addAccount(Account account) {
         AccountHolder accountHolder = AccountHolder.builder(account).alwaysAccept(true).build();
         // As this class is accessed both from UI thread and worker threads, we lock the access