Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800763002:
Adjust BookmarkTest for large devices (Closed)
DescriptionAdjust BookmarkTest for large devices
This change makes sure the divider is taken into account
when counting items in BookmarkTest#testSearchBookmarks.
BUG=708585
Review-Url: https://codereview.chromium.org/2800763002
Cr-Commit-Position: refs/heads/master@{#462287}
Committed: https://chromium.googlesource.com/chromium/src/+/3ca8a360ea076f91656d102229049c0fee2b46be
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed Theresa's comments #Messages
Total messages: 20 (10 generated)
kraush@amazon.com changed reviewers: + twellington@chromium.org
Hi Theresa, Can you take a look at this test fix for large devices like Nexus 10? Thanks! :) Holger
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...
https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java (right): https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:240: private void assertBookmarkItems(final String errorMessage, final int expectedOnSmallTablets, nit: exepectedOnRegularDevice? Or something like that. It could be a phone or a small tablet. https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:242: final int expectedCount = DeviceFormFactor.isLargeTablet(getActivity()) Please add a: TODO(twellington): Remove after bookmarks redesign is complete. In the next (hopefully) month or so there will no longer be a split in UI design for large tablets. https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:244: ? expectedOnSmallTablets + 1 Add a comment about what the +1 is for.
https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java (right): https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:240: private void assertBookmarkItems(final String errorMessage, final int expectedOnSmallTablets, On 2017/04/05 15:29:21, Theresa wrote: > nit: exepectedOnRegularDevice? Or something like that. It could be a phone or a > small tablet. Will change! https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:242: final int expectedCount = DeviceFormFactor.isLargeTablet(getActivity()) On 2017/04/05 15:29:21, Theresa wrote: > Please add a: > > TODO(twellington): Remove after bookmarks redesign is complete. > > > In the next (hopefully) month or so there will no longer be a split in UI design > for large tablets. Will do! :) I saw the original TODO that causes the divider to be added, but wasn't sure on how to link them. https://codereview.chromium.org/2800763002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/bookmarks/BookmarkTest.java:244: ? expectedOnSmallTablets + 1 On 2017/04/05 15:29:21, Theresa wrote: > Add a comment about what the +1 is for. Will do!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Addressed the first div's comments.
Please note: I don't think the Mac and IOS failures on the dryrun are related to my change. This file should not be compiled in on Mac, and even the "compile without change" step failed with the same GN error. Android builders passed.
lgtm
The CQ bit was checked by kraush@amazon.com
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kraush@amazon.com
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": 1491435808290240,
"parent_rev": "66d34a387594b54f3fb22a30cb0fd57ff972a002", "commit_rev":
"3ca8a360ea076f91656d102229049c0fee2b46be"}
Message was sent while issue was closed.
Description was changed from ========== Adjust BookmarkTest for large devices This change makes sure the divider is taken into account when counting items in BookmarkTest#testSearchBookmarks. BUG=708585 ========== to ========== Adjust BookmarkTest for large devices This change makes sure the divider is taken into account when counting items in BookmarkTest#testSearchBookmarks. BUG=708585 Review-Url: https://codereview.chromium.org/2800763002 Cr-Commit-Position: refs/heads/master@{#462287} Committed: https://chromium.googlesource.com/chromium/src/+/3ca8a360ea076f91656d10222904... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/3ca8a360ea076f91656d10222904... |
