1423118 - Add automated test for "Tags can be added to bookmarks using the Library"

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Ioana Crisan]

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

Preconditions: A dirty profile with a reasonable amount of browsing history and bookmarks.

Steps:
1. Launch Firefox
2. From the toolbar, click the Bookmarks Menu button
3. Select Show All Bookmarks
4. Select a random bookmark from the list
5. Type a tag name into the Tags field and press enter
6. Add more tags

Expected results:
Step 1: Firefox is successfully launched.
Step 2:	Clicking the button brings up the Bookmarks Menu. 
Step 3: The Library window is opened, with Other Bookmarks selected by default.
Step 4: The bookmark in question is selected.
Step 5: The tag is successfully added to the bookmark.
Step 6: Several tags can be successfully added to a bookmark.

Comment 1

[Ioana Crisan]

Attachment: library_add_multiple_tags

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8934443 [details] [diff] [review]
library_add_multiple_tags

I'm forwarding to Mark that recently looked into a lot of these tests

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8934443 [details] [diff] [review]
library_add_multiple_tags

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

Thanks for the patch.

Please can you change the commit message to something like "Bug 1423118 - Add a test for checking that tags can be added to bookmarks using the library window.". This gives a clearer description as to what the changeset is actually doing.

::: browser/components/places/tests/browser/browser.ini
@@ +19,1 @@
>  support-files =

Although it doesn't seem to at the moment, this could break browser_bookmarklet_windowOpen.js - the support-files statement needs to stay with the test file. Hence, please add the new line after the support-files section.

::: browser/components/places/tests/browser/browser_library_add_multiple_tags.js
@@ +3,5 @@
> + */
> +"use strict";
> +
> +add_task(async function() {
> +  let uri = Services.io.newURI("http://example.com/");

We're slowly moving our APIs away from requiring formed URLs. Can you make this a `const uri = "http://example.com/";` and then do the Services.io.newURI call inline with the getTagsForURI call?

@@ +35,5 @@
> +
> +  // Cleanup
> +  registerCleanupFunction(async function() {
> +    await promiseLibraryClosed(library);
> +    await PlacesUtils.bookmarks.remove(bm);

Please use `await PlacesUtils.bookmarks.eraseEverything()` instead of `.remove(bm)` as that will remove the tags as well. You also won't need the bm intermediate variable.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8934443 [details] [diff] [review]
library_add_multiple_tags

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

Sorry, a couple more items I just remembered.

Please can you change the test filename to be "browser_library_add_tags.js"? I don't think we need the multiple reference in here, and it isn't quite right anyway as the test is only adding a single tag at a time.

::: browser/components/places/tests/browser/browser_library_add_multiple_tags.js
@@ +2,5 @@
> + * Tests that multiple tags can be added to a bookmark using the Library.
> + */
> +"use strict";
> +
> +add_task(async function() {

Please can you add a test name for this, it simplifies adding more tests to the file later and gives more context, e.g.:

```
add_task(async function test_add_tags() {
```

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8934443 [details] [diff] [review]
library_add_multiple_tags

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

::: browser/components/places/tests/browser/browser.ini
@@ +19,1 @@
>  support-files =

Sorry again, one last thing, please can you put this test file alongside the other browser_library_* files in this file, in alphabetical order (as best as possible, without rearranging everything).

Comment 6

[Ioana Crisan]

Attachment: library_add_tags

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6a20084029ecd10dc6c3c253d6e86c787e57526

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8935283 [details] [diff] [review]
library_add_tags

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

Great, thank you for the update.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a655e42edc2a
Add a test for checking that tags can be added to bookmarks using the library window. r=Standard8

Comment 9

[Andrei Ciure[:aciure]]

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