Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2232383002:
[TTS] Delay instrumentation tests at startup for flakes. (Closed)
|
Created:
4 years, 4 months ago by Donn Denman Modified:
4 years, 4 months ago CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[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
Committed: https://crrev.com/52a70a9106d675b556cd83dc2523f126aff86687
Cr-Commit-Position: refs/heads/master@{#411425}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved control of the delay to CS, new method in test base rather than new behavior. #
Total comments: 2
Patch Set 3 : Delay should go *after* startup. #Patch Set 4 : Moved the entire change to CS, no change in the base test infra needed. #Patch Set 5 : Just updated a comment. #Messages
Total messages: 19 (7 generated)
donnd@chromium.org changed reviewers: + wnwen@chromium.org
Peter, PTAL. THANKS!
https://chromiumcodereview.appspot.com/2232383002/diff/1/chrome/test/android/... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://chromiumcodereview.appspot.com/2232383002/diff/1/chrome/test/android/... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:476: if (url != null) Thread.sleep(ACTIVITY_STARTUP_DELAY_MS); Are other tests also being flaky? I think this is only necessary for tests that immediately interact with the DOM. Perhaps a separate method "startMainActivityFromIntentWithDelay" would be better? That way other tests that don't need this legacy delay won't be slowed down.
Thanks, will try that asap... https://chromiumcodereview.appspot.com/2232383002/diff/1/chrome/test/android/... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java (right): https://chromiumcodereview.appspot.com/2232383002/diff/1/chrome/test/android/... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:476: if (url != null) Thread.sleep(ACTIVITY_STARTUP_DELAY_MS); On 2016/08/11 20:14:21, Peter Wen wrote: > Are other tests also being flaky? I think this is only necessary for tests that > immediately interact with the DOM. Perhaps a separate method > "startMainActivityFromIntentWithDelay" would be better? That way other tests > that don't need this legacy delay won't be slowed down. That's probably better, will try.
Description was changed from ========== Add a delay in instrumentation tests at startup for flakes. New startup timing causes flaky tests due to higher performance. This CL introduces a workaround that puts the delay back in the instrumentation-test-startup sequence when there's a page being loaded. Also removes an attempted workaround that failed. BUG=635661 ========== to ========== Add a delay in instrumentation tests at startup for flakes. New startup timing causes flaky tests due to higher performance. This CL introduces a workaround that allows adding a delay in the instrumentation-test-startup sequence when there's a page being loaded. Contextual Search instrumentation tests now use the legacy delay. Also removes an attempted workaround for TTS that failed. BUG=635661 ==========
Peter, PTAL.
donnd@chromium.org changed reviewers: + twellington@chromium.org
Peter, Theresa, PTAL.
https://chromiumcodereview.appspot.com/2232383002/diff/20001/chrome/android/j... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/2232383002/diff/20001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:94: private static final int ACTIVITY_STARTUP_DELAY_MS = 1000; Can we add a TODO note to fix this more correctly? Adding a 1 second startup delay seems like a band aid and I suspect we'll still see flakes, just not as frequently. https://chromiumcodereview.appspot.com/2232383002/diff/20001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:226: startMainActivityWithDelay(mTestServer.getURL(TEST_PAGE), ACTIVITY_STARTUP_DELAY_MS); I have a preference for putting the delay here instead of adding a method to TestCaseBase. I'd like to prevent other tests from building in a sleep (better to wait for specific initialization events).
Description was changed from ========== Add a delay in instrumentation tests at startup for flakes. New startup timing causes flaky tests due to higher performance. This CL introduces a workaround that allows adding a delay in the instrumentation-test-startup sequence when there's a page being loaded. Contextual Search instrumentation tests now use the legacy delay. Also removes an attempted workaround for TTS that failed. BUG=635661 ========== to ========== [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 ==========
Peter and Theresa, PTAL.
lgtm
The CQ bit was checked by donnd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Committed: https://crrev.com/52a70a9106d675b556cd83dc2523f126aff86687 Cr-Commit-Position: refs/heads/master@{#411425} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/52a70a9106d675b556cd83dc2523f126aff86687 Cr-Commit-Position: refs/heads/master@{#411425} |
