1442182 - Add automated test for "Links can be bookmarked via context menu "

Status: NEW

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

Description

[George Echim]

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

Steps:
1.Launch Firefox.
2.Access this Mozilla article. (a page with hyperlinks)
3.Right-click any of the available hyperlinks from the page and select Bookmark This Link.
4.From the New Bookmark dialog box, click Save.
5.Open the link you just bookmarked	

1.Firefox is successfully launched.
2.The website is successfully loaded.
3.Selecting this context menu option brings up the New Bookmark dialog box, with the Name field auto-filled according to that link's title. The default bookmark location is Bookmarks Menu.
4.There are no issues nor errors thrown in the Browser Console for this action.
5.The page bookmarked via link context menu shows the star-shaped button as blue, in the toolbar. The page is successfully loaded.

Comment 1

[George Echim]

Attachment: mochitest_browser_bookmark_hyperlinks

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8955070 [details] [diff] [review]
mochitest_browser_bookmark_hyperlinks

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

Thanks for the test, I think this can be made a lot simpler and clearer though.

::: browser/components/places/tests/browser/browser_bookmark_hyperlinks.js
@@ +12,5 @@
> +
> +const NOT_BOOKMARKED_STAR_COLOR = "rgb(0, 0, 0)";
> +const BOOKMARKED_STAR_COLOR = "rgb(10, 132, 255)";
> +
> +const PAGES = {

We only need to test one page/link here.

@@ +14,5 @@
> +const BOOKMARKED_STAR_COLOR = "rgb(10, 132, 255)";
> +
> +const PAGES = {
> +  example: {
> +    uri: "https://example.com/",

These are using real-world examples, which we can't do in mochitests, although that might have changed recently without me realising. In any case, I think I'd prefer a separate couple of pages here that we control for just the test. See how bookmark_dummy_1.html is referenced and loaded for examples of how to do this. You could set up a bookmark_link_1.html which links to bookmark_link_2.html and check the results.

@@ +55,5 @@
> +  Assert.equal(gBrowser.tabs.length, 1, "Should close all new opened tabs");
> +}
> +
> +async function confirmBookmarkDialog(sourceHyperlink) {
> +  await withBookmarksDialog(false, () => {},

This needs to be combined into bookmarkLinkViaContext and the open function should contain the click that opens the dialog.

Otherwise we run the risk of missing the dialog open and causing intermittent issues.

@@ +58,5 @@
> +async function confirmBookmarkDialog(sourceHyperlink) {
> +  await withBookmarksDialog(false, () => {},
> +    async (dialogWin) => {
> +      let namepicker = dialogWin.document.getElementById("editBMPanel_namePicker");
> +      Assert.ok(!namepicker.readOnly, "Name field is writable");

We don't need to check readOnly here, that's not the aim of this test and is covered by other tests.

@@ +60,5 @@
> +    async (dialogWin) => {
> +      let namepicker = dialogWin.document.getElementById("editBMPanel_namePicker");
> +      Assert.ok(!namepicker.readOnly, "Name field is writable");
> +      Assert.equal(namepicker.value, sourceHyperlink.innerText,
> +        `Bookmark text value should be: ${sourceHyperlink.innerText}`);

No need to document the innerText here, Assert.equal does that for us.

@@ +62,5 @@
> +      Assert.ok(!namepicker.readOnly, "Name field is writable");
> +      Assert.equal(namepicker.value, sourceHyperlink.innerText,
> +        `Bookmark text value should be: ${sourceHyperlink.innerText}`);
> +      let acceptBtn = dialogWin.document.documentElement.getButton("accept");
> +      Assert.ok(acceptBtn, "Save button exists");

No need to check the button exists, if it doesn't acceptBtn.click() will generate the failure for us.

@@ +67,5 @@
> +      acceptBtn.click();
> +    });
> +}
> +
> +async function bookmarLinkViaContext(browser, hyperlink) {

nit: Missed the 'k' from bookmark.

@@ +75,5 @@
> +    type: "contextmenu"
> +  }, browser.contentWindow);
> +  await openContextMenuPromise;
> +
> +  let openInNewTabOption = document.getElementById("context-bookmarklink");

This isn't the open in new tab option.

@@ +95,5 @@
> +    return isStarred === "";
> +  }, "Was unable to parse starred attribute of star button");
> +
> +  let starCssValues = window.getComputedStyle(starElem);
> +  let starImageUri = starCssValues.getPropertyValue("list-style-image").toString();

We shouldn't be checking actual css values here, that's part of the theme, and we don't want to be linking those values to the code. Just checking for "isStarred" is enough.

@@ +108,5 @@
> +  }
> +}
> +
> +async function runTestOn(page) {
> +  let currentTab = await BrowserTestUtils.openNewForegroundTab(gBrowser);

We should just do `let currentTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, page.uri);` - this would avoid doing the extra dance with waiting for the page to load, as openNewForegroundTab does that all for us automatically.

@@ +120,5 @@
> +  await checkPageToBeStarred(false);
> +
> +  let currentDocument = currentBrowser.contentDocument;
> +  Assert.equal(currentDocument.documentURI, page.uri, `Page url should be ${page.uri}`);
> +  Assert.equal(currentDocument.title, page.expectedTitle, `Page title should be ${page.expectedTitle}`);

The URI is effectively checked in openNewForegroundTab, so we don't need to test it again here. I don't think we need to test the title either, as title setting will be tested elsewhere. If you really want to check it, is should be just after the page was loaded.

@@ +124,5 @@
> +  Assert.equal(currentDocument.title, page.expectedTitle, `Page title should be ${page.expectedTitle}`);
> +
> +  let hyperlink = currentDocument.querySelector(page.selector);
> +  Assert.ok(hyperlink, `Page should contain hyperlink: ${page.selector}`);
> +  hyperlink.focus();

I don't think the focus is necessary, but if it is needed, then it should go in bookmarkLinkViaContext, as it is part of selecting the link/context menu.

@@ +128,5 @@
> +  hyperlink.focus();
> +
> +  await bookmarLinkViaContext(currentBrowser, hyperlink);
> +
> +  let forNewPageLoad = promiseNewUrl(currentBrowser, page.uri);

Please use BrowserTestUtils.browserLoaded.

Comment 3

[George Echim]

Attachment: mochitest_browser_bookmark_hyperlinks_2

Hi Mark,

Thank you for reviewing the patch.
I have uploaded the new version.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8956438 [details] [diff] [review]
mochitest_browser_bookmark_hyperlinks_2

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

Thanks for the update, we're almost there, just a few minor issues to tidy up.

::: browser/components/places/tests/browser/browser_bookmark_hyperlinks.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Somehow this file has changed to four-space indentation? We should be using 2.

@@ +46,5 @@
> +            acceptBtn.click();
> +        });
> +}
> +
> +async function checkPageToBeStarred() {

nit: I think a slightly clearer title would be `checkPageIsStarred`

@@ +70,5 @@
> +
> +    await checkPageToBeStarred();
> +
> +    registerCleanupFunction(async () => {
> +        await closeTabs();

There's now only one tab, so you can call `await gBrowser.removeTab()` direct and drop the closeTabs function.

Comment 5

[George Echim]

Attachment: mochitest_browser_bookmark_hyperlinks_3

Hi Mark,

Thank you reviewing the patch.
I have uploaded a new one.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8957148 [details] [diff] [review]
mochitest_browser_bookmark_hyperlinks_3

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

r=Standard8 with the nit fixed.

::: browser/components/places/tests/browser/browser_bookmark_hyperlinks.js
@@ +48,5 @@
> +  }, "Was unable to parse starred attribute of star button");
> +}
> +
> +add_task(async function test_hyperlink_bookmark() {
> +  currentTab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE.uri);

nit: currentTab no longer needs to be a global, so please drop the global and just prefix this line with the `let`.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8957148 [details] [diff] [review]
mochitest_browser_bookmark_hyperlinks_3

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

Running this before potentially pushing, I noticed that it is failing due to:

FAIL Uncaught exception - at chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmark_hyperlinks.js:56 - TypeError: currentDocument is null
Stack trace:
test_hyperlink_bookmark@chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_bookmark_hyperlinks.js:56:7

It does pass in --disable-e10s mode.

So this needs rewriting to perform the document actions in a content task.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.