Touch Feedback tests on Valgrind

Remove the message loop wait that changes timing of tests on valgrind. None of the tests are checking for delayed state changes, so the waits are unnecessary.

Remove the filter to test failures on the bots.

TEST=ShelfViewTest.AppListButtonTouchFeedback, ShelfViewTest.AppListButtonTouchFeedbackCancellation, SystemTrayTest.TrayPopupItemContainerTouchFeedback, SystemTrayTest.TrayPopupItemContainerTouchFeedbackCancellation, TrayDetailsViewTest.HoverHighlightViewTouchFeedback, TrayDetailsViewTest.HoverHighlightViewTouchFeedbackCancellation, TrayDetailsViewTest.TrayPopupHeaderButtonTouchFeedback, TrayDetailsViewTest.TrayPopupHeaderButtonTouchFeedbackCancellation, WebNotificationTrayTest.TouchFeedback, WebNotificationTrayTest.TouchFeedbackCancellation
BUG=421888
Committed: https://crrev.com/7185d9d33461634fbdde9912e95189ae63623785
Cr-Commit-Position: refs/heads/master@{#319881}

[jonross, 2015-03-03 23:22:25]

Patchset #1 (id:1) has been deleted

[jonross, 2015-03-03 23:30:33]

Patchset #1 (id:20001) has been deleted

[jonross, 2015-03-04 16:38:17]

Patchset #1 (id:40001) has been deleted

[jonross, 2015-03-04 16:38:23]

Patchset #1 (id:60001) has been deleted

[jonross, 2015-03-04 16:43:08]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2015-03-04 16:43:08]

Hey Rob,

Could you take a first look at these test updates? I'm removing a message loop
wait to fix a race that occurs on valgrind.

[flackr, 2015-03-04 18:44:29]

This change lgtm but I'm very curious why calling RunAllPendingInMessageLoop()
is causing valgrind failures. Is this always the case? Or are there tests which
pass valgrind and call RunAllPendingInMessageLoop and it only fails in the
context of these tests? Any idea what the particular failure is?

[jonross, 2015-03-06 15:45:29]

On 2015/03/04 18:44:29, flackr wrote:
> This change lgtm but I'm very curious why calling RunAllPendingInMessageLoop()
> is causing valgrind failures. Is this always the case? Or are there tests
which
> pass valgrind and call RunAllPendingInMessageLoop and it only fails in the
> context of these tests? Any idea what the particular failure is?

Digging into the failure, the gesture recognizer adds events to the loop that
inject different events based on time delays. (This can be long presses, or
canceling gestures for upcoming touch releases.)

On valgrind these timeouts appear to be hit, leading to a different set of
events being directed at the targeted view.

This occurs even when both a TouchPress and TouchRelease have forced timestamps
that are below the long press duration.

I'm following up with tdresser@ to see if we can follow up this change with
logging.

[jonross, 2015-03-06 15:46:41]

Hi Stefan,

I've updated some tests that were failing on Valgrind, so that we can enable
them again.

Could you provide an owners review?

Thanks

[flackr, 2015-03-06 15:47:05]

On 2015/03/06 15:45:29, jonross wrote:
> On 2015/03/04 18:44:29, flackr wrote:
> > This change lgtm but I'm very curious why calling
RunAllPendingInMessageLoop()
> > is causing valgrind failures. Is this always the case? Or are there tests
> which
> > pass valgrind and call RunAllPendingInMessageLoop and it only fails in the
> > context of these tests? Any idea what the particular failure is?
> 
> Digging into the failure, the gesture recognizer adds events to the loop that
> inject different events based on time delays. (This can be long presses, or
> canceling gestures for upcoming touch releases.)
> 
> On valgrind these timeouts appear to be hit, leading to a different set of
> events being directed at the targeted view.
> 
> This occurs even when both a TouchPress and TouchRelease have forced
timestamps
> that are below the long press duration.
> 
> I'm following up with tdresser@ to see if we can follow up this change with
> logging.

Thanks for looking into it, perhaps it's worth filing a bug to track adding the
logging to easily identify why tests are failing on valgrind and how to fix. As
for this bug, slgtm

[jonross, 2015-03-06 15:47:16]

jonross@chromium.org changed reviewers:
+ skuhne@chromium.org

[jonross, 2015-03-06 15:47:31]

Hi Stefan,

I've updated some tests that were failing on Valgrind, so that we can enable
them again.

Could you provide an owners review?

Thanks

[Mr4D (OOO till 08-26), 2015-03-06 16:34:41]

I am worried that the real cause of this flakiness is still out there. If (as
discussed offline) this is a timeout issue of some kind we should fix that
instead.

I know that I have written tests which require to have waits for tasks /
messages and without them they would pass, but their passing is meaningless.

As such I would prefer a fix (or analysis) of the root cause.

[jonross, 2015-03-06 19:20:06]

Updated AshTestBase to change GestureConfiguration in order to avoid the
valgrind race conditions.

Ran ash_unittests regularly and under valgrind, no failures were introduced.

PTAL

[Mr4D (OOO till 08-26), 2015-03-06 20:48:07]

LGTM! Thanks for investigating it!

[jonross, 2015-03-09 13:49:03]

jonross@chromium.org changed reviewers:
+ oshima@chromium.org

[jonross, 2015-03-09 13:49:03]

oshima@chromium.org: Please review changes in 

ash/test/ash_test_base.cc

I've updated it to override default GestureConfiguration. As this was causing a
race on valgrind.

[oshima, 2015-03-09 22:55:57]

lgtm

https://codereview.chromium.org/968873005/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/968873005/diff/100001/ash/test/ash_test_base....
ash/test/ash_test_base.cc:137: // Changing GestureConfiguration shouldn't make
tests fail.
nit: explanation why these values are necessary would be good.

[jonross, 2015-03-10 13:40:43]

The CQ bit was checked by jonross@chromium.org

[jonross, 2015-03-10 13:40:43]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
oshima@chromium.org, skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/968873005/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-03-10 13:41:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/968873005/120001

[jonross, 2015-03-10 15:07:57]

https://codereview.chromium.org/968873005/diff/100001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/968873005/diff/100001/ash/test/ash_test_base....
ash/test/ash_test_base.cc:137: // Changing GestureConfiguration shouldn't make
tests fail.
On 2015/03/09 22:55:56, oshima wrote:
> nit: explanation why these values are necessary would be good.

Done.

[commit-bot: I haz the power, 2015-03-10 15:14:35]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-03-10 15:15:21]

Patchset 3 (id:??) landed as
https://crrev.com/7185d9d33461634fbdde9912e95189ae63623785
Cr-Commit-Position: refs/heads/master@{#319881}