Add automated test for "Specific tags can be removed from a bookmark"

RESOLVED FIXED in Firefox 60

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: pauly, Assigned: pauly)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
TestRail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4150

Steps:
1. Launch Firefox and open a bookmark.
2. Click the star-shaped button from the toolbar.
3. Remove a tag from the Tags field and either press enter or click Done.
4. Enable the Bookmarks Toolbar and right-click the same bookmark, then select Properties.
5. Remove a tag from the Tags field and either press enter or click Save.
6. Enable the Bookmarks Sidebar and right-click the same bookmark, then select Properties.
7. Remove a tag from the Tags field and either press enter or click Save.

Expected results:
1. Firefox is successfully launched.
The selected bookmark is successfully loaded.
2. On click, the button brings up the Edit This Bookmark panel.
3. The tag is successfully removed from that bookmark.
4. A dialog box is brought up showing the properties of the selected bookmark.
5. The tag is successfully removed from that bookmark.
6. A dialog box is brought up showing the properties of the selected bookmark.
7. The tag is successfully removed from that bookmark.
(Assignee)

Comment 1

a year ago
Assignee: nobody → paul.silaghi
Attachment #8943611 - Flags: review?(standard8)
Comment on attachment 8943611 [details] [diff] [review]
browser_remove_bookmark_tags_starBtn_toolbar_sidebar

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

Thank you for the test. It generally looks good, just a few things to tidy up.

::: browser/components/places/tests/browser/browser.ini
@@ +90,4 @@
>  [browser_paste_into_tags.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
>  subsuite = clipboard
> +[browser_remove_bookmark_tags_starBtn_toolbar_sidebar.js]

For the filename, lets just go with `browser_bookmark_remove_tags.js` it is slightly easier to read.

::: browser/components/places/tests/browser/browser_remove_bookmark_tags_starBtn_toolbar_sidebar.js
@@ +2,5 @@
> + * Tests that the bookmark tags can be removed from the bookmark star, toolbar and sidebar.
> + */
> +"use strict";
> +
> +let bookmarkPanel = document.getElementById("editBookmarkPanel");

Please put these near to where they are used, unless they spread across more than one test.

@@ +6,5 @@
> +let bookmarkPanel = document.getElementById("editBookmarkPanel");
> +let doneButton = document.getElementById("editBookmarkPanelDoneButton");
> +let bookmarkStar = BookmarkingUI.star;
> +let TEST_URL = "about:buildconfig";
> +let TEST_URI = Services.io.newURI(TEST_URL);

Please make just TEST_URL/TEST_URI into constants. They can stay global, since they are general to the tests.

@@ +12,5 @@
> +let toolbar = document.getElementById("PersonalToolbar");
> +let wasCollapsed = toolbar.collapsed;
> +let tab;
> +
> +function getToolbarNodeForItemGuid(aItemGuid) {

Since this is also defined in browser_click_bookmarks_on_toolbar.js, we should just move it to head.js. Please can you add some documentation (see other functions in head.js), and change the parameter name from `aItemGuid` to `itemGuid` at the same time.

@@ +54,5 @@
> +
> +  // Update the "tags" field.
> +  await fillBookmarkTextField("editBMPanel_tagsField", "tag1, tag2, tag3", window);
> +
> +  let promiseNotification = PlacesTestUtils.waitForNotification("onItemChanged", (id, property) => property === "tags");

I think it is possible that fillBookmarkTextField could blur and start to cause the notification to be sent before we're ready for it. Hence, to be safe, lets move this waitForNotification to just before the fillBookmarkTextField.

@@ +139,5 @@
> +  });
> +});
> +
> +// Cleanup.
> +registerCleanupFunction(async () => {

Most of this cleanup can just be defined within the setup test, after the existing entries - it makes it easier to know in advance that the appropriate clean-up items have been implemented.

@@ +145,5 @@
> +  if (wasCollapsed) {
> +    await promiseSetToolbarVisibility(toolbar, false);
> +  }
> +  await PlacesUtils.bookmarks.eraseEverything();
> +  await BrowserTestUtils.removeTab(tab);

The removeTab part can be in a separate registerCleanupFunction just after where the tab is created with openNewForegroundTab
Attachment #8943611 - Flags: review?(standard8)
Priority: -- → P1
Whiteboard: [fx-search]
(Assignee)

Comment 3

a year ago
Posted patch browser_bookmark_remove_tags (obsolete) — Splinter Review
Thanks for the review.
Added the changes requested in comment 2.
Attachment #8943611 - Attachment is obsolete: true
Attachment #8944677 - Flags: review?(standard8)
Comment on attachment 8944677 [details] [diff] [review]
browser_bookmark_remove_tags

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

Thanks for the updates. r=Standard8
Attachment #8944677 - Flags: review?(standard8) → review+
Whiteboard: [fx-search] → [fxsearch]
Status: NEW → ASSIGNED
(Assignee)

Comment 5

a year ago
Changed browser.ini so the test skips running on Win10-ccov due to bug 1423667.
Attachment #8944677 - Attachment is obsolete: true

Comment 7

a year ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e82864a182ce
Add a test for checking that specific tags can be removed from a bookmark r=standard8
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e82864a182ce
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.