Closed Bug 1427701 Opened 2 years ago Closed 2 years ago

Add automated test for "Tags can be added to bookmarks using the star-shaped button"

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: icrisan, Assigned: icrisan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 2 obsolete files)

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

Steps:
1. Launch Firefox
2. Open a website that's currently not in your bookmarks list
3. Click the star-shaped button to bookmark it
4. Click the button again
5. Type a tag name into the Tags field and either press enter or click the Done button from the panel
6. Add more tags

Expected results:
Step 1: Firefox is successfully launched.
Step 3: On click, the star-shaped button shows an animation and changes its color to blue.
        The website is successfully bookmarked and no errors are thrown for this action.
Step 4: Clicking the button again brings up the Edit This Bookmark panel.
Step 5: The tag is successfully added to the bookmark.
Step 6: Several tags can be successfully added to a bookmark.
Attached patch bookmarkProperties_add_tags (obsolete) — Splinter Review
Attachment #8939524 - Flags: review?(standard8)
Comment on attachment 8939524 [details] [diff] [review]
bookmarkProperties_add_tags

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

Thanks for writing this, it is a good start. There's a few things that I think can be improved, mainly around what we're checking (and how) at each stage.

::: browser/components/places/tests/browser/browser_bookmarkProperties_addTags.js
@@ +8,5 @@
> +let bookmarkStar = BookmarkingUI.star;
> +let bookmarkPanelTitle = document.getElementById("editBookmarkPanelTitle");
> +let TEST_URL = "about:buildconfig";
> +
> +function promisePopupShown(popup) {

The promisePopupShown/promisePopupHidden functions should ideally be shared with other code. It looks like m-c already has a bit of a mess wrt to these, so for now, I'll be happy if you copy the implementations from browser/base/content/test/general/head.js into browser/components/places/tests/browser/head.js. Please add a reference so we know they are copied.

In the long term, we should look at merging all the current implementations into BrowserTestUtils.jsm or something similar.

@@ +41,5 @@
> +
> +add_task(async function test_add_tags() {
> +  info("Loading test page: " + TEST_URL);
> +  gBrowser.selectedBrowser.loadURI(TEST_URL);
> +  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);

It is normal for tests to load pages in a new tab, so that we leave the UI in a good, known state for whichever test is run next.

I'd suggest using BrowserTestUtils.openNewForegroundTab here, and adding a registerCleanupFunction to call BrowserTestUtils.removeTab().

@@ +43,5 @@
> +  info("Loading test page: " + TEST_URL);
> +  gBrowser.selectedBrowser.loadURI(TEST_URL);
> +  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +  // Click the bookmark star to bookmark the page

The bookmarking is instant-apply, so we shouldn't need to open it and shut it again before adding tags.

@@ +50,5 @@
> +  Assert.equal(bookmarkStar.getAttribute("starred"), "true", "Page is starred");
> +  let hiddenPromise = promisePopupHidden(bookmarkPanel);
> +  await hiddenPromise;
> +
> +  // Click the bookmark star again to add tags

General nit: full stop on the end of sentences please.

@@ +52,5 @@
> +  await hiddenPromise;
> +
> +  // Click the bookmark star again to add tags
> +  await clickBookmarkStar();
> +  Assert.equal(bookmarkPanelTitle.value, gNavigatorBundle.getString("editBookmarkPanel.editBookmarkTitle"), "Bookmark title is correct");

I don't think this test is providing much value. We'll know if we've got completely the wrong dialog from the next part of the test when we try to edit items.

I think you could check that you get the bookmark added when the window is opened, using PlacesTestUtils.waitForNotification, e.g. https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/browser/base/content/test/general/browser_bookmark_titles.js#94

@@ +53,5 @@
> +
> +  // Click the bookmark star again to add tags
> +  await clickBookmarkStar();
> +  Assert.equal(bookmarkPanelTitle.value, gNavigatorBundle.getString("editBookmarkPanel.editBookmarkTitle"), "Bookmark title is correct");
> +  await fillBookmarkTextField("editBMPanel_tagsField", "tag1", window);

I believe there's an onItemChanged notification for tag additions, so we should check that we get that notification here. I don't think it contains the details of the tags, but you could do the database fetch to confirm that it has just the expected tag.

@@ +55,5 @@
> +  await clickBookmarkStar();
> +  Assert.equal(bookmarkPanelTitle.value, gNavigatorBundle.getString("editBookmarkPanel.editBookmarkTitle"), "Bookmark title is correct");
> +  await fillBookmarkTextField("editBMPanel_tagsField", "tag1", window);
> +  hiddenPromise = promisePopupHidden(bookmarkPanel);
> +  doneButton.click();

These hiding sections could be merged into a `hideBookmarksPanel` function (just within this file).

@@ +66,5 @@
> +  doneButton.click();
> +  await hiddenPromise;
> +
> +  let bookmarks = [];
> +  await PlacesUtils.bookmarks.fetch({ url: TEST_URL }, bm => bookmarks.push(bm));

If we're using this form of the API, lets also check that we've got just one bookmark stored.
Attachment #8939524 - Flags: review?(standard8)
Comment on attachment 8939524 [details] [diff] [review]
bookmarkProperties_add_tags

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

::: browser/components/places/tests/browser/browser_bookmarkProperties_addTags.js
@@ +43,5 @@
> +  info("Loading test page: " + TEST_URL);
> +  gBrowser.selectedBrowser.loadURI(TEST_URL);
> +  await BrowserTestUtils.browserLoaded(gBrowser.selectedBrowser);
> +
> +  // Click the bookmark star to bookmark the page

I did this because I followed the steps from the test case. This test case is owned by the QA manual team and we try to automate it. We also intend to automate more test cases.
Hi Mark,
It is ok if we follow one to one the steps/expected results from this bug or it doesn't make sense?
(In reply to Ioana Crisan from comment #5)
> It is ok if we follow one to one the steps/expected results from this bug or
> it doesn't make sense?

I'm ok for following the steps, it just seemed a little unnecessary as I was reading it.
Priority: -- → P3
Attached patch bookmarkProperties_add_tags_1 (obsolete) — Splinter Review
Attachment #8939524 - Attachment is obsolete: true
Attachment #8941057 - Flags: review?(standard8)
Comment on attachment 8941057 [details] [diff] [review]
bookmarkProperties_add_tags_1

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

Looks good, thanks.

::: browser/components/places/tests/browser/browser_bookmarkProperties_addTags.js
@@ +32,5 @@
> +  await hideBookmarksPanel(async () => {
> +    await clickBookmarkStar();
> +    Assert.equal(bookmarkPanelTitle.value, gNavigatorBundle.getString("editBookmarkPanel.pageBookmarkedTitle"), "Bookmark title is correct");
> +    Assert.equal(bookmarkStar.getAttribute("starred"), "true", "Page is starred");
> +  });

Please add a comment here to state that the bookmarks panel is expected to auto-close which is why there's no explicit action for hiding the panel.
Attachment #8941057 - Flags: review?(standard8) → review+
Attachment #8941057 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69e9e95470a
Add a test for checking that tags can be added to bookmarks using the star-shaped button. r=Standard8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b69e9e95470a
Status: NEW → RESOLVED
Closed: 2 years ago
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.