Commit 6222bdea authored by Sky Malice's avatar Sky Malice Committed by Commit Bot

Mitigate ToolbarButtonIphTest flaky test.

Attempts to address the problem caused by https://crbug.com/1144328 by
watching for browser controls change events and then manually re-trying
the test body. This doesn't seem to be entirely foolproof, sometimes
onControlsOffsetChanged() is not called and the IPH is still prematurely
dismissed. However this seems to be very rare and it's not clear if the
root cause is actually 1144328 or something else. Locally testing,
testTabSwitcherButtonIph will sometimes fail after 20 runs, but
sometimes pass 100 in a row. This should be good enough for the bots
when they already use 3 attempts before marking failure. Hopefully this
mitigation is only temporary.

Bug: 1142979
Change-Id: I2b98da227d1f34ef49b20b6fe6f4c69f8e3a0737
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533201
Commit-Queue: Sky Malice <skym@chromium.org>
Reviewed-by: default avatarTheresa  <twellington@chromium.org>
Reviewed-by: default avatarWenyu Fu <wenyufu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827300}
parent 170f7981
...@@ -24,6 +24,7 @@ import androidx.test.espresso.action.ViewActions; ...@@ -24,6 +24,7 @@ import androidx.test.espresso.action.ViewActions;
import androidx.test.espresso.assertion.ViewAssertions; import androidx.test.espresso.assertion.ViewAssertions;
import androidx.test.filters.MediumTest; import androidx.test.filters.MediumTest;
import org.hamcrest.Matchers;
import org.junit.After; import org.junit.After;
import org.junit.Before; import org.junit.Before;
import org.junit.Rule; import org.junit.Rule;
...@@ -33,10 +34,14 @@ import org.mockito.Mock; ...@@ -33,10 +34,14 @@ import org.mockito.Mock;
import org.mockito.MockitoAnnotations; import org.mockito.MockitoAnnotations;
import org.chromium.base.Callback; import org.chromium.base.Callback;
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.base.test.util.CommandLineFlags; import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.DisabledTest; import org.chromium.base.test.util.Criteria;
import org.chromium.base.test.util.CriteriaHelper;
import org.chromium.base.test.util.Restriction; import org.chromium.base.test.util.Restriction;
import org.chromium.chrome.R; import org.chromium.chrome.R;
import org.chromium.chrome.browser.browser_controls.BrowserControlsStateProvider;
import org.chromium.chrome.browser.browser_controls.BrowserControlsUtils;
import org.chromium.chrome.browser.feature_engagement.TrackerFactory; import org.chromium.chrome.browser.feature_engagement.TrackerFactory;
import org.chromium.chrome.browser.flags.ChromeFeatureList; import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches; import org.chromium.chrome.browser.flags.ChromeSwitches;
...@@ -50,9 +55,7 @@ import org.chromium.components.feature_engagement.Tracker; ...@@ -50,9 +55,7 @@ import org.chromium.components.feature_engagement.Tracker;
import org.chromium.content_public.browser.test.util.TestThreadUtils; import org.chromium.content_public.browser.test.util.TestThreadUtils;
import org.chromium.ui.test.util.UiRestriction; import org.chromium.ui.test.util.UiRestriction;
/** /** Integration tests for showing IPH bubbles on the toolbar. */
* Integration tests for showing IPH bubbles on the toolbar.
*/
@RunWith(ChromeJUnit4ClassRunner.class) @RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE}) @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
@Features.EnableFeatures(ChromeFeatureList.TOOLBAR_IPH_ANDROID) @Features.EnableFeatures(ChromeFeatureList.TOOLBAR_IPH_ANDROID)
...@@ -63,6 +66,9 @@ public class ToolbarButtonIphTest { ...@@ -63,6 +66,9 @@ public class ToolbarButtonIphTest {
@Mock @Mock
private Tracker mTracker; private Tracker mTracker;
private BrowserControlsStateProvider.Observer mBrowserControlsObserver;
private CallbackHelper mBrowserControlsChangedCallbackHelper;
@Before @Before
public void setUp() { public void setUp() {
Looper.prepare(); Looper.prepare();
...@@ -78,24 +84,71 @@ public class ToolbarButtonIphTest { ...@@ -78,24 +84,71 @@ public class ToolbarButtonIphTest {
TrackerFactory.setTrackerForTests(mTracker); TrackerFactory.setTrackerForTests(mTracker);
mActivityTestRule.startMainActivityOnBlankPage(); mActivityTestRule.startMainActivityOnBlankPage();
mBrowserControlsChangedCallbackHelper = new CallbackHelper();
mBrowserControlsObserver = new BrowserControlsStateProvider.Observer() {
@Override
public void onControlsOffsetChanged(int topOffset, int topControlsMinHeightOffset,
int bottomOffset, int bottomControlsMinHeightOffset, boolean needsAnimate) {
// When glitching, the toolbar temporarily moves offscreen and animates back down.
// Wait for it to be done moving, which is known when the controls are fully
// visible.
if (BrowserControlsUtils.areBrowserControlsFullyVisible(
mActivityTestRule.getActivity().getBrowserControlsManager())) {
mBrowserControlsChangedCallbackHelper.notifyCalled();
}
}
};
mActivityTestRule.getActivity().getBrowserControlsManager().addObserver(
mBrowserControlsObserver);
} }
@After @After
public void tearDown() { public void tearDown() {
TrackerFactory.setTrackerForTests(null); TrackerFactory.setTrackerForTests(null);
mActivityTestRule.getActivity().getBrowserControlsManager().removeObserver(
mBrowserControlsObserver);
}
/**
* If onControlsOffsetChanged() is called during the test run, try again. The toolbar glitches
* off the screen animates back sometimes, which will cause the IPH bubbles to auto-dismiss.
* This only happens during startup and the second attempt should not be affected.
* TODO(https://crbug.com/1144328): Remove this once the underlying bug is fixed.
*/
private void runWithControlsChangeForgiveness(Runnable testBody) throws InterruptedException {
try {
testBody.run();
} catch (AssertionError ae) {
// This is a bit racy with checking our boolean flag, give it a little times. Don't let
// the error from CriteriaHelper escape because it would hide the underlying failure.
try {
CriteriaHelper.pollInstrumentationThread(
()
-> Criteria.checkThat(
mBrowserControlsChangedCallbackHelper.getCallCount(),
Matchers.greaterThan(0)));
} catch (Throwable ignored) {
throw new AssertionError("Found no browser controls change", ae);
}
testBody.run();
}
} }
@Test @Test
@MediumTest @MediumTest
public void testNewTabButtonIph() { public void testNewTabButtonIph() throws InterruptedException {
when(mTracker.shouldTriggerHelpUI(FeatureConstants.NEW_TAB_PAGE_HOME_BUTTON_FEATURE)) when(mTracker.shouldTriggerHelpUI(FeatureConstants.NEW_TAB_PAGE_HOME_BUTTON_FEATURE))
.thenReturn(true); .thenReturn(true);
mActivityTestRule.loadUrl(UrlConstants.NTP_URL); runWithControlsChangeForgiveness(() -> {
onView(withId(R.id.home_button)).check(matches(withHighlight(false))); mActivityTestRule.loadUrl(UrlConstants.NTP_URL);
onView(withId(R.id.home_button)).check(matches(withHighlight(false)));
mActivityTestRule.loadUrl("about:blank"); mActivityTestRule.loadUrl("about:blank");
onView(withId(R.id.home_button)).check(matches(withHighlight(true))); onView(withId(R.id.home_button)).check(matches(withHighlight(true)));
});
} }
@Test @Test
...@@ -121,22 +174,23 @@ public class ToolbarButtonIphTest { ...@@ -121,22 +174,23 @@ public class ToolbarButtonIphTest {
@Test @Test
@MediumTest @MediumTest
@DisabledTest(message = "https://crbug.com/1144263")
@Restriction({UiRestriction.RESTRICTION_TYPE_PHONE}) @Restriction({UiRestriction.RESTRICTION_TYPE_PHONE})
public void testTabSwitcherButtonIph() { public void testTabSwitcherButtonIph() throws InterruptedException {
when(mTracker.shouldTriggerHelpUI(FeatureConstants.TAB_SWITCHER_BUTTON_FEATURE)) when(mTracker.shouldTriggerHelpUI(FeatureConstants.TAB_SWITCHER_BUTTON_FEATURE))
.thenReturn(true); .thenReturn(true);
// Navigating to about:blank here was flaky for some reason, probably because the page was runWithControlsChangeForgiveness(() -> {
// already on about:blank. // Navigating to about:blank here was flaky for some reason, probably because the page
mActivityTestRule.loadUrl("chrome://about/"); // was already on about:blank.
ViewInteraction toolbarTabButtonInteraction = onView(withId(R.id.tab_switcher_button)); mActivityTestRule.loadUrl("chrome://about/");
toolbarTabButtonInteraction.check(ViewAssertions.matches(withHighlight(true))); ViewInteraction toolbarTabButtonInteraction = onView(withId(R.id.tab_switcher_button));
toolbarTabButtonInteraction.check(ViewAssertions.matches(withHighlight(true)));
toolbarTabButtonInteraction.perform(ViewActions.click()); toolbarTabButtonInteraction.perform(ViewActions.click());
onView(withId(R.id.new_tab_button)).check(ViewAssertions.matches(withHighlight(true))); onView(withId(R.id.new_tab_button)).check(ViewAssertions.matches(withHighlight(true)));
onView(withId(R.id.tab_switcher_mode_tab_switcher_button)).perform(ViewActions.click()); onView(withId(R.id.tab_switcher_mode_tab_switcher_button)).perform(ViewActions.click());
toolbarTabButtonInteraction.check(ViewAssertions.matches(withHighlight(false))); toolbarTabButtonInteraction.check(ViewAssertions.matches(withHighlight(false)));
});
} }
} }
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