New issue
Advanced search Search tips

Issue 635661 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

ContextualSearchManagerTests flaky without test delay

Project Member Reported by jbudorick@chromium.org, Aug 8 2016

Issue description

Over the past day or two, several ContextualSearchManagerTests appear to have gotten flakier on the L & M bots on chromium.android: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk&tests=ContextualSearchManagerTest
 

Comment 1 by donnd@chromium.org, Aug 9 2016

Components: UI>Browser>Mobile>TouchToSearch
Status: Started (was: Assigned)
I suspect this is because the test config changed recently, so now we're testing everything with two relatively new features enabled: Tap Suppression and Translation.  Tap Suppression is probably responsible.  I'll look closer at this after lunch today.
I'm actually rethinking whether the fieldtrial_testing_config.json change would be affecting instrumentation tests. 

From the README, it sounds like they're used for the perf bots and browser tests in the waterfall:
https://cs.chromium.org/chromium/src/testing/variations/README?q=fieldtrial_testing_config&sq=package:chromium&l=6&dr=C

Comment 3 by donnd@chromium.org, Aug 9 2016

I'm pretty sure the fieldtrial_testing_config.json change does affect the instrumentation tests because when I first tried to land that change it broke a few of the tests and I had to land two CLs to fix the issues.

DETAILS - drilling down that CL from https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2187213003/20001 we see
Fri Jul 29 2016 10:17:45 GMT-0700 ()(1 hour 2 minutes)
CQ stopped processing patch
(Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/113016))
But for some reason that link is no longer live.


Project Member

Comment 4 by bugdroid1@chromium.org, Aug 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6b444818bde9d542ca81ab21284bb187e94eecb

commit d6b444818bde9d542ca81ab21284bb187e94eecb
Author: donnd <donnd@chromium.org>
Date: Wed Aug 10 00:40:39 2016

[TTS] Workaround for flaky Contextual Search tests.

This change should help us isolate the cause of the recent flakiness we're seeing on L+.
We think the flakiness is due to Tap Suppression now being default-enabled based on the updated test configuration landed in CL 2187213003.

BUG= 635661 

Review-Url: https://codereview.chromium.org/2235523002
Cr-Commit-Position: refs/heads/master@{#410901}

[modify] https://crrev.com/d6b444818bde9d542ca81ab21284bb187e94eecb/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

It looks like these are still flaking. The bots I see the failures on are tablet testers, might be worth trying to repro locally on an L or M tablet.

Comment 6 by donnd@chromium.org, Aug 10 2016

Cc: tedc...@chromium.org wnwen@chromium.org jbudorick@chromium.org
Looks like the proximate cause is accellerated test-startup timing due to the recent change to "Separate deferred startup into tasks": Review-Url: https://codereview.chromium.org/2207933002 (Original: https://codereview.chromium.org/2121863002/)

I can repro locally on my tablet.  When the instrumentation tests start up there used to be an approximately 1/2 second gap between the page rendering and the first simulated event.  Now there is no time gap at all, and it appears that sometimes the initial simulated event is dropped for some reason.  All the failures I see in ContextualSearchManagerTest.java happen because the initial click event does not register probably because of the startup timing.

Looks like the only major impact is on ContextualSearchManagerTest so we can try to find a fix to apply during the startup of that class, but any code calling DOMUtils.clickNode at the beginning of a unit test will probably be flaky due to this issue too.

Labels: -Pri-2 Pri-1
Peter, can you please help look into this?

The bots have been consistently red for a couple of days, and I'd like to get them green again tomorrow (hopefully without a revert, but as sheriff my mandate is to revert first and ask questions later).

Comment 8 by wnwen@chromium.org, Aug 11 2016

Cc: -wnwen@chromium.org donnd@chromium.org
Owner: wnwen@chromium.org
Will look into this today.

All failures caused by this change should be fixed on test-side, as this is a 1-second improvement for user startup tasks that broke some tests which implicitly depended on a 1-second Criteria poll that used to be in every test, making every test take at least 1 second slower. So a side effect of that change is every ChromeActivity/ChromeTabbedActivity instrumentation test should run 1 second faster, and only the ones that need to wait will wait. :)

Revert and reland sounds good, especially if I don't get a fix in today.
For a bit more context, and so you can test a possible fix. This is where we've seen these tests flaking more often:

# Lollipop Tablet Tester: 4362 -> 4353 #

-1-1------ contextualsearch.ContextualSearchManagerTest#testChainedSearchContentVisibility
---1------ contextualsearch.ContextualSearchManagerTest#testLongPressGestureFollowedByScrollMaintainsSelection
--1--1---- contextualsearch.ContextualSearchManagerTest#testPreventHandlingCurrentSelectionModification
--1----111 contextualsearch.ContextualSearchManagerTest#testTap
-1-------1 contextualsearch.ContextualSearchManagerTest#testTapALot
----1----- contextualsearch.ContextualSearchManagerTest#testTapDisablePreload

# Marshmallow Tablet Tester: 5050 -> 5041 #

----1----- contextualsearch.ContextualSearchManagerTest#testPanelDismissedOnToggleFullscreen
--1------- contextualsearch.ContextualSearchManagerTest#testPreventHandlingCurrentSelectionModification
----1---1- contextualsearch.ContextualSearchManagerTest#testTapSearchBarPromotesToTab

(the "1"s are the failures)
Re #8, that's awesome!

For the Contextual Search tests, I wonder if there's some method we can hook into to know whether the DOM is initiated? Or some contextual search thing is hooked up (I can help look into that). 
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/52a70a9106d675b556cd83dc2523f126aff86687

commit 52a70a9106d675b556cd83dc2523f126aff86687
Author: donnd <donnd@chromium.org>
Date: Thu Aug 11 21:48:45 2016

[TTS] Delay instrumentation tests at startup for flakes.

New startup timing causes flaky tests due to higher performance.
This CL introduces a workaround that allows delays the
the Contextual Search instrumentation test startup sequence for all
CS tests.

Contextual Search instrumentation tests now use the legacy delay.
Also removes an attempted workaround for TTS that failed.

BUG= 635661 

Review-Url: https://codereview.chromium.org/2232383002
Cr-Commit-Position: refs/heads/master@{#411425}

[modify] https://crrev.com/52a70a9106d675b556cd83dc2523f126aff86687/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java

Comment 13 by donnd@chromium.org, Aug 11 2016

We landed a workaround for Contextual Search only.  Anyone think we need to scan for other flaky tests due to this issue?  

We don't need to merge this into M-53, right?


Cc: -tedc...@chromium.org -donnd@chromium.org -jbudorick@chromium.org
Labels: -Pri-1 Pri-2
Owner: donnd@chromium.org
Summary: ContextualSearchManagerTests flaky without test delay (was: ContextualSearchManagerTests flaky on L+)
We don't need a merge (unless Peter's change went into 43).

I haven't seen any other tests flake as a result; the bots are actually in pretty good shape now. 

Let's leave this open to track a future more robust fix.
You started fixing this bug over two years ago. Are you still working on it? 
Status: Fixed (was: Started)
Been pretty reliable lately.

Sign in to add a comment