1431382 - Add automated test for "Tags can be added to bookmarks using the star-shaped button, the library and the sidebar"

Status: RESOLVED FIXED

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

Description

[Ioana Crisan]

testrail link: https://testrail.stage.mozaws.net/index.php?/cases/view/4147

Steps:
1. Launch Firefox
2. Enable the Bookmarks Sidebar from 'View history, saved bookmarks and more' -> `Bookmarks' -> Bookmarking Tools
3. Right-click any of the bookmarks displayed in the sidebar and select Properties
4. Type a tag name into the Tags field and either press enter or click Save
5. Bring back up the properties dialog for that bookmark and add more tags

Expected results:
Step 1: Firefox is successfully launched.
Step 2: The Bookmarks Sidebar is enabled and displayed at the left side.
Step 3: A dialog box is brought up showing the properties of the selected bookmark.
Step 4: The tag is successfully added to the bookmark.
Step 5: Several tags can be successfully added to a bookmark.

Comment 1

[Ioana Crisan]

Attachment: bookmarks_sidebar_add_tags

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8943587 [details] [diff] [review]
bookmarks_sidebar_add_tags

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

This is looking good. Just a couple of small things to tidy up.

Thank you for your work on these.

::: browser/components/places/tests/browser/browser_bookmarks_sidebar_add_tags.js
@@ +2,5 @@
> + * Tests that tags can be added to bookmarks using the sidebar.
> + */
> +"use strict";
> +
> +let TEST_URL = "about:buildconfig";

Please change just this to a const.

@@ +17,5 @@
> +    url: TEST_URL,
> +    title: "Bookmark Title"
> +  });
> +
> +  async function addTags(tagValue, tree, expected) {

Please can you put this function at the end of the file - it makes it easier to read the logic of the test.

@@ +42,5 @@
> +    await addTags(["tag2", "tag3"], tree, ["tag1", "tag2", "tag3"]);
> +  });
> +
> +  // Setup and cleanup.
> +  add_task(async function setup() {

I'm not actually sure how this will work with add_task inside an add_task. In any case, I think you can drop the add_task wrapper, and just have the registerCleanupFunction - probably put it just under the openNewForgroundTab call.

@@ +44,5 @@
> +
> +  // Setup and cleanup.
> +  add_task(async function setup() {
> +    registerCleanupFunction(async () => {
> +      PlacesUtils.bookmarks.eraseEverything();

This needs an await, otherwise the erase may run into the next test.

Comment 3

[Ioana Crisan]

Attachment: bookmarks_sidebar_add_tags_1

Comment 4

[Ioana Crisan]

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

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8944388 [details] [diff] [review]
bookmarks_sidebar_add_tags_1

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

Thank you. The test looks good.

::: browser/components/places/tests/browser/browser_bookmarks_sidebar_add_tags.js
@@ +4,5 @@
> +"use strict";
> +
> +const TEST_URL = "about:buildconfig";
> +
> +add_task(async function test_add_bookmark_tags_from_sidebar() {

Rather than a separate test file, could you wrap this into browser_library_add_tags.js and rename it to browser_bookmark_add_tags.js? This would match the work that Paul is doing in bug 1431390, and keep these similar tests together.

@@ +5,5 @@
> +
> +const TEST_URL = "about:buildconfig";
> +
> +add_task(async function test_add_bookmark_tags_from_sidebar() {
> +  let tab = await BrowserTestUtils.openNewForegroundTab({

Since we're not opening the page to bookmark it, we shouldn't need to open a new tab here.

Comment 6

[Ioana Crisan]

(In reply to Mark Banner (:standard8) from comment #5)
> Comment on attachment 8944388 [details] [diff] [review]
> bookmarks_sidebar_add_tags_1
> 
> Review of attachment 8944388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you. The test looks good.
> 
> :::
> browser/components/places/tests/browser/browser_bookmarks_sidebar_add_tags.js
> @@ +4,5 @@
> > +"use strict";
> > +
> > +const TEST_URL = "about:buildconfig";
> > +
> > +add_task(async function test_add_bookmark_tags_from_sidebar() {
> 
> Rather than a separate test file, could you wrap this into
> browser_library_add_tags.js and rename it to browser_bookmark_add_tags.js?
> This would match the work that Paul is doing in bug 1431390, and keep these
> similar tests together.
> 

Hi Mark,
I noticed that we have another test that adds tags(browser_bookmarkProperties_addTags.js). It is ok to put it also in the browser_bookmark_add_tags.js?
Thank you.

Comment 7

[Mark Banner (:standard8)]

(In reply to Ioana Crisan from comment #6)
> I noticed that we have another test that adds
> tags(browser_bookmarkProperties_addTags.js). It is ok to put it also in the
> browser_bookmark_add_tags.js?

I was thinking about keeping them separate, as this was seemingly testing more functional things. However, now I look again, I see it is testing the star ui instances, and I think it is OK to merge them together.

Thanks for asking!

Comment 8

[Ioana Crisan]

Attachment: bookmark_add_tags

Comment 9

[Ioana Crisan]

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

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8946223 [details] [diff] [review]
bookmark_add_tags

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

::: browser/components/places/tests/browser/browser.ini
@@ -30,4 @@
>  skip-if = (os == 'win' && ccov) # Bug 1423667
>  [browser_bookmarkProperties_addLivemark.js]
>  skip-if = (os == 'win' && ccov) # Bug 1423667
> -[browser_bookmarkProperties_addTags.js]

I'm not seeing the repository removals for the files that you're removing from here.

::: browser/components/places/tests/browser/browser_bookmark_add_tags.js
@@ +1,2 @@
> +/**
> + * Tests that multiple tags can be added to a bookmark using the star-shaped button, the library and the sidebar.

Please add a public domain license header to this file (https://www.mozilla.org/en-US/MPL/headers/#pd-c)

@@ +106,5 @@
> +  fillBookmarkTextField("editBMPanel_tagsField", "tag1, tag2", library);
> +
> +  await waitForCondition(() => bookmarkNode.tags === "tag1, tag2", "Node tag is correct");
> +
> +  // Check the tag change has been.

nit: ... has been completed.

Comment 11

[Ioana Crisan]

Attachment: bookmark_add_tags_1

Comment 12

[Ioana Crisan]

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

Comment 13

[Mark Banner (:standard8)]

Note: when you push to try for these, you only really need linux64, mac64 & win64 platforms - android doesn't matter, and we're not touching 32 bit specific things.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8947038 [details] [diff] [review]
bookmark_add_tags_1

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

Great, thank you.

Comment 15

[Ioana Crisan]

(In reply to Mark Banner (:standard8) from comment #13)
> Note: when you push to try for these, you only really need linux64, mac64 &
> win64 platforms - android doesn't matter, and we're not touching 32 bit
> specific things.

Ok, thank you!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/923d4e8331b2
Add a test for checking that tags can be added to bookmarks using the star-shaped button, the library and the sidebar. r=standard8

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/923d4e8331b2