1427755 - Add automated test for "The name of a bookmark can be changed"

Status: RESOLVED FIXED

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

Description

[Ioana Crisan]

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

Steps:
1. Launch Firefox and open a bookmark
2. Click the star-shaped button from the toolbar
3. Change the value of the Name field and either press enter or click Done
4. Enable the Bookmarks Toolbar and right-click the same bookmark, then select Properties
5. Change the value of the Name field and either press enter or click Save
6. Enable the Bookmarks Sidebar and right-click the same bookmark, then select Properties
7. Change the value of the Name field and either press enter or click Save

Expected results:
Step 1: Firefox is successfully launched.
        The selected bookmark is successfully loaded.
Step 2: On click, the button brings up the Edit This Bookmark panel.
Step 3: The name of the bookmark is successfully modified.
Step 4: A dialog box is brought up showing the properties of the selected bookmark.
Step 5: The name of the bookmark is successfully modified.
Step 6. A dialog box is brought up showing the properties of the selected bookmark.
Step 7: The name of the bookmark is successfully modified.

Comment 1

[Ioana Crisan]

Attachment: bookmarks_change_title

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8939840 [details] [diff] [review]
bookmarks_change_title

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

Thanks for this. Can you also apply some of the comments from bug 1427701 to this one as well please? I've not duplicated them here.

::: browser/components/places/tests/browser/browser_bookmarks_change_title.js
@@ +39,5 @@
> +  // Uncollapse the personal toolbar if needed
> +  let toolbar = document.getElementById("PersonalToolbar");
> +  let wasCollapsed = toolbar.collapsed;
> +  if (wasCollapsed) {
> +    await promiseSetToolbarVisibility(toolbar, true);

The test needs to hide the toolbar again if necessary - use the registerCleanupFunction.

@@ +42,5 @@
> +  if (wasCollapsed) {
> +    await promiseSetToolbarVisibility(toolbar, true);
> +  }
> +  registerCleanupFunction(() => {
> +    PlacesUtils.bookmarks.eraseEverything();

The callback function here should be async, and we should be `await`ing the eraseEverything.

@@ +66,5 @@
> +
> +  // Update the bookmark's title
> +  await fillBookmarkTextField("editBMPanel_namePicker", titleAfterFirstUpdate, window);
> +
> +  let promiseNotification = PlacesTestUtils.waitForNotification("onItemChanged", (id, property) => property == "title");

You should be able to check the title value here as well.

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

If you save the guid of the bookmark above, you can just do

let updatedBm = await PlacesUtils.bookmarks.fetch(originalBm.guid);

which makes this a bit simpler.

@@ +72,5 @@
> +  await promiseNotification;
> +
> +  let bookmarks = [];
> +  await PlacesUtils.bookmarks.fetch({ url: TEST_URL }, bm => bookmarks.push(bm));
> +  Assert.equal(bookmarks[0].title, titleAfterFirstUpdate, "Bookmark " + bookmarks[0].url + " has title: " + bookmarks[0].title);

The message can just be something like "Should have updated the bookmark title in the database". No need to inject the extra parts, as some of that will be implicit, and the title itself will be displayed on the output should it error.

@@ +80,5 @@
> +  let toolbarBookmark = await PlacesUtils.bookmarks.insert({
> +    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
> +    title: titleAfterFirstUpdate,
> +    url: TEST_URL
> +  });

I'd suggest using a different url here. Other

@@ +102,5 @@
> +    async function test(dialogWin) {
> +      let namepicker = dialogWin.document.getElementById("editBMPanel_namePicker");
> +      Assert.equal(namepicker.value, titleAfterFirstUpdate, "Name field is correct before update.");
> +
> +      let promiseTitleChange = PlacesTestUtils.waitForNotification("onItemChanged", (id, prop) => prop == "title");

Please check the title value here as well.

Comment 3

[Ioana Crisan]

Attachment: bookmarks_change_title_1

Comment 4

[Ioana Crisan]

Comment on attachment 8939840 [details] [diff] [review]
bookmarks_change_title

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

::: browser/components/places/tests/browser/browser_bookmarks_change_title.js
@@ +80,5 @@
> +  let toolbarBookmark = await PlacesUtils.bookmarks.insert({
> +    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
> +    title: titleAfterFirstUpdate,
> +    url: TEST_URL
> +  });

I used the same url to follow the steps from the bug(test case). I can change it if necessary.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8942644 [details] [diff] [review]
bookmarks_change_title_1

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

Almost there, just a couple more things to tidy up.

::: browser/components/places/tests/browser/browser_bookmarks_change_title.js
@@ +2,5 @@
> + * Tests that the title of a bookmark can be changed from the bookmark star, toolbar, and sidebar.
> + */
> +"use strict";
> +
> +let bookmarkPanel = document.getElementById("editBookmarkPanel");

Sorry for not mentioning this before, generally I'd prefer most of these to be declared next to where they are first used, unless they need to be used across tests.

@@ +5,5 @@
> +
> +let bookmarkPanel = document.getElementById("editBookmarkPanel");
> +let doneButton = document.getElementById("editBookmarkPanelDoneButton");
> +let bookmarkStar = BookmarkingUI.star;
> +let TEST_URL = "about:buildconfig";

This and titleAfterFirstUpdate should be a const.

@@ +139,5 @@
> +  });
> +});
> +
> +// Cleanup.
> +registerCleanupFunction(async () => {

Generally I prefer this to be defined within the setup function just under where other items are set up - that way it is clearer when reading through the test that the cleanup is actually going to occur.

@@ +145,5 @@
> +  if (wasCollapsed) {
> +    await promiseSetToolbarVisibility(toolbar, false);
> +  }
> +  await PlacesUtils.bookmarks.eraseEverything();
> +  await BrowserTestUtils.removeTab(tab);

For the removeTab, this can be a separate registerCleanupFunction just under where the tab is added.

Comment 6

[Ioana Crisan]

Attachment: bookmarks_change_title_2

Comment 7

[Ioana Crisan]

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

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8943938 [details] [diff] [review]
bookmarks_change_title_2

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

Thanks for the updates. Regarding the constants, it was only the ones at the top of the file - applying globally that we tend to define as const. The others could have stayed as `let`.

However, I'll leave it up to you if you want to adjust those back or not, I don't mind too much either way.

Comment 9

[Ioana Crisan]

Attachment: bookmarks_change_title_3

Comment 10

[Ioana Crisan]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fec1e1d57d4
Add a test for checking that the title of a bookmark can be changed from the bookmark star, toolbar, and sidebar. r=Standard8

Comment 12

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/3fec1e1d57d4

Comment 13

[Fahima Zulfath]

Managed to reproduce the issue on an affected build, using the steps to reproduce from Comment 0, on Windows 10x64. The Bookmark properties is getting updated to as per the changes given..

Browser: Firefox Nightly 60.0a1
Build ID: 20180313100127

[bugday-20180307]

Comment 14

[Mark Banner (:standard8)]

(In reply to Fahima Zulfath from comment #13)
> Managed to reproduce the issue on an affected build, using the steps to
> reproduce from Comment 0, on Windows 10x64. The Bookmark properties is
> getting updated to as per the changes given..

There was no issue here or previously. This is just adding an automated test for an existing manually-run testcase.

Comment 15

[Fahima Zulfath]

(In reply to Mark Banner (:standard8) from comment #14)
> There was no issue here or previously. This is just adding an automated test
> for an existing manually-run testcase.

Well, I am newbie to automation. Would like to verify for automated test and want to know how it works. So can you please guide me how to do so?