Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2238083003:
Use CallbackHelpers in TabStripTest (Closed)
|
Created:
4 years, 4 months ago by Theresa Modified:
4 years, 4 months ago Reviewers:
Changwan Ryu CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse CallbackHelpers in TabStripTest
Also, run more methods that obtain StripLayoutTab state on the UI
thread rather than the test thread and disable animations for
testScrollingStripStackerTabOffsets.
BUG=632843
Committed: https://crrev.com/af4de58dba85c0556a4ca0716c8154e8dbfcfde7
Cr-Commit-Position: refs/heads/master@{#411722}
Patch Set 1 #Patch Set 2 : Update some comments #
Total comments: 16
Patch Set 3 : Changes from changwan@ review #Patch Set 4 : Remove StripLayoutHelper observer #
Messages
Total messages: 22 (14 generated)
The CQ bit was checked by twellington@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...
The CQ bit was checked by twellington@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...
twellington@chromium.org changed reviewers: + changwan@chromium.org
ptal The re-enabled test I've run 30 times already locally w/ no flakes. The other tests in the suite I ran x2 without flakes. I'm going to run x50 over night to make sure these changes are indeed robust.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:60: public interface StripLayoutHelperObserver { how about using just "Observer"? https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:1184: observer.onStripUpdated(); could we pass onStripUpdated(isAnimating)? Then I assume we don't need to disable animations just for testing. (BTW, you'll need to make sure that isAnimating is true when animation starts...) https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTab.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTab.java:39: public interface StripLayoutTabObserver { Hmm... Just "Observer" should be fine, I think. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:509: tab.addObserver(new StripLayoutTabObserver() { How about new StripLayoutTab.Observer? https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:662: throws InterruptedException, ExecutionException, TimeoutException { How about throws Exception? (and in other places as well) https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:749: public void onStripUpdated() { Instead of deviating the behavior just for testing, how about checking the animation status here? https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:764: stripUpdatedHelper.waitForCallback(currentCallCount); Hmm.. I feel that we are leaving a possibility that onStripUpdated() gets called *after* waitForCallback(0) is called and *before* getCallCount() is called. How about just using 0, 1, 2, 3 assuming it's called only once per wait. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:777: // Wait for the first tab to no longer be visible. s/tab/tab strip?
https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelper.java:60: public interface StripLayoutHelperObserver { On 2016/08/12 04:59:21, Changwan Ryu wrote: > how about using just "Observer"? Done. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTab.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutTab.java:39: public interface StripLayoutTabObserver { On 2016/08/12 04:59:22, Changwan Ryu wrote: > Hmm... Just "Observer" should be fine, I think. Done. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:509: tab.addObserver(new StripLayoutTabObserver() { On 2016/08/12 04:59:22, Changwan Ryu wrote: > How about new StripLayoutTab.Observer? Done. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:662: throws InterruptedException, ExecutionException, TimeoutException { On 2016/08/12 04:59:22, Changwan Ryu wrote: > How about throws Exception? (and in other places as well) Done. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:749: public void onStripUpdated() { On 2016/08/12 04:59:22, Changwan Ryu wrote: > Instead of deviating the behavior just for testing, how about checking the > animation status here? I don't think updateStrip() gets called again when the animation is finished (right? or does it get called multiple times while the scroll is animating?). What I'm trying to test is that the tab x offsets get calculated correctly for different scroll offset positions, which is hard to do if the scroll offset is animating. Disabling animations for this test seems cleaner and less likely to result in flakes. I could try to add a new observer method/callback for when the scroll animation finishes, but I think that's going to add extra unnecessary code complexity and a fair amount of plumbing that's not used for anything other this one test. The animation is controlled by StackScroller, which doesn't currently notify anyone when the animations are finished. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:764: stripUpdatedHelper.waitForCallback(currentCallCount); On 2016/08/12 04:59:22, Changwan Ryu wrote: > Hmm.. I feel that we are leaving a possibility that onStripUpdated() gets called > *after* waitForCallback(0) is called and *before* getCallCount() is called. How > about just using 0, 1, 2, 3 assuming it's called only once per wait. Done. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:777: // Wait for the first tab to no longer be visible. On 2016/08/12 04:59:22, Changwan Ryu wrote: > s/tab/tab strip? It's the first tab in the strip. Updated the comment to be more explicit.
The CQ bit was checked by twellington@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm assuming onStripUpdate() and related callback helpers are removed. https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java (right): https://codereview.chromium.org/2238083003/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/compositor/overlays/strip/TabStripTest.java:749: public void onStripUpdated() { On 2016/08/12 06:07:12, Theresa Wellington wrote: > On 2016/08/12 04:59:22, Changwan Ryu wrote: > > Instead of deviating the behavior just for testing, how about checking the > > animation status here? > > I don't think updateStrip() gets called again when the animation is finished > (right? or does it get called multiple times while the scroll is animating?). > What I'm trying to test is that the tab x offsets get calculated correctly for > different scroll offset positions, which is hard to do if the scroll offset is > animating. Disabling animations for this test seems cleaner and less likely to > result in flakes. > > I could try to add a new observer method/callback for when the scroll animation > finishes, but I think that's going to add extra unnecessary code complexity and > a fair amount of plumbing that's not used for anything other this one test. The > animation is controlled by StackScroller, which doesn't currently notify anyone > when the animations are finished. Ok, I'm fine with reducing the test complexity, as long as production code is well covered. FYI, the current structure using CallbackHelper for onStripUpdate() is not needed at all because it is a synchronous blocking call. You may want to remove those callback helpers.
On 2016/08/12 10:41:18, Changwan Ryu wrote: > Ok, I'm fine with reducing the test complexity, as long as production code is > well covered. > > FYI, the current structure using CallbackHelper for onStripUpdate() is not > needed at all because it is a synchronous blocking call. You may want to remove > those callback helpers. A number of other tests set the scroll position and check tab strip state, and #testScrollingStripStackerScrollsToSelectedTab explicitly checks the auto-scroll behavior so production code is well covered. Thanks for catching that, removed the callback helpers.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from changwan@chromium.org Link to the patchset: https://codereview.chromium.org/2238083003/#ps60001 (title: "Remove StripLayoutHelper observer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use CallbackHelpers in TabStripTest Also, run more methods that obtain StripLayoutTab state on the UI thread rather than the test thread and disable animations for testScrollingStripStackerTabOffsets. BUG=632843 ========== to ========== Use CallbackHelpers in TabStripTest Also, run more methods that obtain StripLayoutTab state on the UI thread rather than the test thread and disable animations for testScrollingStripStackerTabOffsets. BUG=632843 Committed: https://crrev.com/af4de58dba85c0556a4ca0716c8154e8dbfcfde7 Cr-Commit-Position: refs/heads/master@{#411722} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/af4de58dba85c0556a4ca0716c8154e8dbfcfde7 Cr-Commit-Position: refs/heads/master@{#411722} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
