Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
929523002:
Add tests to EnhancedBookmarksModel (Closed)
DescriptionAdd tests to EnhancedBookmarksModel
Enhanced bookmark model is the data abstraction layer of enhanced bookmark.
This CL also fixes the bug that add folder returns a wrong bookmarkid.
BUG=458632, 458703
Committed: https://crrev.com/ad8f1cade54237959861511e7a0f58c5520b5224
Cr-Commit-Position: refs/heads/master@{#318718}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Split this CL to two smaller ones to make reviewing easier #
Total comments: 14
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 1
Patch Set 5 : Removed one todo #
Messages
Total messages: 17 (4 generated)
ianwen@chromium.org changed reviewers: + kkimlabs@chromium.org
(didn't look other parts yet) https://codereview.chromium.org/929523002/diff/1/chrome/browser/enhanced_book... File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/929523002/diff/1/chrome/browser/enhanced_book... chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:208: env, new_node->id(), BookmarkType::BOOKMARK_TYPE_NORMAL); Since this should be merged, wouldn't it be easier to make this a separate CL? Also we'll need a proper bug for merge-request.
ianwen@chromium.org changed reviewers: + tedchoc@chromium.org
BookmarksBridgeTest is deleted because the original bridge and the enhanced bridge are used together, it does not make sense to test them separately. https://codereview.chromium.org/929523002/diff/1/chrome/android/java/src/org/... File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java (right): https://codereview.chromium.org/929523002/diff/1/chrome/android/java/src/org/... chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:486: public BookmarkId addFolder(BookmarkId parent, int index, String title) { These two functions are only used in SigninTest. Mark it to be VisibleForTesting in case it is striped. https://codereview.chromium.org/929523002/diff/1/chrome/browser/enhanced_book... File chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc (right): https://codereview.chromium.org/929523002/diff/1/chrome/browser/enhanced_book... chrome/browser/enhanced_bookmarks/android/enhanced_bookmarks_bridge.cc:208: env, new_node->id(), BookmarkType::BOOKMARK_TYPE_NORMAL); On 2015/02/13 23:26:26, Kibeom Kim wrote: > Since this should be merged, wouldn't it be easier to make this a separate CL? > Also we'll need a proper bug for merge-request. crbug.com/458703 I will have a specific CL on M41.
In the second patch I made the CL significantly smaller to facilitate reviewing. The old tests are retained and will be removed later, if necessary.
mostly minor comments https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:26: * nit: unnecessary line? https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java (right): https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:139: HashSet<BookmarkId> movedBookmarks = new HashSet<BookmarkId>(4); nit: why 4 here? https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:173: new HashSet<BookmarkId>(expectedChildren)); Do we want to add folderAA here too? https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:182: verifyBookmark(bookmarkA, "a", "http://a.com/", false, mDesktopNode); nit: let's put empty lines after each verifyBookmark, I think that will be easier to read. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:199: // Moved from BookmarksBridgeTest nit: remove the comment. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:210: private void verifyBookmarkWithDescription(BookmarkId idToVerify, String expectedTitle, I think the other way around naming is safer to use. Something like verifyBookmarkExceptDescription, at #200. Or actually, do we need the verifyBookmark function at #200? How about using just one function and pass null or "" if we're not interested in description. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:223: private void verifyBookmarkListNoOrder(List<BookmarkId> listToVerify, Let's use more explicit name, something like verifyBookmarkIdListNoOrder. And I'd just copy |expectedIds| instead of the warning comment, since this is not performance critical.
https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java (right): https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:139: HashSet<BookmarkId> movedBookmarks = new HashSet<BookmarkId>(4); On 2015/02/17 19:52:50, Kibeom Kim wrote: > nit: why 4 here? Changed to 6. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:173: new HashSet<BookmarkId>(expectedChildren)); On 2015/02/17 19:52:49, Kibeom Kim wrote: > Do we want to add folderAA here too? It's in #170 lol. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:182: verifyBookmark(bookmarkA, "a", "http://a.com/", false, mDesktopNode); On 2015/02/17 19:52:49, Kibeom Kim wrote: > nit: let's put empty lines after each verifyBookmark, I think that will be > easier to read. Done. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:199: // Moved from BookmarksBridgeTest On 2015/02/17 19:52:49, Kibeom Kim wrote: > nit: remove the comment. Done. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:210: private void verifyBookmarkWithDescription(BookmarkId idToVerify, String expectedTitle, On 2015/02/17 19:52:49, Kibeom Kim wrote: > I think the other way around naming is safer to use. Something like > verifyBookmarkExceptDescription, at #200. Or actually, do we need the > verifyBookmark function at #200? How about using just one function and pass null > or "" if we're not interested in description. Done. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:223: private void verifyBookmarkListNoOrder(List<BookmarkId> listToVerify, On 2015/02/17 19:52:49, Kibeom Kim wrote: > Let's use more explicit name, something like verifyBookmarkIdListNoOrder. And > I'd just copy |expectedIds| instead of the warning comment, since this is not > performance critical. Done. https://codereview.chromium.org/929523002/diff/20001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:223: private void verifyBookmarkListNoOrder(List<BookmarkId> listToVerify, On 2015/02/17 19:52:49, Kibeom Kim wrote: > Let's use more explicit name, something like verifyBookmarkIdListNoOrder. And > I'd just copy |expectedIds| instead of the warning comment, since this is not > performance critical. Done.
https://codereview.chromium.org/929523002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java (right): https://codereview.chromium.org/929523002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:210: assertEquals(item.getParentId(), expectedParent); Expected is the first argument for assertEquals. here and #208.
https://codereview.chromium.org/929523002/diff/40001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java (right): https://codereview.chromium.org/929523002/diff/40001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/enhanced_bookmarks/EnhancedBookmarksModelTest.java:210: assertEquals(item.getParentId(), expectedParent); On 2015/02/24 00:23:10, Kibeom Kim wrote: > Expected is the first argument for assertEquals. here and #208. Done.
lgtm
lgtm https://codereview.chromium.org/929523002/diff/60001/chrome/android/javatests... File chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java (right): https://codereview.chromium.org/929523002/diff/60001/chrome/android/javatests... chrome/android/javatests/src/org/chromium/chrome/browser/BookmarksBridgeTest.java:28: // TODO(ianwen): move all tests here to EnhancedBookmarksModelTest. I don't know about this TODO. As long as we have the BookmarksBridge class, we should have tests that only test that class. Testing transitively via the EnhancedBookmarksBridge is a bit scarier.
The CQ bit was checked by ianwen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, kkimlabs@chromium.org Link to the patchset: https://codereview.chromium.org/929523002/#ps80001 (title: "Removed one todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/929523002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ad8f1cade54237959861511e7a0f58c5520b5224 Cr-Commit-Position: refs/heads/master@{#318718} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
