Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826253004:
Enable all tests in CopylessPasteTest (Closed)
DescriptionEnable all tests in CopylessPasteTest
The following tests are re-enabled:
- testValid
- testNoMeta
- testCache
All flaky failures in CopylessPasteTest are due to timeout in
CallbackHelper.waitForCallback(). Wait for longer to see if it gets
better.
Reference:
https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=chrome_public_test_apk%20(with%20patch)&showAllRuns=true&tests=org.chromium.chrome.browser.CopylessPasteTest%23
BUG=713172, 713895, 713878, 693650
Review-Url: https://codereview.chromium.org/2826253004
Cr-Commit-Position: refs/heads/master@{#485865}
Committed: https://chromium.googlesource.com/chromium/src/+/0982cbaa2225d8a7a895aebaaed64be032b418a1
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase, wait for longer #Messages
Total messages: 19 (9 generated)
wychen@chromium.org changed reviewers: + mariakhomenko@chromium.org
PTAL
https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java (right): https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java:170: loadUrl(mTestServer.getURL(DATA_PAGE)); From CL description, it sounds like you think that two waits are what's causing flakiness (I find this a really odd explanation). But here you also remove the unique tag from the test. Do you think that has an effect too? I am a bit concerned that we are fixing the test by removing an expectation.
https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java (right): https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java:170: loadUrl(mTestServer.getURL(DATA_PAGE)); On 2017/04/20 21:34:30, Maria wrote: > From CL description, it sounds like you think that two waits are what's causing > flakiness (I find this a really odd explanation). But here you also remove the > unique tag from the test. Do you think that has an effect too? > > I am a bit concerned that we are fixing the test by removing an expectation. The flakiness of testNoMeta and testValid is around half of testCache, and all the failures are about wait timeout, so I think this is the most reasonable explanation. It turned out that the uniqueTag is not needed since each test begins with a clean cache. It shouldn't affect the flakiness. The waits here are a bit redundant, since these expectations were checked in other tests (testNoMeta and testValid respectively). The cache-specific expectation is that repeated URLs don't get callback. I keep one wait for readability.
On 2017/04/20 22:00:54, wychen wrote: > https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java > (right): > > https://codereview.chromium.org/2826253004/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/CopylessPasteTest.java:170: > loadUrl(mTestServer.getURL(DATA_PAGE)); > On 2017/04/20 21:34:30, Maria wrote: > > From CL description, it sounds like you think that two waits are what's > causing > > flakiness (I find this a really odd explanation). But here you also remove the > > unique tag from the test. Do you think that has an effect too? > > > > I am a bit concerned that we are fixing the test by removing an expectation. > > The flakiness of testNoMeta and testValid is around half of testCache, and all > the failures are about wait timeout, so I think this is the most reasonable > explanation. > > It turned out that the uniqueTag is not needed since each test begins with a > clean cache. It shouldn't affect the flakiness. > > The waits here are a bit redundant, since these expectations were checked in > other tests (testNoMeta and testValid respectively). > The cache-specific expectation is that repeated URLs don't get callback. > I keep one wait for readability. This concerns me, because I don't think the flakiness needs to be halved. I think all the test cases need to not be flaky (including the ones that are flaking more rarely). Can we figure out what's causing the timeouts to occur?
On 2017/04/20 22:38:26, Maria wrote: > This concerns me, because I don't think the flakiness needs to be halved. I > think all the test cases need to not be flaky (including the ones that are > flaking more rarely). Can we figure out what's causing the timeouts to occur? That's what I planned to do, but my local run is 400/400 passed. I'll have another try.
On 2017/04/21 01:19:58, wychen wrote: > On 2017/04/20 22:38:26, Maria wrote: > > This concerns me, because I don't think the flakiness needs to be halved. I > > think all the test cases need to not be flaky (including the ones that are > > flaking more rarely). Can we figure out what's causing the timeouts to occur? > > That's what I planned to do, but my local run is 400/400 passed. > I'll have another try. lgtm Consider running on an older phone -- they tend to show the problems more. But if everything passes, then feel free to submit.
The CQ bit was checked by wychen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable test CopylessPasteTest#testCache All flaky failures in CopylessPasteTest are due to timeout in CallbackHelper.waitForCallback(). testValid() and testNoMeta() only waits once, while testCache() waits twice, which makes it more flaky. Changed to wait once as well. Reference: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... BUG=713172,693650 ========== to ========== Enable all tests in CopylessPasteTest The following tests are re-enabled: - testValid - testNoMeta - testCache All flaky failures in CopylessPasteTest are due to timeout in CallbackHelper.waitForCallback(). Wait for longer to see if it gets better. Reference: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... BUG=713172,713895,713878,693650 ==========
I'll re-enabled all the tests with 2X timeout threshold.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by wychen@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499835955749190,
"parent_rev": "dce252aba6252439025768325887acaa44340eda", "commit_rev":
"0982cbaa2225d8a7a895aebaaed64be032b418a1"}
Message was sent while issue was closed.
Description was changed from ========== Enable all tests in CopylessPasteTest The following tests are re-enabled: - testValid - testNoMeta - testCache All flaky failures in CopylessPasteTest are due to timeout in CallbackHelper.waitForCallback(). Wait for longer to see if it gets better. Reference: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... BUG=713172,713895,713878,693650 ========== to ========== Enable all tests in CopylessPasteTest The following tests are re-enabled: - testValid - testNoMeta - testCache All flaky failures in CopylessPasteTest are due to timeout in CallbackHelper.waitForCallback(). Wait for longer to see if it gets better. Reference: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType... BUG=713172,713895,713878,693650 Review-Url: https://codereview.chromium.org/2826253004 Cr-Commit-Position: refs/heads/master@{#485865} Committed: https://chromium.googlesource.com/chromium/src/+/0982cbaa2225d8a7a895aebaaed6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/0982cbaa2225d8a7a895aebaaed6... |
