Add automated test for "The items from the Bookmarks Toolbar can be sorted by name"

NEW
Unassigned

Status

()

enhancement
P3
normal
Last year
10 months ago

People

(Reporter: gechim, Unassigned)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

Reporter

Description

Last year
TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4161

Steps:
1.Launch Firefox.
2.Enable the Bookmarks Toolbar.
3.Enable the Bookmarks Sidebar.
4.Right-click the Bookmarks Toolbar entry from the sidebar and select Sort By Name.

Expected Result:
1.Firefox is successfully launched.
2.The Bookmarks Toolbar is enabled and displayed below the Location Bar.
3.The Bookmarks Sidebar is enabled and displayed at the left side.
4.The items from the Bookmarks Toolbar are successfully sorted by name and displayed accordingly in both the Bookmarks Sidebar and Toolbar.
Reporter

Updated

Last year
Assignee: nobody → gechim
Blocks: 1419383
Depends on: 1440242
Reporter

Comment 1

Last year
Attachment #8955086 - Flags: review?(standard8)
Priority: -- → P2
Whiteboard: [fxsearch]
Comment on attachment 8955086 [details] [diff] [review]
mochitest_browser_bookmark_sorting

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

I've only reviewed the general flow here, as we can't have tests that generate random input (see below).

It will definitely be good to get a test for this as I can't see any test coverage for this function at the moment.

::: browser/components/places/tests/browser/browser_bookmark_sorting.js
@@ +160,5 @@
> +    if (bookmarkFromDB !== null) {
> +      await PlacesUtils.bookmarks.remove(bookmarkFromDB.guid);
> +    }
> +    return bookmarkFromDB === null;
> +  }, "Not able to remove initial bookmarks");

This shouldn't be necessary after eraseEverything. If it is, please put in a separate comment what you're seeing.

@@ +163,5 @@
> +    return bookmarkFromDB === null;
> +  }, "Not able to remove initial bookmarks");
> +});
> +
> +add_task(async function main_test() {

nit: main_test -> test_sort_by_name

@@ +164,5 @@
> +  }, "Not able to remove initial bookmarks");
> +});
> +
> +add_task(async function main_test() {
> +  await checkOrAddWidgetButton("library-button");

Why do we need the library-button here?

@@ +165,5 @@
> +});
> +
> +add_task(async function main_test() {
> +  await checkOrAddWidgetButton("library-button");
> +  await checkOrAddWidgetButton("sidebar-button");

I just realised, we don't need to have everything you're doing in the checkOrAddWidgetButton function - all we need to do is call `CustomizableUI.addWidgetToArea("sidebar-button", CustomizableUI.AREA_NAVBAR);` direct.

Note: addWidgetToArea is not async, so you don't need to await for it (https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/browser/components/customizableui/CustomizableUI.jsm#3216)

@@ +167,5 @@
> +add_task(async function main_test() {
> +  await checkOrAddWidgetButton("library-button");
> +  await checkOrAddWidgetButton("sidebar-button");
> +
> +  initialBookmarks = await addRandomBookmarks(totalMockBookmarks);

We don't use random input for our test suites. This is because it makes reproducing failures really difficult and could introduce intermittent failures.

This should have a specific test case of a few bookmarks, e.g. no more than about 5 or 6, and at least one including non-ascii characters. We could possibly take the example from bug 201901 comment 0.
Attachment #8955086 - Flags: review?(standard8) → review-
George, are you still working on these?
Flags: needinfo?(gechim)
Priority: P2 → P3
Assignee: gechim → nobody
Flags: needinfo?(gechim)
You need to log in before you can comment on or make changes to this bug.