1433901 - Add automated test for "Bookmarks can be removed from the Bookmarks Toolbar and 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/4098

Preconditions: A dirty profile with a reasonable amount of browsing history and bookmarks.

Steps:
1. Launch Firefox
2. Enable the Bookmarks Toolbar
3. Right-click any of the bookmarks displayed in the toolbar and select Delete.

Expected results:
Step 1: Firefox is successfully launched.
Step 2: The Bookmarks Toolbar is enabled and displayed below the Location Bar.
Step 3: The bookmark is successfully removed.

Comment 1

[Ioana Crisan]

Hi Mark,
It is ok to merge also this test and browser_library_remove_bookmarks.js in a new js file for example browser_remove_bookmarks.js?
Thank you!

Comment 2

[Mark Banner (:standard8)]

(In reply to Ioana Crisan from comment #1)
> It is ok to merge also this test and browser_library_remove_bookmarks.js in

That took me a while, there's no 's' on the actual filename.

> a new js file for example browser_remove_bookmarks.js?

Yes, that would be fine.

Comment 3

[Ioana Crisan]

(In reply to Mark Banner (:standard8) from comment #2)
> (In reply to Ioana Crisan from comment #1)
> > It is ok to merge also this test and browser_library_remove_bookmarks.js in
> 
> That took me a while, there's no 's' on the actual filename.
> 
> > a new js file for example browser_remove_bookmarks.js?
> 
> Yes, that would be fine.

Indeed! Sorry for the 's'.

Comment 4

[Ioana Crisan]

Attachment: browser_remove_bookmarks

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8947079 [details] [diff] [review]
browser_remove_bookmarks

Review of attachment 8947079 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch, looking good, just a couple of things to tidy up.

::: browser/components/places/tests/browser/browser_remove_bookmarks.js
@@ +9,5 @@
> + */
> +
> +const TEST_URL = "about:mozilla";
> +
> +function getToolbarNodeForItemGuid(aItemGuid) {

This function is already in head.js, so shouldn't need repeating here.

@@ +48,5 @@
> +  let toolbarNode = getToolbarNodeForItemGuid(toolbarBookmark.guid);
> +
> +  toolbarBookmark = [];
> +  await PlacesUtils.bookmarks.fetch({ url: TEST_URL }, bm => toolbarBookmark.push(bm));
> +  Assert.equal(toolbarBookmark.length, 1, "Should have one bookmark in the database");

I don't think we need the additional check here prior to the test - we know what we're putting into it. You could put an eraseEverything into setup (so that we erase before & after).

@@ +69,5 @@
> +  await removePromise;
> +
> +  toolbarBookmark = [];
> +  await PlacesUtils.bookmarks.fetch({ url: TEST_URL }, bm => toolbarBookmark.push(bm));
> +  Assert.equal(toolbarBookmark.length, 0, "Should have removed the bookmark from the database");

This last bit can be simplified to:

Assert.equal(PlacesUtils.bookmarks.fetch({ url: TEST_URL }), null, "Should have removed the bookmark from the database");

@@ +134,5 @@
> +  Assert.equal(library.ContentTree.view.result.root.childCount, 2, "Should have removed the bookmark from the display");
> +
> +  // Cleanup.
> +  registerCleanupFunction(async function() {
> +    await PlacesUtils.bookmarks.eraseEverything();

This cleanup erase is already registered in the setup function, so no need for it to be here as well.

Comment 7

[Ioana Crisan]

Attachment: browser_remove_bookmarks_1

Comment 8

[Ioana Crisan]

TRY results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51982fe1c50f956f2cfa9ed28c3e4eb6990b3d76

Comment 9

[Ioana Crisan]

Please consider these TRY results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=311b9a13ea1d624c801bfa3047e4b65cfac6f308

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8950150 [details] [diff] [review]
browser_remove_bookmarks_1

Review of attachment 8950150 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you. r=Standard8

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/575415a1d055
Add a test for checking that bookmarks can be removed from toolbar and from library. r=Standard8

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/575415a1d055