941477 - Create a remote Mozmill test for the 'Get more Search Engines' feature

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Similar to the functional test for getting more search engines we need a test which directly tests the add-on website. Since bug 621590 has been fixed we see a list of search engines now, and we have to test that installing one of them succeeds. 

URL:
https://addons.mozilla.org/en-US/firefox/search/?atype=4

Steps:
1. Open search engine manager
2. Click on get more search engines
3. Install one of the listed engines
4. Verify that the installation was successful

We might want to consider to move and enhance the functional test into the remote testrun instead.

To get started please check:
https://developer.mozilla.org/en/Mozmill_Tests

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I don't see why this bug is major enough to be tracked as Q2 goal. Removing dependency.

Comment 2

[Teodor Druta]

Attachment: b941477.patch

The attached patch will add a a new "testAddSearchEngine" test and a pair of new methods in search.js library.

Comment 3

[Andrei Eftimie]

Comment on attachment 8507749 [details] [diff] [review]
b941477.patch

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

Nice work here.
We still have to change some stuff around, but the test works nicely as intended.

::: firefox/lib/search.js
@@ +211,5 @@
> +   *        MozMillController of the window to operate on
> +   *
> +   * @private
> +   */
> +  _handleEngineInstallNoDomainCheck : function engineManager_handleEngineInstall(aController) {

Instead of a new method I'd rather we extend _handleEngineInstall with a new argument.

But do we really need it?
So you're removing a check that the "info.body" element should not contain "localhost". Seems we fail in retrieving info.body.
Please check what this is and why it is working differently than in other places.

I think we should be able to use the same install method.

@@ +400,5 @@
> +    var md = new modalDialog.modalDialog(this._controller.window);
> +
> +    md.start(this._handleEngineInstallNoDomainCheck);
> +
> +    var name = aAddEngineCallback().replace(/ *\([^)]*\) */g, "").trim();

What does this do? Please add a comment whenever you use a regex.
Will this work for all search engine names?

@@ +406,5 @@
> +    md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> +
> +    // Check if the local engine has been installed
> +    var searchBarInstance = new searchBar(this._controller);
> +

Please remove most empty newlines you have, keep them for separating logical blocks, don't be excessive.

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +17,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +
> +  aModule.engineManager = new search.engineManager(aModule.controller);
> +

nit: Please remove this empty newline

@@ +33,5 @@
> +  // store number of tabs before opening a new tab
> +  var tabCount = controller.tabs.length;
> +
> +  // Open the engine manager and click "Get more search engines..."
> +  searchBar.openEngineManager(enginesHandler);

I'd keep the handler callback function inline as an anonymous function, it is pretty basic.

@@ +36,5 @@
> +  // Open the engine manager and click "Get more search engines..."
> +  searchBar.openEngineManager(enginesHandler);
> +
> +  assert.waitFor(() => (controller.tabs.length === (tabCount + 1)),
> +                 "The 'Get More Engines' link has been opened in a new tab");

These last 3 lines can be done with the tabbrowser library with a call to openTab() with a custom callback.
That will also handle waiting for the tab to finish opening (animation-wise)

@@ +41,5 @@
> +
> +  controller.waitForPageLoad();
> +
> +  // check if correct url has been opened
> +  utils.assertLoadedUrlEqual(controller, TEST_DATA);

This doesn't work as you're hardcoding an en-US specific URL.
Check with a localised build:
> Current URL should be identical to the target URL - expected https://addons.mozilla.org/zh-CN/firefox/search/?atype=4

I'm not sure we have a proper way of asserting we are on the correct page here.

@@ +55,5 @@
> + *        MozMillController of the window to operate on
> + */
> +function enginesHandler(aController) {
> +  // Click Browse link - dialog will close automatically
> +  var browseLink = findElement.ID(aController.window.document, "addEngines");

You can get this via the search lib: `getElemet({ type: "more_engines" })`

@@ +56,5 @@
> + */
> +function enginesHandler(aController) {
> +  // Click Browse link - dialog will close automatically
> +  var browseLink = findElement.ID(aController.window.document, "addEngines");
> +  aController.waitThenClick(browseLink);

browseLink.click();

@@ +62,5 @@
> +
> +/**
> + * Click the "Add to Firefox" button from the addons page
> + */
> +function installFromAddonPageHandler() {

So which addon do we install, the first one available?

@@ +64,5 @@
> + * Click the "Add to Firefox" button from the addons page
> + */
> +function installFromAddonPageHandler() {
> +  // get first add to firefox link
> +  var link = findElement.Selector(controller.tabs.activeTab, '.clickHijack');

Please use the actual link: ".install-button > a"

@@ +67,5 @@
> +  // get first add to firefox link
> +  var link = findElement.Selector(controller.tabs.activeTab, '.clickHijack');
> +
> +  // trigger mouseover for the add to firefox link
> +  link.mouseOver();

Is this still needed?

Comment 4

[Teodor Druta]

> 
> ::: firefox/lib/search.js
> @@ +211,5 @@
> > +   *        MozMillController of the window to operate on
> > +   *
> > +   * @private
> > +   */
> > +  _handleEngineInstallNoDomainCheck : function engineManager_handleEngineInstall(aController) {
> 
> Instead of a new method I'd rather we extend _handleEngineInstall with a new
> argument.
> 
> But do we really need it?
> So you're removing a check that the "info.body" element should not contain
> "localhost". Seems we fail in retrieving info.body.
> Please check what this is and why it is working differently than in other
> places.
> 
> I think we should be able to use the same install method.
> 

I searched mercurial changeset logs and I found that a more general solution for checking 
the domain url in now called _handleEngineInstall method has been replaced with a hardcoded one.
There is a verfication for checking if the search engine is being installed from the 'localhost' domain.

I'm talking about Bug 519027. -r 174

>+    // Retrieve the URL which is used for the currently selected search engine
>+    var targetURL = this._bss.currentEngine.getSubmission(searchTerm, null).uri;
>+
>+    // Check if pure domain names are identical
>+    var domainName = targetURL.host.replace(/.+\.(\w+)\.\w+$/gi, "$1");
>+    if(locationBar.getNode().value.indexOf(domainName) == -1)
>+      throw "Expected domain name doesn't match the current one"

The lines above have been changed with:

>+    var infoBody = controller.window.document.getElementById("info.body");
>+    controller.waitForEval("subject.textContent.indexOf('litmus.mozilla.org') != -1",
>+                           gTimeout, 100, infoBody);

The hardcoded solution above.

If we could return to a similar solution as the one before this change.
Then there will be no need to add a new method.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #4)
> I searched mercurial changeset logs and I found that a more general solution
> for checking 
> the domain url in now called _handleEngineInstall method has been replaced
> with a hardcoded one.
> There is a verfication for checking if the search engine is being installed
> from the 'localhost' domain.
> 
> I'm talking about Bug 519027. -r 174

I would like to see if you all can come up with a proper idea how to best handle that. So lets get this discussed with Andrei, who actually gave you that reply.

Comment 6

[Teodor Druta]

Attachment: b941477v2.patch

(In reply to Andrei Eftimie from comment #3)
> 
> ::: firefox/lib/search.js
> @@ +211,5 @@
> > +   *        MozMillController of the window to operate on
> > +   *
> > +   * @private
> > +   */
> > +  _handleEngineInstallNoDomainCheck : function engineManager_handleEngineInstall(aController) {
> 
> Instead of a new method I'd rather we extend _handleEngineInstall with a new
> argument.
> 
> But do we really need it?
> So you're removing a check that the "info.body" element should not contain
> "localhost". Seems we fail in retrieving info.body.
> Please check what this is and why it is working differently than in other
> places.
> 
> I think we should be able to use the same install method.
> 

Removed the the install method from the previous patch.
Updated the current _handleEngineInstall method, now it checks for the current open URL hostname.

> @@ +400,5 @@
> > +    var md = new modalDialog.modalDialog(this._controller.window);
> > +
> > +    md.start(this._handleEngineInstallNoDomainCheck);
> > +
> > +    var name = aAddEngineCallback().replace(/ *\([^)]*\) */g, "").trim();
> 
> What does this do? Please add a comment whenever you use a regex.
> Will this work for all search engine names?
> 

Sometimes the search engines have parenthesis in their name, this will remove them as isEngineInstalled() method used below returns the search engine without brackets.

> Please remove most empty newlines you have, keep them for separating logical
> blocks, don't be excessive.
> 

Fixed

> 
> > +  // Open the engine manager and click "Get more search engines..."
> > +  searchBar.openEngineManager(enginesHandler);
> 
> I'd keep the handler callback function inline as an anonymous function, it
> is pretty basic.
> 

Refactored a lot of code added an inline callback.

> @@ +36,5 @@
> > +  // Open the engine manager and click "Get more search engines..."
> > +  searchBar.openEngineManager(enginesHandler);
> > +
> > +  assert.waitFor(() => (controller.tabs.length === (tabCount + 1)),
> > +                 "The 'Get More Engines' link has been opened in a new tab");
> 
> These last 3 lines can be done with the tabbrowser library with a call to
> openTab() with a custom callback.
> That will also handle waiting for the tab to finish opening (animation-wise)
> 

Using openTab() method now.


> > +  // check if correct url has been opened
> > +  utils.assertLoadedUrlEqual(controller, TEST_DATA);
> 
> This doesn't work as you're hardcoding an en-US specific URL.

Changed to get the locale from properties and add it to the url string.


> @@ +55,5 @@
> > + *        MozMillController of the window to operate on
> > + */
> > +function enginesHandler(aController) {
> > +  // Click Browse link - dialog will close automatically
> > +  var browseLink = findElement.ID(aController.window.document, "addEngines");
> 
> You can get this via the search lib: `getElemet({ type: "more_engines" })`
> 

Changed to use getMoreSearchEngines() method from engineManager

> 
> @@ +64,5 @@
> > + * Click the "Add to Firefox" button from the addons page
> > + */
> > +function installFromAddonPageHandler() {
> > +  // get first add to firefox link
> > +  var link = findElement.Selector(controller.tabs.activeTab, '.clickHijack');
> 
> Please use the actual link: ".install-button > a"
> 
> @@ +67,5 @@
> > +  // get first add to firefox link
> > +  var link = findElement.Selector(controller.tabs.activeTab, '.clickHijack');
> > +
> > +  // trigger mouseover for the add to firefox link
> > +  link.mouseOver();
> 
> Is this still needed?

Changed the selector for ".install-button > a", the mouseOver() is required as the button element won't be visible if not triggered.

Comment 7

[Andrei Eftimie]

Comment on attachment 8516682 [details] [diff] [review]
b941477v2.patch

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

Good work, there are still some things we'll need to address, but this is shaping up nicely.

::: firefox/lib/search.js
@@ +197,2 @@
>      // Check that the correct domain is shown
> +    let hostURL = mozmill.getBrowserController().tabs.activeTabWindow.document.location.hostname;

*sigh*, since this method is called from within modal-dialog, this get bound to the md observer, and that's not what we'd like, because we should be able to use `this.controller` here.

To fix this we need to bind this class when passing this method as a handler, see below.

@@ +363,5 @@
> +                 "Callback for adding an engine has been specified");
> +
> +    // Open installer dialog
> +    var md = new modalDialog.modalDialog(this._controller.window);
> +    md.start(this._handleEngineInstall);

The following fixes the reference issues we have in `_handleEngineInstall` as this will point to this class, not the dialog observer:
> md.start(this._handleEngineInstall.bind(this));

With this you can access the controller with `this.controller` in the handler function.
(Please also make this change in the other install method where we use `_handleEngineInstall`)

@@ +367,5 @@
> +    md.start(this._handleEngineInstall);
> +
> +    // Return engine name stripping it of
> +    // eventual brackets and trailing whitespaces
> +    var name = aAddEngineCallback().replace(/ *\([^)]*\) */g, "");

This does work for DuckDuckGo as the name in AMO is "DuckDuckGo (HTTPS / SSL)" and the keyword name in the Search Engine Manager is "DuckDuckGo". But this will fail for other addons like: "Private Anonymous Search (Google + Bing + HTTPS)" => "PrivateLee HTTPS".

We take the first addon which currently is DuckDuckGo, but if the list changes we risc this failing.

From the names I see there appear to be 2 distinct fields (ling AMO name, and keyword name for appearance in the Search Engine manager.

Instead of manipulating the name, we should make sure to check the same field. Unfortunately it seems that in each place we only have access to one of them...

The only place I have seen the actual name is the Install Modal Dialog content, so we might be able to take the proper name in `_handleEngineInstall`

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +17,5 @@
> +  aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  tabBrowser.closeAllTabs();

We'll need to clean up and try removing the installed engine here, preferably through an API call (search > removeEngine)

@@ +24,5 @@
> +/**
> + * Test add search engine from the add-ons page
> + */
> +function testAddSearchEngine() {
> +  // store number of tabs before opening a new tab

nit: Please uppercase the first letter of the phrase (applies to all comments)

@@ +28,5 @@
> +  // store number of tabs before opening a new tab
> +  let tabsLength = tabBrowser.length;
> +
> +  // Open the engine manager and click "Get more search engines..."
> +  searchBar.openEngineManager((aController) => {

nit: you can remove the parentheses around the argument

@@ +42,5 @@
> +  controller.waitForPageLoad();
> +
> +  // check if correct url has been opened
> +  let locale = prefs.preferences.getPref("general.useragent.locale", "");
> +  let url = "https://addons.mozilla.org/" + locale + "/firefox/search/?atype=4";

I'm not sure we should keep this check. It can always change, and we shouldn't bound ourselves to keep track of it.

@@ +46,5 @@
> +  let url = "https://addons.mozilla.org/" + locale + "/firefox/search/?atype=4";
> +  utils.assertLoadedUrlEqual(controller, url);
> +
> +  // click on the "Add to firefox" button
> +  engineManager.installFromAddonPage(installFromAddonPageHandler);

Just add the handler here directly as an anonymous function, no need for keep it separate.

@@ +70,5 @@
> +  // Click the link open the "Add to Firefox" dialog
> +  link.click();
> +
> +  // Return search engine name for checking if the installation was successful
> +  return link.getNode().parentNode.parentNode.getAttribute("data-name");

As said above, I would much rather we get the proper name from the Install Dialog, so this can probably be removed.

Comment 8

[Teodor Druta]

Attachment: b941477v3.patch

(In reply to Andrei Eftimie from comment #7)
> -----------------------------------------------------------------
> 
> Good work, there are still some things we'll need to address, but this is
> shaping up nicely.
> 
> 
> To fix this we need to bind this class when passing this method as a
> handler, see below.
> > md.start(this._handleEngineInstall.bind(this));
> 

Modified the code to use .bind(this);


> This does work for DuckDuckGo as the name in AMO is "DuckDuckGo (HTTPS /
> SSL)" and the keyword name in the Search Engine Manager is "DuckDuckGo". But
> this will fail for other addons like: "Private Anonymous Search (Google +
> Bing + HTTPS)" => "PrivateLee HTTPS".
> 
> We take the first addon which currently is DuckDuckGo, but if the list
> changes we risc this failing.
> 
> From the names I see there appear to be 2 distinct fields (ling AMO name,
> and keyword name for appearance in the Search Engine manager.
> 
> Instead of manipulating the name, we should make sure to check the same
> field. Unfortunately it seems that in each place we only have access to one
> of them...
> 
> The only place I have seen the actual name is the Install Modal Dialog
> content, so we might be able to take the proper name in
> `_handleEngineInstall`
> 

Added a new private method _latestInstalledEngineName for the engineManager class.
This initially is null and gets a value after a search engine has been installed

> ::: firefox/tests/remote/testAddons/testAddSearchEngine.js
> @@ +17,5 @@
> > +  aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);
> > +}
> > +
> > +function teardownModule(aModule) {
> > +  tabBrowser.closeAllTabs();
> 
> We'll need to clean up and try removing the installed engine here,
> preferably through an API call (search > removeEngine)

We can't remove the engine from the test itself as the API removeEngine method requires the name as a parameter which we don't have access in the test.
Used restoreDefaultEngines() which does the same thing in this case.


> nit: Please uppercase the first letter of the phrase (applies to all
> comments)

Fixed.

> @@ +28,5 @@
> > +  // store number of tabs before opening a new tab
> > +  let tabsLength = tabBrowser.length;
> > +
> > +  // Open the engine manager and click "Get more search engines..."
> > +  searchBar.openEngineManager((aController) => {
> 
> nit: you can remove the parentheses around the argument
> 

Fixed.

> @@ +42,5 @@
> > +  controller.waitForPageLoad();
> > +
> > +  // check if correct url has been opened
> > +  let locale = prefs.preferences.getPref("general.useragent.locale", "");
> > +  let url = "https://addons.mozilla.org/" + locale + "/firefox/search/?atype=4";
> 
> I'm not sure we should keep this check. It can always change, and we
> shouldn't bound ourselves to keep track of it.
> 

Removed this code.

> @@ +46,5 @@
> > +  let url = "https://addons.mozilla.org/" + locale + "/firefox/search/?atype=4";
> > +  utils.assertLoadedUrlEqual(controller, url);
> > +
> > +  // click on the "Add to firefox" button
> > +  engineManager.installFromAddonPage(installFromAddonPageHandler);
> 
> Just add the handler here directly as an anonymous function, no need for
> keep it separate.

Fixed.

> @@ +70,5 @@
> > +  // Click the link open the "Add to Firefox" dialog
> > +  link.click();
> > +
> > +  // Return search engine name for checking if the installation was successful
> > +  return link.getNode().parentNode.parentNode.getAttribute("data-name");
> 
> As said above, I would much rather we get the proper name from the Install
> Dialog, so this can probably be removed.

Fixed, now we are getting the name from the Install Dialog.

Comment 9

[Teodor Druta]

Other changes for attachment 8517533 [details] [diff] [review]:

The installFromUrl() and installFromAddonPage() method have 90% similar code.
I have added a new private method _install for EngineManager Class which is called by those two public methods.

Comment 10

[Andrei Eftimie]

Comment on attachment 8517533 [details] [diff] [review]
b941477v3.patch

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

Nice work, looks way better now.
With the refactoring there's still a few issues to fix.

::: firefox/lib/search.js
@@ +193,5 @@
>  
>      expect.equal(title, confirmTitle, "Window contains search engine title");
>  
> +    // Get Search Engine Installer dialog content
> +    var infoBody = aController.window.document.getElementById("info.body");

Leave this after the hostURL declaration, you can remove the above comment, and use var for hostURL for consistency (or let for both).

@@ +203,5 @@
> +    }, "Search Engine URL contains the '" + hostURL + "' domain");
> +
> +    // Retrieve the name of the search engine which will be installed,
> +    // match for the first occurence of a pair of double quotes
> +    this._latestInstalledEngineName = infoBody.textContent.match(/\"([^|]*)\"/)[1];

Just to have another alternative on the table:
> split('"')[1]

(you can leave the regex, I just don't personally like them)

@@ +223,5 @@
> +   *        Callback to trigger the installation of the engine
> +   *
> +   * @private
> +   */
> +  _install : function engineManager_install(aName, aUrl, aAddEngineCallback) {

Leave this method only with the callback and the name as a second argument.

@@ +230,5 @@
> +
> +    if (aUrl) {
> +      this._controller.open(aUrl);
> +      this._controller.waitForPageLoad();
> +    }

This step should be part of `installFromUrl`

@@ +253,5 @@
> +      md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> +
> +      // Handle the case when the engine is installed from the add-ons page
> +      // and we don't know the name beforehand
> +      aName = (!aName) ? this._latestInstalledEngineName : aName;

If `aName` has been passed in, it should be equal to `this._latestInstalledEngineName`, we can probably assert that.

Also never reassign an argument variable, use a local variable.
With the above assert we can take the value from _latestInstalledEngineName

@@ +262,5 @@
> +    finally {
> +      Services.obs.removeObserver(observer, TOPIC_SEARCH_ENGINE_MODIFIED);
> +    }
> +
> +    // Check if the local engine has been installed

This not always a local engine anymore.

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +48,5 @@
> +    let link = findElement.Selector(controller.window.document,
> +                                    ".install-button > a");
> +    // Trigger mouseover for the add to firefox link to make the link visible
> +    link.mouseOver();
> +    // Check if link is visible

Replace this comment with an empty line.

@@ +51,5 @@
> +    link.mouseOver();
> +    // Check if link is visible
> +    assert.waitFor(() => utils.isDisplayed(controller, link),
> +                                           "Link has been found !");
> +    // Click the link open the "Add to Firefox" dialog

Please remove this comment.

Comment 11

[Teodor Druta]

Attachment: b941477v4.patch

(In reply to Andrei Eftimie from comment #10)
> 
> ::: firefox/lib/search.js
> @@ +193,5 @@
> >  
> >      expect.equal(title, confirmTitle, "Window contains search engine title");
> >  
> > +    // Get Search Engine Installer dialog content
> > +    var infoBody = aController.window.document.getElementById("info.body");
> 
> Leave this after the hostURL declaration, you can remove the above comment,
> and use var for hostURL for consistency (or let for both).
> 

Fixed.

> 
> @@ +223,5 @@
> > +   *        Callback to trigger the installation of the engine
> > +   *
> > +   * @private
> > +   */
> > +  _install : function engineManager_install(aName, aUrl, aAddEngineCallback) {
> 
> Leave this method only with the callback and the name as a second argument.
> 
> @@ +230,5 @@
> > +
> > +    if (aUrl) {
> > +      this._controller.open(aUrl);
> > +      this._controller.waitForPageLoad();
> > +    }
> 
> This step should be part of `installFromUrl`
> 

Modified, also small changes through installFromAddonPage.

> @@ +253,5 @@
> > +      md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> > +
> > +      // Handle the case when the engine is installed from the add-ons page
> > +      // and we don't know the name beforehand
> > +      aName = (!aName) ? this._latestInstalledEngineName : aName;
> 
> If `aName` has been passed in, it should be equal to
> `this._latestInstalledEngineName`, we can probably assert that.
> 
> Also never reassign an argument variable, use a local variable.
> With the above assert we can take the value from _latestInstalledEngineName
> 

Fixed, added an assert to test the argument variable value with _latestInstalledEngineName

Fixed comments in testAddSearchEngine.js.

Comment 12

[Andrei Eftimie]

Comment on attachment 8518170 [details] [diff] [review]
b941477v4.patch

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

Just a couple changes regarding how we handle _latestInstalledEngineName

::: firefox/lib/search.js
@@ +192,5 @@
>      }
>  
>      expect.equal(title, confirmTitle, "Window contains search engine title");
>  
> +

nit: please remove this extra added line

@@ +224,5 @@
> +  _install : function engineManager_install(aAddEngineCallback, aName=null) {
> +    assert.equal(typeof aAddEngineCallback, "function",
> +                 "Callback for adding an engine has been specified");
> +
> +    var name = aName;

Remove this declaration from here.

@@ +247,5 @@
> +      md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> +
> +      // Handle the case when the engine is installed from the add-ons page
> +      // and we don't know the name beforehand
> +      name = (!name) ? this._latestInstalledEngineName : aName;

Move this after the name assert, and you can leave it as name = this._latestInstalledEngineName;

@@ +251,5 @@
> +      name = (!name) ? this._latestInstalledEngineName : aName;
> +
> +      // Check if name matches the latest installed engine name
> +      assert.equal(name, this._latestInstalledEngineName,
> +                   "Installed engine name match");

Do this assert before the assignment to name, directly against aName, if aName is specified.

Comment 13

[Teodor Druta]

Attachment: b941477v5.patch

(In reply to Andrei Eftimie from comment #12)

Removed surplus empty lines.

> Remove this declaration from here.
> 
> Move this after the name assert, and you can leave it as name =
> this._latestInstalledEngineName;

> Do this assert before the assignment to name, directly against aName, if
> aName is specified.

Modified in _install() method.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8518839 [details] [diff] [review]
b941477v5.patch

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

::: firefox/lib/search.js
@@ +193,5 @@
>  
>      expect.equal(title, confirmTitle, "Window contains search engine title");
>  
>      // Check that the correct domain is shown
> +    let hostURL = this._controller.tabs.activeTabWindow.document.location.hostname;

nit: It's not a URL but the hostname

@@ +201,5 @@
> +    }, "Search Engine URL contains the '" + hostURL + "' domain");
> +
> +    // Retrieve the name of the search engine which will be installed,
> +    // match for the first occurence of a pair of double quotes
> +    this._latestInstalledEngineName = infoBody.textContent.match(/\"([^|]*)\"/)[1];

I would set the engine to be installed inside of _install and verify it here. We shouldn't install it if it fails the check. Also make sure to name the variable correctly. We shouldn't have to declare this in the constructor, when you delete it right here again.

@@ +225,5 @@
> +                 "Callback for adding an engine has been specified");
> +
> +    // Create a modal dialog instance to handle the installation dialog
> +    var md = new modalDialog.modalDialog(this._controller.window);
> +    md.start(this._handleEngineInstall.bind(this));

Please move this code down before the try block.

@@ +246,5 @@
> +      // Check if aName matches the latest installed engine name
> +      if (aName) {
> +        assert.equal(aName, this._latestInstalledEngineName,
> +                     "Installed engine name match");
> +      }

As mentioned above please check it in the installation dialog.

@@ +258,5 @@
> +      Services.obs.removeObserver(observer, TOPIC_SEARCH_ENGINE_MODIFIED);
> +    }
> +
> +    // Check if the engine has been installed
> +    var searchBarInstance = new searchBar(this._controller);

The EngineManager should get a Browser instance as parameter to the constructor. Similar to the search bar class. That would make things easier here. Please add a TODO here and file a bug which will do exactly that.

I would rename `searchBarInstance` to `searchbar`.

@@ +260,5 @@
> +
> +    // Check if the engine has been installed
> +    var searchBarInstance = new searchBar(this._controller);
> +    assert.waitFor(function () {
> +      return searchBarInstance.isEngineInstalled(name);

Why not a fat arrow?

@@ +265,5 @@
> +    }, "Search engine '" + name + "' has been installed");
> +
> +    // The engine should not be selected by default
> +    expect.notEqual(searchBarInstance.selectedEngine, name,
> +                    "Search engine has been selected");

The message is wrong. It's exactly the opposite as what you are testing.

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +6,5 @@
> +
> +// Include necessary modules
> +var prefs = require("../../../lib/prefs");
> +var tabs = require("../../../lib/tabs");
> +var search = require("../../../lib/search");

nit: alphabetical order please.

@@ +24,5 @@
> +
> +/**
> + * Test add search engine from the add-ons page
> + */
> +function testAddSearchEngine() {

This test replaces everything what we test already in functional/testSearch/testGetMoreSearchEngines.js. I feel we can remove the latter test.

@@ +36,5 @@
> +      let engineManager = new search.engineManager(aController);
> +      engineManager.getMoreSearchEngines();
> +    }});
> +    expect.equal(tabBrowser.length, tabsLength + 1,
> +                 "Tab has been opened via '" + tabsLength + "'");

Please add a blank line above. Without it's hard to read. Also I would put the comment from above about number of tabs here, and explain why we have to do that check.

Comment 15

[Teodor Druta]

Attachment: b941477v6.patch

(In reply to Henrik Skupin (:whimboo) from comment #14)

> > +    let hostURL = this._controller.tabs.activeTabWindow.document.location.hostname;
> nit: It's not a URL but the hostname

Updated to hostname

> 
> @@ +201,5 @@
> > +    }, "Search Engine URL contains the '" + hostURL + "' domain");
> > +
> > +    // Retrieve the name of the search engine which will be installed,
> > +    // match for the first occurence of a pair of double quotes
> > +    this._latestInstalledEngineName = infoBody.textContent.match(/\"([^|]*)\"/)[1];
> 
> I would set the engine to be installed inside of _install and verify it
> here. We shouldn't install it if it fails the check. Also make sure to name
> the variable correctly. We shouldn't have to declare this in the
> constructor, when you delete it right here again.
> 
> @@ +246,5 @@
> > +      // Check if aName matches the latest installed engine name
> > +      if (aName) {
> > +        assert.equal(aName, this._latestInstalledEngineName,
> > +                     "Installed engine name match");
> > +      }
> 
> As mentioned above please check it in the installation dialog.
>

Assigned the aName method parameter to a class scope private parameter (engineToBeInstalled) moved the check to the _handleEngineInstall method.

> @@ +225,5 @@
> > +                 "Callback for adding an engine has been specified");
> > +
> > +    // Create a modal dialog instance to handle the installation dialog
> > +    var md = new modalDialog.modalDialog(this._controller.window);
> > +    md.start(this._handleEngineInstall.bind(this));
> 
> Please move this code down before the try block.
> 

Fixed.

> 
> @@ +258,5 @@
> > +      Services.obs.removeObserver(observer, TOPIC_SEARCH_ENGINE_MODIFIED);
> > +    }
> > +
> > +    // Check if the engine has been installed
> > +    var searchBarInstance = new searchBar(this._controller);
> 
> The EngineManager should get a Browser instance as parameter to the
> constructor. Similar to the search bar class. That would make things easier
> here. Please add a TODO here and file a bug which will do exactly that.
> 
> I would rename `searchBarInstance` to `searchbar`.
> 

Fixed variable name, added a TODO to the engineManager class constructor, filed Bug 1098233.

>
> Why not a fat arrow?
>

Updated to fat arrow everywhere it applied.
 
> @@ +24,5 @@
> > +
> > +/**
> > + * Test add search engine from the add-ons page
> > + */
> > +function testAddSearchEngine() {
> 
> This test replaces everything what we test already in
> functional/testSearch/testGetMoreSearchEngines.js. I feel we can remove the
> latter test.

Renmoved the test, updated corresponding manifest.ini.

Other changes: Updated wrong messages, added blank lines where needed, resolved nits.

Comment 16

[Andrei Eftimie]

Comment on attachment 8522161 [details] [diff] [review]
b941477v6.patch

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

::: firefox/lib/search.js
@@ +205,5 @@
> +
> +    // Check if engine to be installed name matches the latest installed engine name
> +    if (this._engineToBeInstalled) {
> +      assert.equal(this._engineToBeInstalled, this._latestInstalledEngineName,
> +                   "Engine to be installed name match");

Attempted to install the correct engine. Tried: this._latestInstalledEngineName, expected: this._engineToBeInstalled.

@@ +206,5 @@
> +    // Check if engine to be installed name matches the latest installed engine name
> +    if (this._engineToBeInstalled) {
> +      assert.equal(this._engineToBeInstalled, this._latestInstalledEngineName,
> +                   "Engine to be installed name match");
> +    }

Also in case of a failure here, we don't handle the Install Dialog window anywhere, so we'll fail with a Disconnect Error.
If this fails we need to close the Install Dialog with cancel/esc.

@@ +228,5 @@
> +  _install : function engineManager_install(aAddEngineCallback, aName=null) {
> +    assert.equal(typeof aAddEngineCallback, "function",
> +                 "Callback for adding an engine has been specified");
> +
> +    this._engineToBeInstalled = aName;

At the end of the _install method we need to remove this property.

@@ +250,5 @@
> +      aAddEngineCallback();
> +
> +      md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> +
> +      var name = this._latestInstalledEngineName;

Move this assignment after the waitFor, this way we won't risc race conditions.
EDIT. Even better this should probably live out of the try/finally block

@@ +253,5 @@
> +
> +      var name = this._latestInstalledEngineName;
> +
> +      assert.waitFor(() => engineAdded,
> +                     "Search engine '" + name + "' has been installed");

also use this._latestInstalledEngineName in the message

@@ +266,5 @@
> +                   "Search engine '" + name + "' has been installed");
> +
> +    // The engine should not be selected by default
> +    expect.notEqual(searchbar.selectedEngine, name,
> +                    "Search engine has not been selected by default");

"...has been selected as default"

Comment 17

[Teodor Druta]

Attachment: b941477v7.patch

(In reply to Andrei Eftimie from comment #16)
> Comment on attachment 8522161 [details] [diff] [review]
> b941477v6.patch
> 
> Review of attachment 8522161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/search.js
> @@ +205,5 @@
> > +
> > +    // Check if engine to be installed name matches the latest installed engine name
> > +    if (this._engineToBeInstalled) {
> > +      assert.equal(this._engineToBeInstalled, this._latestInstalledEngineName,
> > +                   "Engine to be installed name match");
> 
> Attempted to install the correct engine. Tried:
> this._latestInstalledEngineName, expected: this._engineToBeInstalled.
> 
> @@ +206,5 @@
> > +    // Check if engine to be installed name matches the latest installed engine name
> > +    if (this._engineToBeInstalled) {
> > +      assert.equal(this._engineToBeInstalled, this._latestInstalledEngineName,
> > +                   "Engine to be installed name match");
> > +    }
> 
> Also in case of a failure here, we don't handle the Install Dialog window
> anywhere, so we'll fail with a Disconnect Error.
> If this fails we need to close the Install Dialog with cancel/esc.
> 

Fixed message. Updated the code in case of non-equal names it will close the install dialog window and return a failure. 

> @@ +228,5 @@
> > +  _install : function engineManager_install(aAddEngineCallback, aName=null) {
> > +    assert.equal(typeof aAddEngineCallback, "function",
> > +                 "Callback for adding an engine has been specified");
> > +
> > +    this._engineToBeInstalled = aName;
> 
> At the end of the _install method we need to remove this property.
> 

Fixed.

> @@ +250,5 @@
> > +      aAddEngineCallback();
> > +
> > +      md.waitForDialog(TIMEOUT_INSTALL_DIALOG);
> > +
> > +      var name = this._latestInstalledEngineName;
> 
> Move this assignment after the waitFor, this way we won't risc race
> conditions.
> EDIT. Even better this should probably live out of the try/finally block
> 

This assignment cannot be moved outside the try/finally block, because of the removeObserver() function from finally part.
Moved the assignment after the waitFor.

> @@ +253,5 @@
> > +
> > +      var name = this._latestInstalledEngineName;
> > +
> > +      assert.waitFor(() => engineAdded,
> > +                     "Search engine '" + name + "' has been installed");
> 
> also use this._latestInstalledEngineName in the message
> 
> @@ +266,5 @@
> > +                   "Search engine '" + name + "' has been installed");
> > +
> > +    // The engine should not be selected by default
> > +    expect.notEqual(searchbar.selectedEngine, name,
> > +                    "Search engine has not been selected by default");
> 
> "...has been selected as default"

Fixed.

Comment 18

[Andrei Eftimie]

Comment on attachment 8522262 [details] [diff] [review]
b941477v7.patch

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

Just a few nits from me. With this updates please request a review from Henrik.

::: firefox/lib/search.js
@@ +208,5 @@
> +        this._engineToBeInstalled !== this._latestInstalledEngineName) {
> +      aController.keypress(null, "VK_ESCAPE", {});
> +      assert.fail("Attempted to install the correct engine. " +
> +                  "Tried: " + this._latestInstalledEngineName + ", " +
> +                  "expected: " + this._engineToBeInstalled + ".");

Put single quotes around the search engine names please.

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +// Include necessary modules
> +var prefs = require("../../../lib/prefs");

This is not used in the test, you can remove it.

@@ +6,5 @@
> +
> +// Include necessary modules
> +var prefs = require("../../../lib/prefs");
> +var search = require("../../../lib/search");
> +var tabs = require("../../../lib/tabs");

You can also remove this with transitioning to browserWindow

@@ +10,5 @@
> +var tabs = require("../../../lib/tabs");
> +var utils = require("../../../../lib/utils");
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();

Please replace this with `aModule.browserWindow = new browser.BrowserWindow();` and all `controller` instances with `browserWindow.controller`

@@ +13,5 @@
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.engineManager = new search.engineManager(aModule.controller);
> +  aModule.searchBar = new search.searchBar(aModule.controller);
> +  aModule.tabBrowser = new tabs.tabBrowser(aModule.controller);

With `browserWindow` you can also remove this line, we have access to `tabs` directly via `browserWindow.tabs`

@@ +30,5 @@
> +
> +  // Open the engine manager and click "Get more search engines..."
> +  searchBar.openEngineManager(aController => {
> +    tabBrowser.openTab({method: "callback", callback: () => {
> +      // Click "Get more search engines" link - dialog will close automatically

I think this comment can be removed.

Comment 19

[Teodor Druta]

Attachment: b941477v8.patch

Removed tabs and prefs as required modules from testAddSearchEngine.
Updated testAddSearchEngine to use BrowserWindow.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8525347 [details] [diff] [review]
b941477v8.patch

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

::: firefox/lib/search.js
@@ +208,5 @@
> +        this._engineToBeInstalled !== this._latestInstalledEngineName) {
> +      aController.keypress(null, "VK_ESCAPE", {});
> +      assert.fail("Attempted to install the correct engine. " +
> +                  "Tried: '" + this._latestInstalledEngineName + "', " +
> +                  "expected: '" + this._engineToBeInstalled + "'.");

I do not see why we should use such a complexity. Why doesn't it simply work with assert.notEqual()?

Also I do not understand the code by the comment. Why do we have to compare the engine to be installed with another engine as installed earlier? This doesn't make sense.

@@ +246,5 @@
> +    Services.obs.addObserver(observer, TOPIC_SEARCH_ENGINE_MODIFIED, false);
> +
> +    // Create a modal dialog instance to handle the installation dialog
> +    var md = new modalDialog.modalDialog(this._controller.window);
> +    md.start(this._handleEngineInstall.bind(this));

Hm. Maybe we could add another parameter to _handleEngineInstall for the engine to be installed? That way we could safe us from the extra temporary variable.

@@ +256,5 @@
> +
> +      assert.waitFor(() => engineAdded,
> +                     "Search engine '" + this._latestInstalledEngineName + "' has been installed");
> +
> +      var name = this._latestInstalledEngineName;

Not necessary. See below.

@@ +265,5 @@
> +
> +    // Check if the engine has been installed
> +    var searchbar = new searchBar(this._controller);
> +    assert.waitFor(() => searchbar.isEngineInstalled(name),
> +                   "Search engine '" + name + "' has been installed");

You do not want to check for the engine, which has been installed, but the engine which as expected to be installed. Otherwise you will miss if some other engine has been installed.

::: firefox/tests/remote/testAddons/testAddSearchEngine.js
@@ +34,5 @@
> +    }});
> +
> +    // Check if a new tab for search add-ons page has been opened
> +    expect.equal(browserWindow.tabs.length, tabsLength + 1,
> +                 "Tab has been opened via '" + tabsLength + "'");

I do not see why we have to test that a new tab has been opened. Exactly that check has been performed in the tabs class already. What you should do instead is to check that the right page/domain has been opened.

Comment 21

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 8525347 [details] [diff] [review]
> b941477v8.patch
> 
> Review of attachment 8525347 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/search.js
> @@ +208,5 @@
> > +        this._engineToBeInstalled !== this._latestInstalledEngineName) {
> > +      aController.keypress(null, "VK_ESCAPE", {});
> > +      assert.fail("Attempted to install the correct engine. " +
> > +                  "Tried: '" + this._latestInstalledEngineName + "', " +
> > +                  "expected: '" + this._engineToBeInstalled + "'.");
> 
> I do not see why we should use such a complexity. Why doesn't it simply work
> with assert.notEqual()?
> 
> Also I do not understand the code by the comment. Why do we have to compare
> the engine to be installed with another engine as installed earlier? This
> doesn't make sense.
> 

We are comparing the engine name set as a parameter (aName) for the _install method, with the engine name from the engine install dialog, if those do not match we close the dialog and return a failure so the test would not continue.

> @@ +246,5 @@
> > +    Services.obs.addObserver(observer, TOPIC_SEARCH_ENGINE_MODIFIED, false);
> > +
> > +    // Create a modal dialog instance to handle the installation dialog
> > +    var md = new modalDialog.modalDialog(this._controller.window);
> > +    md.start(this._handleEngineInstall.bind(this));
> 
> Hm. Maybe we could add another parameter to _handleEngineInstall for the
> engine to be installed? That way we could safe us from the extra temporary
> variable.
> 

We can't add a parameter for the _handleEngineInstall method as it is a callback function for a ModalDialog object.

> @@ +256,5 @@
> > +
> > +      assert.waitFor(() => engineAdded,
> > +                     "Search engine '" + this._latestInstalledEngineName + "' has been installed");
> > +
> > +      var name = this._latestInstalledEngineName;
> 
> Not necessary. See below.
> 
> @@ +265,5 @@
> > +
> > +    // Check if the engine has been installed
> > +    var searchbar = new searchBar(this._controller);
> > +    assert.waitFor(() => searchbar.isEngineInstalled(name),
> > +                   "Search engine '" + name + "' has been installed");
> 
> You do not want to check for the engine, which has been installed, but the
> engine which as expected to be installed. Otherwise you will miss if some
> other engine has been installed.
> 

in the _install method after add engine callback finishes the engine should be already installed, so we check if engineAdded was set to true in the observer.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Teodor Druta from comment #21)
> > ::: firefox/lib/search.js
> > @@ +208,5 @@
> > > +        this._engineToBeInstalled !== this._latestInstalledEngineName) {
> > > +      aController.keypress(null, "VK_ESCAPE", {});
> > > +      assert.fail("Attempted to install the correct engine. " +
> > > +                  "Tried: '" + this._latestInstalledEngineName + "', " +
> > > +                  "expected: '" + this._engineToBeInstalled + "'.");
> > 
> > I do not see why we should use such a complexity. Why doesn't it simply work
> > with assert.notEqual()?
> > 
> > Also I do not understand the code by the comment. Why do we have to compare
> > the engine to be installed with another engine as installed earlier? This
> > doesn't make sense.
> > 
> 
> We are comparing the engine name set as a parameter (aName) for the _install
> method, with the engine name from the engine install dialog, if those do not
> match we close the dialog and return a failure so the test would not
> continue.

This is not what the current comment reflects: "// Check if engine to be installed name matches the latest installed engine name".

To make this clear it should be like: "Check that the name of the engine to be installed matches the one in the installation dialog".

> > Hm. Maybe we could add another parameter to _handleEngineInstall for the
> > engine to be installed? That way we could safe us from the extra temporary
> > variable.
> > 
> 
> We can't add a parameter for the _handleEngineInstall method as it is a
> callback function for a ModalDialog object.

Hm, and we do not pass through parameters, right? That would be a nice addition for another bug. So ensure to name the temporary class variable accordingly.

> > > +    // Check if the engine has been installed
> > > +    var searchbar = new searchBar(this._controller);
> > > +    assert.waitFor(() => searchbar.isEngineInstalled(name),
> > > +                   "Search engine '" + name + "' has been installed");
> > 
> > You do not want to check for the engine, which has been installed, but the
> > engine which as expected to be installed. Otherwise you will miss if some
> > other engine has been installed.
> 
> in the _install method after add engine callback finishes the engine should
> be already installed, so we check if engineAdded was set to true in the
> observer.

Please re-read my comment. You misread it. You are checking for the installed engine here, but not for the expected engine to be installed.

Comment 23

[Teodor Druta]

> Please re-read my comment. You misread it. You are checking for the installed engine here, but not for > the expected engine to be installed.

That's because if we install the engine from addons page not knowing beforehand the engine name the _engineToBeInstalled property would be null. 
_engineToBeInstalled is used only when an engine is installed from an url and the engine name is specified beforehand.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The test can perfectly retrieve the engine name which will be installed from the AMO page. So I do not see the problem here.

Comment 25

[Teodor Druta]

I will not be able to finish the work on this bug. The test from the latest attached patch stopped working with release and up, because of the latest firefox search bar UI.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.