Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372823003:
[Contextual Search] Add a basic test for ContextualSearchImageControl (Closed)
|
Created:
4 years, 2 months ago by Theresa Modified:
4 years, 2 months ago Reviewers:
Donn Denman CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Contextual Search] Add a basic test for ContextualSearchImageControl
BUG=644932
Committed: https://crrev.com/26f789a322bf63fb60a88b75a60be40bf81db823
Cr-Commit-Position: refs/heads/master@{#421349}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes from donnd@ review #
Messages
Total messages: 17 (11 generated)
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
twellington@chromium.org changed reviewers: + donnd@chromium.org
ptal
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 -- just a few suggestions. I like this pattern of getting sub-object to test and making assertions, seems useful going forward. Thanks for doing this! https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2659: @SmallTest Nit: Maybe add a header comment? https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2675: assertTrue(imageControl.getThumbnailVisible()); Maybe also check here that the iconSpriteControl is not visible?
The CQ bit was checked by twellington@chromium.org to run a CQ dry run
https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2659: @SmallTest On 2016/09/27 18:20:04, Donn Denman wrote: > Nit: Maybe add a header comment? Done. https://codereview.chromium.org/2372823003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:2675: assertTrue(imageControl.getThumbnailVisible()); On 2016/09/27 18:20:04, Donn Denman wrote: > Maybe also check here that the iconSpriteControl is not visible? iconSpriteControl will still be visible because the animation is running. I added a CriteriaHelper.pollUiThread() that waits for the icon to be completely hidden.
The CQ bit was unchecked by twellington@chromium.org
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
The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org Link to the patchset: https://codereview.chromium.org/2372823003/#ps20001 (title: "Changes from donnd@ review")
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 #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Add a basic test for ContextualSearchImageControl BUG=644932 ========== to ========== [Contextual Search] Add a basic test for ContextualSearchImageControl BUG=644932 Committed: https://crrev.com/26f789a322bf63fb60a88b75a60be40bf81db823 Cr-Commit-Position: refs/heads/master@{#421349} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/26f789a322bf63fb60a88b75a60be40bf81db823 Cr-Commit-Position: refs/heads/master@{#421349} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
