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

RESOLVED FIXED in Firefox 59

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: icrisan, Assigned: icrisan)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Posted patch library_add_multiple_tags (obsolete) — Splinter Review
Attachment #8934443 - Flags: review?(adw)
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
Attachment #8934443 - Flags: review?(adw) → review?(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.
Attachment #8934443 - Flags: review?(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 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 on attachment 8935283 [details] [diff] [review]
library_add_tags

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

Great, thank you for the update.
Attachment #8935283 - Flags: review?(standard8) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a655e42edc2a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Resolution: FIXED → DUPLICATE
Duplicate of bug: 1431382
Blocks: 1431382
Resolution: DUPLICATE → FIXED
Whiteboard: [fxsearch]
You need to log in before you can comment on or make changes to this bug.