Closed Bug 1423137 Opened 7 years ago Closed 7 years ago

Add automated test for "Bookmarks can be removed from the Library"

Categories

(Firefox :: Bookmarks & History, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: icrisan, Assigned: icrisan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 2 obsolete files)

testrail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4100 Preconditions: A dirty profile with a reasonable amount of browsing history and bookmarks. Steps: 1. Launch Firefox 2. Open the Library from the Bookmarks Menu by accessing Show All Bookmarks 3. Right-click any of the bookmarks displayed in the Library and select Delete Expected results: Step 1: Firefox is successfully launched. Step 2: The Library window is opened, with All Bookmarks selected by default. Step 3: The bookmark is successfully removed.
Attached patch library_remove_bookmark (obsolete) — Splinter Review
Attachment #8934492 - Flags: review?(standard8)
Comment on attachment 8934492 [details] [diff] [review] library_remove_bookmark Review of attachment 8934492 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, it is always nice to get new tests. There's a few comments here, but I think most of them should be quite simple to resolve. ::: browser/components/places/tests/browser/browser.ini @@ +15,4 @@ > [browser_addBookmarkForFrame.js] > [browser_bookmark_folder_moveability.js] > [browser_bookmarklet_windowOpen.js] > +[browser_library_remove_bookmark.js] Please put this alongside the rest of the browser_library_* tests, preferably in alphabetical-ish order. ::: browser/components/places/tests/browser/browser_library_remove_bookmark.js @@ +2,5 @@ > + * Test deleting bookmarks from library > + */ > +"use strict"; > + > +add_task(async function test_tags() { Nit: need to update the test name here. @@ +4,5 @@ > +"use strict"; > + > +add_task(async function test_tags() { > + const uris = [ > + Services.io.newURI("http://example.com/1"), We don't need the Services.io.newURI wrappers here - insertTree can take just strings. @@ +9,5 @@ > + Services.io.newURI("http://example.com/2"), > + Services.io.newURI("http://example.com/3"), > + ]; > + > + let children = uris.map((uri, index, arr) => { No need for the arr parameter here. @@ +22,5 @@ > + guid: PlacesUtils.bookmarks.unfiledGuid, > + children > + }); > + > + // Open the Library and select the "UnfilledBookmarks". nit: spelling "UnfilledBookmarks" > "UnfiledBookmarks" @@ +27,5 @@ > + let library = await promiseLibrary("UnfiledBookmarks"); > + > + let PO = library.PlacesOrganizer; > + > + Assert.equal(PO._places.selectedNode.title, "Other Bookmarks", "Other Bookmarks selected"); I think it would be better to use this form: Assert.equal(PlacesUtils.getConcreteItemGuid(PO._places.selectedNode), PlacesUtils.bookmarks.unfiledGuid, "Should have selected unfiled bookmarks."); That means we're not depending on the title string which could change (or be different in builds for different locales). @@ +31,5 @@ > + Assert.equal(PO._places.selectedNode.title, "Other Bookmarks", "Other Bookmarks selected"); > + > + // Make sure tree matches bookmarks > + let tree = await PlacesUtils.promiseBookmarksTree(PlacesUtils.bookmarks.unfiledGuid); > + Assert.equal(children.length, tree.children.length, "Bookmark length matches that in tree."); I'm not convinced we need this part of the test - this is just checking that what is in the database is what we inserted via insertTree, and we have plenty of other tests for that. @@ +37,5 @@ > + Assert.equal(children[i].title, tree.children[i].title, > + `Bookmark title ${children[i].title} matches ${tree.children[i].title}`); > + } > + > + Assert.equal(PlacesUtils.getBookmarksForURI(children[0].url, {}).length, 1); I'm not sure what the intention of this equals is, I think it can probably be dropped. @@ +48,5 @@ > + let treeBoxObject = library.ContentTree.view.treeBoxObject; > + let firstColumn = library.ContentTree.view.columns[0]; > + let firstBookmarkRect = treeBoxObject.getCoordsForCellItem(0, firstColumn, "bm0"); > + > + EventUtils.synthesizeMouse( Using synthesizeMouse seems ok here, but please run it through try server on all platforms (opt + debug) before we land this. Normally we just call the command directly, but I'm also ok with doing it this way for a slightly more comprehensive test. @@ +61,5 @@ > + > + EventUtils.synthesizeMouseAtCenter(contextMenuDeleteItem, {}, library); > + > + await waitForCondition( > + () => PlacesUtils.getBookmarksForURI(children[0].url, {}).length === 0, This is really a deprecated API that we shouldn't be using. The better way to check for this would be to wait for the notification for the OnItemRemoved, e.g. ``` let removePromise = PlacesTestUtils.waitForNotification("onItemRemoved", itemId, parentId, index, type, uri, guid) => uri == uris[0]); EventUtils.synthesizeMouseAtCenter(contextMenuDeleteItem, {}, library); await removePromise; ``` Note that we set the wait up before the action takes place, as this helps to avoid issues with intermittents. @@ +63,5 @@ > + > + await waitForCondition( > + () => PlacesUtils.getBookmarksForURI(children[0].url, {}).length === 0, > + "First bookmark has been deleted" > + ); For added bonus, I think you could test what is displayed in the library, something like: Assert.equal(library.ContentTree.view.result.root.childCount, 2, "should have removed the bookmark from the display"); (this length could be tested before removal as well).
Attachment #8934492 - Flags: review?(standard8)
Comment on attachment 8935770 [details] [diff] [review] library_remove_bookmark_1 Review of attachment 8935770 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for updating it.
Attachment #8935770 - Flags: review?(standard8) → review+
Keywords: checkin-needed
This needs a rebased patch. Also, please update your commit information so that there's a space between your name and email. Thanks!
Flags: needinfo?(icrisan)
Keywords: checkin-needed
Priority: -- → P2
(In reply to Ryan VanderMeulen [:RyanVM] from comment #5) > This needs a rebased patch. Also, please update your commit information so > that there's a space between your name and email. Thanks! Updated my commit information, pushed the patch to try and got the following results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bad1c2d8d99ce23693323b23a8c534447f37b603&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified. From the results I noticed that the test from this patch(browser_library_remove_bookmark.js) and another test(browser_library_add_tags.js) fail on windows10-64-ccov - debug. I skipped both tests from running on windows10-64-ccov because I saw that there is already logged an issue(https://bugzilla.mozilla.org/show_bug.cgi?id=1423667) for this problem. After the above changes the try results look like this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c28ade83c0aa1cd9ab91585e0034941745b01af7 If this patch is approved should we close also this issue https://bugzilla.mozilla.org/show_bug.cgi?id=1424793?
Flags: needinfo?(icrisan)
Attachment #8935770 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Ioana Crisan from comment #6) > If this patch is approved should we close also this issue > https://bugzilla.mozilla.org/show_bug.cgi?id=1424793? Thanks for picking that up, yes we can close that issue. Bug 1423667 can take up the proper fix there.
Blocks: 1424793
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec28a490365c Add a test for checking that bookmarks can be removed from the library window. r=Standard8
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Resolution: FIXED → DUPLICATE
Blocks: 1433901
Resolution: DUPLICATE → FIXED
Whiteboard: [fxsearch]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: