Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289803002:
Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. (Closed)
|
Created:
4 years, 3 months ago by Roger McFarlane (Chromium) Modified:
4 years, 3 months ago CC:
chromium-reviews, sync-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order.
BUG=620414, 640669, 642128
Committed: https://crrev.com/778c94f05935d22b8e0555616d0faaf277ad5b7a
Cr-Commit-Position: refs/heads/master@{#415430}
Patch Set 1 #Patch Set 2 : update comments #Patch Set 3 : order the item extraction sequentially #
Total comments: 6
Patch Set 4 : add tracking bug for explicit bookmark order #Messages
Total messages: 30 (21 generated)
The CQ bit was checked by rogerm@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 ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414 R=maxbogue@chromium.org, zea@chromium.org ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414 ==========
Description was changed from ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414 ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669 ==========
Please take a look? This CL makes testDownloadMovedBookmark more robust to changes in the order of the synced bookmark items. This is needed to unblock a reland of https://codereview.chromium.org/2276103003/ Thanks!
The CQ bit was checked by rogerm@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.
The CQ bit was checked by rogerm@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/2289803002/diff/40001/chrome/android/sync_she... File chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java (right): https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:163: // other a bookmark. Figure out which is which. Could you file a bug to fix this, and add a comment mentioning the bug and why we're doing it this way? https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:172: // On the server, move on the bookmark into the folder then sync, and nit: "move on" -> "move the"
Description was changed from ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669 ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 ==========
lgtm https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... File chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java (right): https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:164: assertEquals(2, getClientBookmarks().size()); nit: I know we weren't before, but can we just call getClientBookmarks once and store the list?
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by rogerm@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.
Patchset #5 (id:100001) has been deleted
Thanks! One last look? https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... File chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java (right): https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:163: // other a bookmark. Figure out which is which. On 2016/08/29 20:02:04, Nicolas Zea wrote: > Could you file a bug to fix this, and add a comment mentioning the bug and why > we're doing it this way? Done. https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:164: assertEquals(2, getClientBookmarks().size()); On 2016/08/29 21:18:09, maxbogue wrote: > nit: I know we weren't before, but can we just call getClientBookmarks once and > store the list? Done. https://codereview.chromium.org/2289803002/diff/40001/chrome/android/sync_she... chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/BookmarksTest.java:172: // On the server, move on the bookmark into the folder then sync, and On 2016/08/29 20:02:04, Nicolas Zea wrote: > nit: "move on" -> "move the" Done.
Still lgtm! zea@ is OOO all week so feel free to land this.
On 2016/08/30 20:14:10, maxbogue wrote: > Still lgtm! zea@ is OOO all week so feel free to land this. Thanks for the heads up!
The CQ bit was checked by rogerm@chromium.org
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.
Description was changed from ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 Committed: https://crrev.com/778c94f05935d22b8e0555616d0faaf277ad5b7a Cr-Commit-Position: refs/heads/master@{#415430} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/778c94f05935d22b8e0555616d0faaf277ad5b7a Cr-Commit-Position: refs/heads/master@{#415430}
Message was sent while issue was closed.
Description was changed from ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 64218 Committed: https://crrev.com/778c94f05935d22b8e0555616d0faaf277ad5b7a Cr-Commit-Position: refs/heads/master@{#415430} ========== to ========== Fix BookmarksTest#testDownloadMovedBookmark to be insensitive to item order. BUG=620414, 640669, 642128 Committed: https://crrev.com/778c94f05935d22b8e0555616d0faaf277ad5b7a Cr-Commit-Position: refs/heads/master@{#415430} ========== |
