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)
Firefox
Bookmarks & History
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)
3.64 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8934492 -
Flags: review?(standard8)
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8934492 -
Attachment is obsolete: true
Attachment #8935770 -
Flags: review?(standard8)
Comment 4•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 5•7 years ago
|
||
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
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 6•7 years ago
|
||
(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)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8935770 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 8•7 years ago
|
||
(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
Comment 10•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•7 years ago
|
Resolution: FIXED → DUPLICATE
Updated•7 years ago
|
Whiteboard: [fxsearch]
You need to log in
before you can comment on or make changes to this bug.
Description
•