Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1516323003:
[Contextual Search] Update testTapOnARIAIgnored. (Closed)
|
Created:
5 years ago by Donn Denman Modified:
4 years, 11 months ago Reviewers:
Theresa CC:
chromium-reviews, pedro (no code reviews), mdjones Base URL:
https://chromium.googlesource.com/chromium/src.git@6t Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Reenable testTapOnARIAIgnored.
When this test was disabled, we never got a good read on
what was causing it to fail (bot just timed out).
However, during code review Pedro noted that this is not
a good test because it might miss failures that happen after the test exits.
The test has been updated to catch failures that happen within
a reasonable window, so everything should work consistently now.
I'm reenabling after verifying that everything works solidly
on both phone and tablet. If not, please re-disable
and update this bug.
BUG=542874
Committed: https://crrev.com/e780e1e501283bb8a13d9aeb4bb5bb43600c849f
Cr-Commit-Position: refs/heads/master@{#369506}
Patch Set 1 #Patch Set 2 : Trivial (no longer dependent on other patches). #
Total comments: 2
Patch Set 3 : Improved tests to monitor the panel when activity is not expected. #Patch Set 4 : Updated description. #
Total comments: 3
Messages
Total messages: 21 (9 generated)
Trivial (no longer dependent on other patches).
donnd@chromium.org changed reviewers: + pedrosimonetti@chromium.org, twellington@chromium.org
Theresa and Pedro, PTAL and let me know if you this test could be flaky for some reason. It did flake and blocked jam@, who wrote: "There were at least 6 instances where this test caused a try run to fail. i.e. it failed 4 times in a row in each try run. This doesn't seem like an anomaly. Please investigate the test closely first." My plan is to figure out a semi-safe way to land these "reenable" CLs, e.g. wait for low-traffic time right before the holidays, want the tree for a day or two after, etc.
https://chromiumcodereview.appspot.com/1516323003/diff/20001/chrome/android/j... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1516323003/diff/20001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1461: clickNode("aria"); This one seems odd (since all the other really simple ones pass). The timeout was likely just the CriteriaHelper.pollForCriteria timing out. It'd be nice if we still had logs to know what state the panel thought it was in. Maybe CLOSED? Same question for repeat=50 on this test. I think if we can't repro the failure locally, run it through the trybots a handful of times to see if we can get a flake there, and if not, land this and keep a close eye out for flakes to revert if necessary. It's also totally possible we had a bug in our code however long ago that caused the failure and that the issue has since been fixed.
https://codereview.chromium.org/1516323003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1516323003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1462: assertPanelNeverOpened(); I'm not sure if this test is doing the right thing. We are not waiting for the click to be processed by our code. Suppose we had a bug in our code that would make it trigger on elements with ARIA roles (situation that should not trigger CS). If we don't wait for the gesture processing this test might still pass since right after clicking on a node nothing happens. The Panel opens later, after selecting the word. Am I missing something?
On 2015/12/17 02:43:13, pedrosimonetti wrote: > https://codereview.chromium.org/1516323003/diff/20001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java > (right): > > https://codereview.chromium.org/1516323003/diff/20001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:1462: > assertPanelNeverOpened(); > I'm not sure if this test is doing the right thing. We are not waiting for the > click to be processed by our code. Suppose we had a bug in our code that would > make it trigger on elements with ARIA roles (situation that should not trigger > CS). > > If we don't wait for the gesture processing this test might still pass since > right after clicking on a node nothing happens. The Panel opens later, after > selecting the word. > > Am I missing something? That's a good point Pedro! This test seems like it's flawed because it might pass cases that really fail. I'll have to think about that and try to address it... eventually.
Description was changed from ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). I'm reenabling with the hope that the test will work solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ========== to ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). I'm reenabling with the hope that the test will work solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ==========
donnd@chromium.org changed reviewers: + mdjones@chromium.org
Description was changed from ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). I'm reenabling with the hope that the test will work solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ========== to ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exists. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ==========
donnd@chromium.org changed reviewers: - mdjones@chromium.org, pedrosimonetti@chromium.org
Theresa, PTAL. I think we can reenable this test with these changes, though the original cause of failure is still unknown.
lgtm % 1 comment - ran these locally on my N5 and N7 and they all pass https://chromiumcodereview.appspot.com/1516323003/diff/60001/chrome/android/j... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1516323003/diff/60001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:615: didChangeState = mPanel.getPanelState() != initialState; Do we want to break if didChangeState is true so that we fail more quickly?
Thanks Theresa! https://chromiumcodereview.appspot.com/1516323003/diff/60001/chrome/android/j... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://chromiumcodereview.appspot.com/1516323003/diff/60001/chrome/android/j... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:615: didChangeState = mPanel.getPanelState() != initialState; On 2016/01/13 23:15:12, Theresa Wellington wrote: > Do we want to break if didChangeState is true so that we fail more quickly? That's already being done by the "while" condition (unless I'm missing something).
https://codereview.chromium.org/1516323003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1516323003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:615: didChangeState = mPanel.getPanelState() != initialState; On 2016/01/13 23:41:27, Donn Denman wrote: > On 2016/01/13 23:15:12, Theresa Wellington wrote: > > Do we want to break if didChangeState is true so that we fail more quickly? > > That's already being done by the "while" condition (unless I'm missing > something). Nope, my mistake, I didn't read the while closely enough.
The CQ bit was checked by donnd@chromium.org
Description was changed from ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exists. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ========== to ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exits. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1516323003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516323003/60001
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exits. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ========== to ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exits. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exits. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 ========== to ========== [Contextual Search] Reenable testTapOnARIAIgnored. When this test was disabled, we never got a good read on what was causing it to fail (bot just timed out). However, during code review Pedro noted that this is not a good test because it might miss failures that happen after the test exits. The test has been updated to catch failures that happen within a reasonable window, so everything should work consistently now. I'm reenabling after verifying that everything works solidly on both phone and tablet. If not, please re-disable and update this bug. BUG=542874 Committed: https://crrev.com/e780e1e501283bb8a13d9aeb4bb5bb43600c849f Cr-Commit-Position: refs/heads/master@{#369506} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e780e1e501283bb8a13d9aeb4bb5bb43600c849f Cr-Commit-Position: refs/heads/master@{#369506} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
