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

Status: RESOLVED FIXED

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

Description

[Ioana Crisan]

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.

Comment 1

[Ioana Crisan]

Attachment: library_remove_bookmark

Comment 2

[Mark Banner (: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).

Comment 3

[Ioana Crisan]

Attachment: library_remove_bookmark_1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45c228d4f3aab011348d678b9316638e37e83fa1

Comment 4

[Mark Banner (: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.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch. Also, please update your commit information so that there's a space between your name and email. Thanks!

Comment 6

[Ioana Crisan]

(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?

Comment 7

[Ioana Crisan]

Attachment: library_remove_bookmark_2

Comment 8

[Mark Banner (:standard8)]

(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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ec28a490365c