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

Status: RESOLVED FIXED

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

Description

[Paul Silaghi, QA [:pauly]]

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.

Comment 1

[Paul Silaghi, QA [:pauly]]

Attachment: browser_remove_bookmark_tags_starBtn_toolbar_sidebar

Comment 2

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

Comment 3

[Paul Silaghi, QA [:pauly]]

Attachment: browser_bookmark_remove_tags

Thanks for the review.
Added the changes requested in comment 2.

Comment 4

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

Comment 5

[Paul Silaghi, QA [:pauly]]

Attachment: browser_bookmark_remove_tags

Changed browser.ini so the test skips running on Win10-ccov due to bug 1423667.

Comment 6

[Paul Silaghi, QA [:pauly]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=47276c70790a71dca423bd35aaeeee38f0ed4165&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Comment 7

[Pulsebot]

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

Comment 8

[Cristina Coroiu [:ccoroiu]]

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