Open
Bug 1442188
Opened 7 years ago
Updated 2 years ago
Add automated test for "The items from the Bookmarks Toolbar can be sorted by name"
Categories
(Firefox :: Bookmarks & History, enhancement, P3)
Firefox
Bookmarks & History
Tracking
()
NEW
People
(Reporter: gechim, Unassigned)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [fxsearch])
Attachments
(1 file)
8.14 KB,
patch
|
standard8
:
review-
|
Details | Diff | Splinter Review |
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•7 years ago
|
Reporter | ||
Comment 1•7 years ago
|
||
Attachment #8955086 -
Flags: review?(standard8)
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [fxsearch]
Comment 2•7 years ago
|
||
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-
Updated•7 years ago
|
Priority: P2 → P3
Updated•6 years ago
|
Assignee: gechim → nobody
Flags: needinfo?(gechim)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•