[TTS] Mojo-version of Tap notification is flaky, based on automated tests |
|
Issue descriptionWhen we landed the mojo version of CS Tap notification on trunk on 3/2/18, one or two tests started to fail on the KitKat Release build (crbug.com/818374), so the CL was reverted. Evidently something in the call chain for mojo notification is flaky (or causing the test to be flaky). I can occasionally reproduce the failure locally on a Kitkat Release phone. Hoping to figure out what's causing the flake, or limit the flakiness, so we can reland.
,
Mar 7 2018
The following revision WAS SUPPOSED TO refer to this bug: ------------------------------ https://chromium.googlesource.com/chromium/src.git/+/84b690feb3f2e20883f72a5bcbfd141c65372a15 commit 84b690feb3f2e20883f72a5bcbfd141c65372a15 Author: Donn Denman <donnd@google.com> Date: Wed Mar 07 02:06:10 2018 [TTS] Minor test cleanup and improvements. Poll the UI thread instead of the Intrumentation thread in some cases -- likely there are others that should be changed too. Remove my initials from test names (done for local debugging and accidently submitted). BUG= 818374 Change-Id: I5bb80c520b155be94a9c1ecfc0c9c1f3a0e4184e Reviewed-on: https://chromium-review.googlesource.com/950319 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Donn Denman <donnd@chromium.org> Cr-Commit-Position: refs/heads/master@{#541282} [modify] https://crrev.com/84b690feb3f2e20883f72a5bcbfd141c65372a15/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java ------------------------------ The above change should allow us to land the mojo-ification without too much of an impact on flakiness. I was unable to apply a good workaround to the code other than just limiting the failing test to fewer iterations and some other possibly helpful changes in this CL. I plan to reland, watch the tree, and update this bug with observations.
,
Mar 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a commit fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a Author: Donn Denman <donnd@google.com> Date: Fri Mar 09 05:08:57 2018 [TTS] Restrict tests that are flaking on KitKat. Changes some Contextual Search tests that are flaking on KitKat. Now they only run on Android-L or later. BUG=818897 Change-Id: I97b10e45b055ccc8b05dce569b44c8f987c13d90 Reviewed-on: https://chromium-review.googlesource.com/953563 Commit-Queue: Donn Denman <donnd@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Cr-Commit-Position: refs/heads/master@{#542033} [modify] https://crrev.com/fbdbb6d5c40e75d30bbbb0112b095ba2ac92f27a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
,
Mar 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5bd2e471d9abc7413907974f56b8f694fb2180bd commit 5bd2e471d9abc7413907974f56b8f694fb2180bd Author: Donn Denman <donnd@google.com> Date: Fri Mar 16 17:16:40 2018 [TTS] Test if tap flakiness is based on the thread Adds another ContextualSearchManagerTest similar to an existing test that is currently somewhat flaky to see if the flakiness is due to polling on the UI thread vs the Instrumentation thread. BUG=818897 Change-Id: Icb40f34db4de4b1d26a4dc555a3bbf0d01b1090d Reviewed-on: https://chromium-review.googlesource.com/963288 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Donn Denman <donnd@chromium.org> Cr-Commit-Position: refs/heads/master@{#543739} [modify] https://crrev.com/5bd2e471d9abc7413907974f56b8f694fb2180bd/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/721b860552205c51f38809c900558258ff61ff63 commit 721b860552205c51f38809c900558258ff61ff63 Author: Donn Denman <donnd@google.com> Date: Fri Apr 20 19:55:58 2018 [TTS] Fix Tap mojo race condition and update tests. This CL implements a workaround for the race condition between the mojo tap notification and the selection-changed notification. Now we never enter the SELECTION_CLEARED internal state unless there was a previous selection that got cleared. Before this CL every tap caused a transition to the SELECTION_CLEARED internal state even when there was no previous selection. This would usually happen before the Tap notification, but now that's faster due to mojo so it sometimes arrives beforehand. With this change we ignore selection changes from empty to empty so there's only one notification for most taps. Now we only enter the SELECTION_CLEARED state when it's really needed (in the tap near previous selection use-case). Also does some minor test cleanup and re-enabling disabled tests that were flaky. BUG=818897 Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e Reviewed-on: https://chromium-review.googlesource.com/1018300 Commit-Queue: Donn Denman <donnd@chromium.org> Reviewed-by: Theresa <twellington@chromium.org> Cr-Commit-Position: refs/heads/master@{#552439} [modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java [modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java [modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java [modify] https://crrev.com/721b860552205c51f38809c900558258ff61ff63/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
,
Apr 20 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93 commit a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93 Author: Donn Denman <donnd@chromium.org> Date: Fri Apr 20 20:34:52 2018 Revert "[TTS] Fix Tap mojo race condition and update tests." This reverts commit 721b860552205c51f38809c900558258ff61ff63. Reason for revert: Caused a few test failures: testTap and testImageControl. Original change's description: > [TTS] Fix Tap mojo race condition and update tests. > > This CL implements a workaround for the race condition between the mojo > tap notification and the selection-changed notification. Now we never enter > the SELECTION_CLEARED internal state unless there was a previous > selection that got cleared. > > Before this CL every tap caused a transition to the SELECTION_CLEARED > internal state even when there was no previous selection. This would usually > happen before the Tap notification, but now that's faster due to mojo so it > sometimes arrives beforehand. With this change we ignore selection changes > from empty to empty so there's only one notification for most taps. Now we only > enter the SELECTION_CLEARED state when it's really needed (in the tap > near previous selection use-case). > > Also does some minor test cleanup and re-enabling disabled tests that > were flaky. > > BUG=818897 > > Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e > Reviewed-on: https://chromium-review.googlesource.com/1018300 > Commit-Queue: Donn Denman <donnd@chromium.org> > Reviewed-by: Theresa <twellington@chromium.org> > Cr-Commit-Position: refs/heads/master@{#552439} TBR=donnd@chromium.org,twellington@chromium.org Change-Id: Ic84b636996d182fce0cf630d1255b142f8f7619b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 818897 Reviewed-on: https://chromium-review.googlesource.com/1022670 Reviewed-by: Donn Denman <donnd@chromium.org> Commit-Queue: Donn Denman <donnd@chromium.org> Cr-Commit-Position: refs/heads/master@{#552456} [modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java [modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java [modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java [modify] https://crrev.com/a9b6ba0f48ae5bf20b8ff91a738e3516491e8a93/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
,
Apr 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f1d3ae38a77103597152d30487d19ddd519dfd3a commit f1d3ae38a77103597152d30487d19ddd519dfd3a Author: Donn Denman <donnd@google.com> Date: Fri Apr 27 15:57:38 2018 Reland "[TTS] Fix Tap mojo race condition and update tests." This is a reland of 721b860552205c51f38809c900558258ff61ff63 I've run the tests that flaked locally for several iterations without failure, so I'm thinking the failures were due to associated flakes. Original change's description: > [TTS] Fix Tap mojo race condition and update tests. > > This CL implements a workaround for the race condition between the mojo > tap notification and the selection-changed notification. Now we never enter > the SELECTION_CLEARED internal state unless there was a previous > selection that got cleared. > > Before this CL every tap caused a transition to the SELECTION_CLEARED > internal state even when there was no previous selection. This would usually > happen before the Tap notification, but now that's faster due to mojo so it > sometimes arrives beforehand. With this change we ignore selection changes > from empty to empty so there's only one notification for most taps. Now we only > enter the SELECTION_CLEARED state when it's really needed (in the tap > near previous selection use-case). > > Also does some minor test cleanup and re-enabling disabled tests that > were flaky. > > BUG=818897 > > Change-Id: I269815ad087aabf93332e99a6c7d1208addabe3e > Reviewed-on: https://chromium-review.googlesource.com/1018300 > Commit-Queue: Donn Denman <donnd@chromium.org> > Reviewed-by: Theresa <twellington@chromium.org> > Cr-Commit-Position: refs/heads/master@{#552439} Bug: 818897 Change-Id: Ibe3034c2fd2dee8bdfb5e69aacbef6e4836ba4ad Reviewed-on: https://chromium-review.googlesource.com/1030770 Reviewed-by: Theresa <twellington@chromium.org> Commit-Queue: Donn Denman <donnd@chromium.org> Cr-Commit-Position: refs/heads/master@{#554398} [modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateController.java [modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java [modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchInternalStateControllerWrapper.java [modify] https://crrev.com/f1d3ae38a77103597152d30487d19ddd519dfd3a/chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java |
|
►
Sign in to add a comment |
|
Comment 1 by donnd@google.com
, Mar 6 2018