1105780 - Refactor search.js library

Status: RESOLVED INVALID

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Teodor Druta]

The search.js library needs to be refactored, due to recent changes in the Search Bar UI (Bug 1088660).

 - engineManager class will move from search.js and become a refactored searchPane class in prefs.js
 - all the backend (API) methods will remain in search.js and it will behave as a "static object" with static methods similar to utils.js.
 - the searchBar class will move from search.js to a refactored searchBar class in toolbars.js, which will only contain methods related to the Search Bar UI.

Comment 1

[Teodor Druta]

I am stuck with implementing a get search bar suggestions ui method, because I can't seem to find an event which fires when search suggestions are "fully" populated.

I only found this function 
http://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/searchSuggestionUI.js#94-105
which I think actually waits for the element's size to grow.
I'm not sure if this is the correct way to handle this.

Florian, can you please provide some info on the matter, is there another event or observer which we could use to handle the search suggestions being populated into the autocomplete tree ?

Comment 2

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

Teodor, does the currently implemented approach not work anymore?

Comment 3

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Teodor, does the currently implemented approach not work anymore?

No, because the current approach (getSuggestions() method), waits for the suggestions popup to change its state to "open" and then waits for the tree node view to be different from null.
In the current search bar UI the suggestions popup is part of the "PopupSearchAutoComplete" which includes an engine-header for the current selected default engine, and a footer with a list of one click search engines and a "change search settings" button.
In the old searchbar UI the AutoComplete popup would appear only if there were any suggestions, in the new UI the AutoComplete popup can be opened even if there is an empty string in the searchbar input  always displaying the "Change Search Settings" button.

To be honest I don't think the old approach ever worked correctly, we are waiting for the tree node view to be different than null which will not mean that the search suggestions popup will be populated with all possible entries.

Comment 4

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

We were aware of that the current approach does not work all the time. So I'm happy with something new. Thanks for the explanation of the problem, given that I haven't checked that new search myself yet. So the best you can do is to ask the developer and not someone from QA.

Comment 5

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

Sorry, I hit enter to fast before I noticed that you already asked Florian. So please skip that part of my last comment.

Comment 6

[Florian Quèze [:florian]]

(In reply to Teodor Druta from comment #1)

> Florian, can you please provide some info on the matter, is there another
> event or observer which we could use to handle the search suggestions being
> populated into the autocomplete tree ?

I don't see anything obvious :-(.

Would repeatedly checking the height of the tree be too ugly of a workaround?

Comment 7

[Teodor Druta]

(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> I don't see anything obvious :-(.
> 
> Would repeatedly checking the height of the tree be too ugly of a workaround?

I guess it will have to do for now. Thanks, Florian.

Comment 8

[Teodor Druta]

Attachment: betav2.patch

This came to be a really big patch.

To sum things up:
  * Moved engineManager class from firefox/lib/search.js -> firefox/lib/ui/pref-window.js
    It was renamed to searchPane and also all its methods have been refactored, new methods have been added.
    Removed openPreferencesDialog method from pref-window.js, added a new open method which waits for the window state to open.
    The preferences dialog is opened now from browser.js openPreferencesDialog method with a callback function as a parameter.

  * Moved all search bar UI related methods from firefox/lib/search.js -> firefox/lib/toolbars.js
    The class remained named searchBar and all its methods have been refactored, a few new methods have been added.

  * Refactor search.js into an API library all the functions are global and exported, it was moved from firefox/lib/search.js to lib/search.js preserving history.
    Added two new functions, the init() function, according to the development documentation should make the library API calls work faster.

  * Added 3 new lib tests to check all the functionality that is handled by these 3 "new" libraries 
    firefox/lib/tests/testSearchBar.js - tests all the functionality of the searchBar class
    firefox/lib/tests/testSearchPane.js - tests all the functionality of the searchPane class
    lib/tests/testSearch.js - tests all the functionality of the search.js API library
    firefox/lib/tests/testSeach.js has been removed as it does not comply with the new libraries anymore

This patch is for "mozilla-beta" branch.

The version for "mozilla-aurora" and "default" will come once the patch from Bug 1005811 will be in a more appropiate production stage.

The current version of firefox beta uses the preferences dialog and the in content preferences are disabled by default.
The current versions of firefox aurora and nightly are using the in content preferences page and it is enabled by default.

The differences between mozilla-beta branch and mozilla-aurora/default branches (proposition):
 * searchPane class should reside in firefox/lib/ui/about-preferences-page.js
   AboutPreferencesPage class from about-preferences-page.js library will have a new method:
>  /**
>   * Returns the searchPane object
>   *
>   * @returns {object} searchPane
>   */
>  get searchPane() {
>    if (!this._searchPane) {
>      this._searchPane = new searchPane(this.controller);
>    }
>    return this._searchPane;
>  },

This method will retrieve a searchPane class object instance.

In the current version of the patch the searchPane can be opened using the "openSearchSettings" method from the searchBar class from firefox/lib/toolbars.js
This method will still exist in the future patch versions for mozilla-aurora and default branches with small changes:
In the current version of the patch we are waiting for a new preferences dialog window to open by clicking the "Open Search settings" button from the searchbar dropdown.
In the mozilla-aurora and default versions of the patch we will wait for an about preferences page to open when clicking the "Open Search settings" button.

Instead of what we have in the current proposed patch for mozilla beta:
::: firefox/lib/toolbars.js
@@ 1579,5 @@
>    var prefDialog = this.browserWindow.openPreferencesDialog(() => {
>      searchSettingsButton.waitThenClick();
>      // searchPane DOM fails to load otherwise
>      this.browserWindow.controller.sleep(0);
>    });

We will have something similar to this:

>    var prefPage = *.openPreferencesPage(() => {
>      searchSettingsButton.waitThenClick();
>    });

prefPage here will be an instance of the AboutPreferencesPage class.
And we will be able to return an instance of the searchPane: var searchPane = prefPage.searchPane;

In my investigations I got to the conclusion that the elements used in the paneSearch of the Preferences Dialog (beta) are the same as in the paneSearch of the in content preferences (aurora, nightly), so changing the patch from mozilla-beta to mozilla-aurora and default branches will result in very few to zero modifications for the searchPane class.

Comment 9

[Teodor Druta]

Just a small note, the patch misses old installFromURL and _handleEngineInstall methods from searchBar, I missed those on purpose I'll handle them in the refactoring search tests Bug 1102821.

Comment 10

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8533148 [details] [diff] [review]
betav2.patch

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

This patch is v1 :)

Please use the "function <function_name>() {" notation everywhere in the tests.

::: firefox/lib/tests/testOpenSearchSettings.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict"

nit: ; at the end

@@ +4,5 @@
> +
> +"use strict"
> +
> +// Include required modules
> +var {expect} = require("../../../lib/assertions");

We don't need this anymore

@@ +6,5 @@
> +
> +// Include required modules
> +var {expect} = require("../../../lib/assertions");
> +var modalDialog = require("../../../lib/modal-dialog");
> +var search = require("../search");

The test fails because it doesn't find the search module. The patch should be ../../../lib/search

@@ +14,5 @@
> +var browser = require("../ui/browser");
> +
> +const DELAY = 500;
> +
> +var setupModule = function(aModule) {

Please use the function <function_name>()" notation (here and everywhere else in the file)

@@ +19,5 @@
> +  aModule.browserWindow = new browser.BrowserWindow();
> +  aModule.controller = aModule.browserWindow.controller;
> +  aModule.navBar = aModule.browserWindow.navBar;
> +  // searchBar = new search.searchBar(controller);
> +  // searchBar.clear();

Please remove these comments

@@ +23,5 @@
> +  // searchBar.clear();
> +}
> +
> +var teardownModule = function(aModule) {
> +  // searchBar.clear();

Please remove this comment

@@ +27,5 @@
> +  // searchBar.clear();
> +}
> +
> +/**
> + * Add a MozSearch Search plugin

Test open search settings

@@ +37,5 @@
> +
> +  // test "search engine settings"
> +  searchBar.searchDropDownOpen = true;
> +  browserWindow.controller.sleep(1000);
> +  searchBar.openSearchSettings();

I don't see why this needs a new test file, it can live in testSearchBar.
Also, you should assert if search settings opened successfully.

::: firefox/lib/tests/testSearchBar.js
@@ +5,5 @@
> +"use strict"
> +
> +// Include required modules
> +var utils = require("../../../lib/utils");
> +var toolbars = require("../toolbars");

Please order alphabetically

@@ +32,5 @@
> +/**
> + * Test search bar UI
> + *
> + * This test requires network connection and it is not intended for production
> + * this checks all the functionality of the searchBar methods, ensuring that

* This test requires network connection and it is not intended for production.
 * This checks all the functionality of the searchBar methods, ensuring that

@@ +43,5 @@
> +  searchBar.focus({type: "shortcut"});
> +
> +  // Check open the drop down
> +  searchBar.searchDropDownOpen = true;
> +  assert.waitFor(() => searchBar.searchDropDownOpen,

It's not relevant to check for the searchBar.searchDropDownOpen value here since you set it to that value. You should assert for something else to see if the drop down indeed opened (maybe the state of the popup)

@@ +49,5 @@
> +
> +  // Check search bar type
> +  searchBar.type(TEST_DATA.searchTerm);
> +  assert.waitFor(() => searchBar.getSearchBarValue() == TEST_DATA.searchTerm,
> +                 "Search bar value has been set, got: " + searchBar.getSearchBarValue());

If this fails, we won't know for sure if it's because of a failure in the type() method or in getSearchValue(). So I suggest you use searchBar.getElement({type: "searchBar"}).getNode().value instead

@@ +50,5 @@
> +  // Check search bar type
> +  searchBar.type(TEST_DATA.searchTerm);
> +  assert.waitFor(() => searchBar.getSearchBarValue() == TEST_DATA.searchTerm,
> +                 "Search bar value has been set, got: " + searchBar.getSearchBarValue());
> +  // Check get search bar value

Add a blank line before this, please

@@ +52,5 @@
> +  assert.waitFor(() => searchBar.getSearchBarValue() == TEST_DATA.searchTerm,
> +                 "Search bar value has been set, got: " + searchBar.getSearchBarValue());
> +  // Check get search bar value
> +  assert.equal(searchBar.getSearchBarValue(),
> +               searchBar.getElement({type: "searchBar"}).getNode().value,

As defined in the lib, "searchBar.getSearchBarValue()" is the same as "searchBar.getElement({type: "searchBar"}).getNode().value". You should compare getSearchBarValue() with TEST_DATA.searchTerm

@@ +58,5 @@
> +
> +  // Check get one click search engines
> +  // The only way to check this is to check length
> +  var oneClickSearchEngines = searchBar.oneClickSearchEngines;
> +  assert.waitFor(() => oneClickSearchEngines.length > 1, "There are one click search engines");

"There are multiple items in the one click search engines list"

@@ +66,5 @@
> +  // For "abc" there should be 10 results for default "google" or new default "yahoo"
> +  // If this fails we will know that the getSuggestions() method needs improvement
> +  assert.waitFor(() => (suggestions.length === 10),
> +                 "There are 10 suggestions, got: " + suggestions.length);
> +  // No "abc" string occurence in the first suggestion => no good

// Check that the searched string appears in the suggestions from the list

@@ +67,5 @@
> +  // If this fails we will know that the getSuggestions() method needs improvement
> +  assert.waitFor(() => (suggestions.length === 10),
> +                 "There are 10 suggestions, got: " + suggestions.length);
> +  // No "abc" string occurence in the first suggestion => no good
> +  assert.waitFor(() => (suggestions[0].search(TEST_DATA.searchTerm) !== -1),

Shouldn't we check this for all suggestions?

@@ +85,5 @@
> +                 "Search bar dropdown has been opened");
> +  // Use amazon.com
> +  searchBar.oneClickSearch(TEST_DATA.oneClickSearch);
> +
> +  searchBar.checkSearchResultPage(TEST_DATA.oneClickSearch, TEST_DATA.searchTerm);

You should use an assert here instead of that function

@@ +91,5 @@
> +  // Check for search with the default selected search engine
> +  searchBar.clear();
> +  searchBar.search({text: TEST_DATA.searchTerm, action: "goButton"});
> +  browserWindow.controller.waitForPageLoad();
> +

You need an assert here to check that the result of these actions is the expected one

@@ +92,5 @@
> +  searchBar.clear();
> +  searchBar.search({text: TEST_DATA.searchTerm, action: "goButton"});
> +  browserWindow.controller.waitForPageLoad();
> +
> +  // Check Add "*" in page search engine

// Check add new search engine from a page

@@ +122,5 @@
> +               "Searchbar value matches " + TEST_DATA.searchTerm);
> +  searchBar.selectContextMenuEntry({method: "shortcut", name: "clearhistory"});
> +  assert.waitFor(() => (searchBar.getSearchBarValue() === ""),
> +                 "Searchbar history has been cleared");
> +}

To be easier to read this test, I suggest you use multiline comments for each functionality test, and single line for the rest of the comments

::: firefox/lib/tests/testSearchPane.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Add "use strict";

@@ +57,5 @@
> +  /*
> +   * Set a new default search engine
> +   */
> +  var currentDefaultSearchEngine = searchPane.selectedEngine;
> +  searchPane.selectedEngine = TEST_DATA.searchEngines[0];

You may want to check if searchEngines[0] is already set as default and use other engine if so. Also, you'll have to update the test everywhere else in this case.

@@ +72,5 @@
> +  assert.equal(oneClickSearchEngines.indexOf(searchPane.selectedEngine), -1,
> +               "Search engine '" + searchPane.selectedEngine + "' " +
> +               "has been removed from one click search engines list");
> +
> +  /*

nit: /**

@@ +75,5 @@
> +
> +  /*
> +   * Check "Provide search suggestions" checkbox
> +   */
> +  var currentSuggestionState = searchPane.suggestionsEnabled;

Rename var into initialSuggestionsState

@@ +90,5 @@
> +   * Check one click search engines state
> +   */
> +  var currentOCSEState = searchPane.getOneClickSearchEngineState(TEST_DATA.searchEngines[1]);
> +  // After changing the default search engine to Yahoo, Google should be
> +  // a one click search engine and enabled by default

nit: // an one click search engine and enabled by default

@@ +127,5 @@
> +
> +  var disabledSearchEngines = searchPane.getOneClickSearchEngines("disabled");
> +  enabledSearchEngines = searchPane.getOneClickSearchEngines("enabled");
> +
> +  // check if any diabled search engines matches an enabled one

// Check if disabled search engines don't appear as enabled

@@ +145,5 @@
> +
> +  // Test if the search engines add-ons page has opened
> +  var stringURL = browserWindow.controller.tabs.activeTabWindow.document.location.toString();
> +  assert.ok(stringURL.search("search"),
> +               "Search engines page has been opened");

Leave the message on the previous line

::: firefox/lib/toolbars.js
@@ +7,5 @@
>   * The ToolbarAPI adds support for accessing and interacting with toolbar elements
>   *
>   * @version 1.0.0
>   */
>  

Add "use strict";

@@ +14,2 @@
>  // Include required modules
>  var { assert } = require("../../lib/assertions");

We don't need this anymore

@@ +1246,5 @@
> +
> +    assert.waitFor(function () {
> +      return this.searchDropDownOpen === aNewState;
> +    }, "Search engines drop down open state has been changed. Expected: '"
> +    + aNewState + "'", undefined, undefined, this);

Please align with the + under the "

@@ +1272,5 @@
> +  get oneClickSearchEngines() {
> +    var engines = [];
> +    var enginesContainerNode = this.getElement({type: "searchBar_enginesContainer"}).getNode();
> +
> +    for (var ii = 0; ii < enginesContainerNode.childNodes.length; ii++) {

"i" instead of "ii"

@@ +1301,5 @@
> +   *
> +   * @param {string} aSearchTerm
> +   *        Text which should be checked for
> +   */
> +  checkSearchResultPage : function searchBar_checkSearchResultPage(aSearchTerm) {

I don't think this function should be in the library

@@ +1358,5 @@
> +    var activeElement = this.browserWindow.controller.window.document.activeElement;
> +    assert.equal(input.getNode(), activeElement, "Search bar text input is focused");
> +  },
> +
> + /**

Add another whitespace before the comment, please

@@ +1395,5 @@
> +    var nodeCollector = new domUtils.nodeCollector(root);
> +    var elems = [];
> +
> +    switch(aSpec.type) {
> +      case "searchBar":

Please order the cases alphabetically

@@ +1492,5 @@
> +   *         SearchBar text value
> +   */
> +  getSearchBarValue : function searchBar_getSearchBarValue() {
> +    var elem = this.getElement({type: "searchBar"});
> +    var value = elem.getNode().value;

Please use more intuitive var names (like "searchBar", "searchBarValue")

@@ +1599,5 @@
> +    button.waitThenClick();
> +  },
> +
> +  /**
> +   * Start a search with the given search term and check if the resulting URL

This function doesn't check the url, so please remove this from the description

::: firefox/lib/ui/pref-window.js
@@ +10,5 @@
>   * @version 1.0.0
>   */
>  
>  // Include required modules
>  var { assert, expect } = require("../../../lib/assertions");

We don't need this anymore

@@ +25,5 @@
>  const PREF_DIALOG_BUTTONS  = '/{"type":"prefwindow"}/anon({"anonid":"dlg-buttons"})';
>  const PREF_DIALOG_DECK     = '/{"type":"prefwindow"}/anon({"class":"paneDeckContainer"})/anon({"anonid":"paneDeck"})';
>  const PREF_DIALOG_SELECTOR = '/{"type":"prefwindow"}/anon({"orient":"vertical"})/anon({"anonid":"selector"})';
>  
>  

Extra blank line, please remove

@@ +262,5 @@
> +    return defaultEngine;
> +  },
> +
> +  /**
> +   * Select the default engine with the given name

* Sets the engine with the given name as default

@@ +303,5 @@
> +    var dtds = ["chrome://browser/locale/engineManager.dtd"];
> +    return dtds;
> +  },
> +
> + /**

Need one more blank space before starting this comment

@@ +374,5 @@
> +    return elems;
> +  },
> +
> +  /**
> +   * Clicks the "Add more search providers..." link

Nit: * Clicks the "Add more search engines..." link

::: lib/search.js
@@ +6,5 @@
>   * @fileoverview
>   * The SearchAPI adds support for search related functions like the search bar.
>   */
>  
> +"use strict"

Add ; at the end

@@ +24,5 @@
> +  var initReady = false;
> +  var promise = new Promise((resolve, reject) => {
> +    Services.search.init(() => resolve(true));
> +  });
> +  promise.then(() => {

Leave a blank line before this

@@ +27,5 @@
> +  });
> +  promise.then(() => {
> +    initReady = true;
> +  });
> +  assert.waitFor(() => initReady,

Add a blank line before

@@ +28,5 @@
> +  promise.then(() => {
> +    initReady = true;
> +  });
> +  assert.waitFor(() => initReady,
> +                "Search service has been initialized");

Leave the message on the same line

@@ +29,5 @@
> +    initReady = true;
> +  });
> +  assert.waitFor(() => initReady,
> +                "Search service has been initialized");
> +  return initReady;

Leave a blank line before return

@@ +34,4 @@
>  }
>  
>  /**
> + * Gets a search engine (API call)

Get the search engine with a specific name

@@ +41,1 @@
>   */

Please add a documentation for the returned value

@@ +46,4 @@
>  }
>  
>  /**
> + * Returns all the visible search engines (API call)

* Get all visible search engines

@@ +50,1 @@
>   */

Please add documentation for the returned value

@@ +56,1 @@
>  

Extra blank line, please remove

@@ +58,5 @@
> + * Check if a search engine supports suggestions (API call)
> + *
> + * @param {string} aName
> + *        Name of the search engine to check
> + */

Please add documentation for the returned value

@@ +72,5 @@
> + * @param {string} aName
> + *         The search engine's name. Must be unique. Must not be null.
> + * @param {string} aURL
> + *        The URL to which search queries should be sent.
> + *        Must not be null.

Move this on the previous line

@@ +87,5 @@
> +  var method = (aSpec.method == undefined) ? "GET" : aSpec.method;
> +  var selected = (aSpec.selected == undefined) ? false : aSpec.selected;
> +
> +  Services.search.addEngineWithDetails(aName, null, null, null,
> +                                       method, aURL);

You can leave all the function arguments on the same line

@@ +89,5 @@
> +
> +  Services.search.addEngineWithDetails(aName, null, null, null,
> +                                       method, aURL);
> +  if (selected) {
> +    aEngine = Services.search.getEngineByName(aName);

"var engine = ...", please.

@@ +94,5 @@
> +    Services.search.currentEngine = aEngine;
> +  }
> +}
> +
> +

Extra blank line, please remove

@@ +101,5 @@
> + *
> + * @param {string} aName
> + *        Name of the search engine to check
> + *
> + * @return {boolean} engine

* @return {boolean} True if engine is installed, false if not

@@ +145,5 @@
> +  Services.search.getEngines().forEach(function (aEngine, aIndex) {
> +    if (defaults.indexOf(aEngine) !== aIndex) {
> +      Services.search.moveEngine(aEngine, defaults.indexOf(aEngine));
> +    }
> +  });

This restore of sorting doesn't make sense anymore since we no longer can change the engines' order

@@ +150,5 @@
>  
> +  // Update the visibility status for each engine and reset the default engine
> +  Services.search.restoreDefaultEngines();
> +  Services.search.currentEngine = Services.search.defaultEngine;
> +}

The restoreDefaultEngines from Services.search is only updating the visibility of the engines, does not remove them.Maybe we should stick to that behavior and don't remove the non-default engines from the list, just mark them as hidden.

::: lib/tests/testSearch.js
@@ +30,5 @@
> +  assert.equal(typeof engines[0], "object",
> +               "Engine type, expected object, got " + typeof engines[0]);
> +
> +  assert.ok(search.hasSuggestions("Google"),
> +            "Engine has suggestions");

You can leave the message on the same line

@@ +40,5 @@
> +
> +  // Check restore defaults
> +  search.restoreDefaults();
> +  assert.ok(!search.isEngineInstalled("IMDb"),
> +            "Search engines have been restored to default");

If restore defaults only hides the engines from the list (doesn't also remove them), you should check for the visibility instead of installation here

Comment 11

[Teodor Druta]

Attachment: betav2.2.patch

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #10)
> Comment on attachment 8533148 [details] [diff] [review]
> betav2.patch
> 
> Review of attachment 8533148 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is v1 :)
> 

The first version wasn't uploaded for review, as the 2.1 version.
So the new patch is v2.2.

> Please use the "function <function_name>() {" notation everywhere in the
> tests.
> 

Updated everywhere it applied.

> 
> I don't see why this needs a new test file, it can live in testSearchBar.
> Also, you should assert if search settings opened successfully.
> 

Actually that test was just for me, I was using it on the implementation of the openSearchSettings method, I forgot to remove it, sorry about that.

> ::: firefox/lib/tests/testSearchBar.js
> @@ +43,5 @@
> > +  searchBar.focus({type: "shortcut"});
> > +
> > +  // Check open the drop down
> > +  searchBar.searchDropDownOpen = true;
> > +  assert.waitFor(() => searchBar.searchDropDownOpen,
> 
> It's not relevant to check for the searchBar.searchDropDownOpen value here
> since you set it to that value. You should assert for something else to see
> if the drop down indeed opened (maybe the state of the popup)
> 

It actually checks for the state of the popup. searchDropDownOpen is a class property with both a setter and a getter, please look over those methods in toolbars.js searchBar class.

> @@ +49,5 @@
> > +
> > +  // Check search bar type
> > +  searchBar.type(TEST_DATA.searchTerm);
> > +  assert.waitFor(() => searchBar.getSearchBarValue() == TEST_DATA.searchTerm,
> > +                 "Search bar value has been set, got: " + searchBar.getSearchBarValue());
> 
> If this fails, we won't know for sure if it's because of a failure in the
> type() method or in getSearchValue(). So I suggest you use
> searchBar.getElement({type: "searchBar"}).getNode().value instead
> 

searchBar.getSearchBarValue() actually returns searchBar.getElement({type: "searchBar"}).getNode().value, so it's the same thing.

> 
> @@ +52,5 @@
> > +  assert.waitFor(() => searchBar.getSearchBarValue() == TEST_DATA.searchTerm,
> > +                 "Search bar value has been set, got: " + searchBar.getSearchBarValue());
> > +  // Check get search bar value
> > +  assert.equal(searchBar.getSearchBarValue(),
> > +               searchBar.getElement({type: "searchBar"}).getNode().value,
> 
> As defined in the lib, "searchBar.getSearchBarValue()" is the same as
> "searchBar.getElement({type: "searchBar"}).getNode().value". You should
> compare getSearchBarValue() with TEST_DATA.searchTerm
> 

Since searchBar.getSearchBarValue() returns searchBar.getElement({type: "searchBar"}).getNode().value this check is useless, removed it.

> 
> @@ +67,5 @@
> > +  // If this fails we will know that the getSuggestions() method needs improvement
> > +  assert.waitFor(() => (suggestions.length === 10),
> > +                 "There are 10 suggestions, got: " + suggestions.length);
> > +  // No "abc" string occurence in the first suggestion => no good
> > +  assert.waitFor(() => (suggestions[0].search(TEST_DATA.searchTerm) !== -1),
> 
> Shouldn't we check this for all suggestions?
> 

We don't know for sure what suggestions will return a certain search engine, this depends on a lot of factors, such as Geographical Location, Language settings, previous searches done from the same public IP (search engines store that kind of data) etc. For all I know maybe we will get suggestions for "abc", which don't even contain "abc", so I think it's safer to just test the first suggestion.

> @@ +85,5 @@
> > +                 "Search bar dropdown has been opened");
> > +  // Use amazon.com
> > +  searchBar.oneClickSearch(TEST_DATA.oneClickSearch);
> > +
> > +  searchBar.checkSearchResultPage(TEST_DATA.oneClickSearch, TEST_DATA.searchTerm);
> 
> You should use an assert here instead of that function
> 

checkSearchResultPage does exactly that.

> @@ +91,5 @@
> > +  // Check for search with the default selected search engine
> > +  searchBar.clear();
> > +  searchBar.search({text: TEST_DATA.searchTerm, action: "goButton"});
> > +  browserWindow.controller.waitForPageLoad();
> > +
> 
> You need an assert here to check that the result of these actions is the
> expected one
> 

I missed that one, added a checkSearchResultPage, thanks.

> 
> @@ +122,5 @@
> > +               "Searchbar value matches " + TEST_DATA.searchTerm);
> > +  searchBar.selectContextMenuEntry({method: "shortcut", name: "clearhistory"});
> > +  assert.waitFor(() => (searchBar.getSearchBarValue() === ""),
> > +                 "Searchbar history has been cleared");
> > +}
> 
> To be easier to read this test, I suggest you use multiline comments for
> each functionality test, and single line for the rest of the comments
> 

I agree with that, but in this case the line will be bigger than 100 characters.

> ::: firefox/lib/tests/testSearchPane.js
> @@ +1,4 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> 
> Add "use strict";
> 

Fixed.

> @@ +57,5 @@
> > +  /*
> > +   * Set a new default search engine
> > +   */
> > +  var currentDefaultSearchEngine = searchPane.selectedEngine;
> > +  searchPane.selectedEngine = TEST_DATA.searchEngines[0];
> 
> You may want to check if searchEngines[0] is already set as default and use
> other engine if so. Also, you'll have to update the test everywhere else in
> this case.
> 

"Google" is still the default search engine for me on latest beta (OSX and Ubuntu), I'll update this code once this patch will be in a more production ready state.


> 
> ::: firefox/lib/toolbars.js
> @@ +7,5 @@
> >   * The ToolbarAPI adds support for accessing and interacting with toolbar elements
> >   *
> >   * @version 1.0.0
> >   */
> >  
> 
> Add "use strict";
> 
> @@ +14,2 @@
> >  // Include required modules
> >  var { assert } = require("../../lib/assertions");
> 
> We don't need this anymore
> 

This is not under the scope of this bug, I didn't modified this lines, and I think I shouldn't do it under this patch.

> 
> @@ +1301,5 @@
> > +   *
> > +   * @param {string} aSearchTerm
> > +   *        Text which should be checked for
> > +   */
> > +  checkSearchResultPage : function searchBar_checkSearchResultPage(aSearchTerm) {
> 
> I don't think this function should be in the library
> 

And where it should be then ? Adding this code to each of our tests will be really redundant, and it is a nice functional addition to the library.

> 
> @@ +1395,5 @@
> > +    var nodeCollector = new domUtils.nodeCollector(root);
> > +    var elems = [];
> > +
> > +    switch(aSpec.type) {
> > +      case "searchBar":
> 
> Please order the cases alphabetically
> 

Fixed.

> @@ +1492,5 @@
> > +   *         SearchBar text value
> > +   */
> > +  getSearchBarValue : function searchBar_getSearchBarValue() {
> > +    var elem = this.getElement({type: "searchBar"});
> > +    var value = elem.getNode().value;
> 
> Please use more intuitive var names (like "searchBar", "searchBarValue")
> 

... ok

> 
> ::: firefox/lib/ui/pref-window.js
> @@ +10,5 @@
> >   * @version 1.0.0
> >   */
> >  
> >  // Include required modules
> >  var { assert, expect } = require("../../../lib/assertions");
> 
> We don't need this anymore
> 

Again this is not under the scope of this bug, modifying this under this patch will result in a misleading repository log history.

> ::: lib/search.js
> 
> @@ +27,5 @@
> > +  });
> > +  promise.then(() => {

Fixed all nits related to this function.

> 
> @@ +145,5 @@
> > +  Services.search.getEngines().forEach(function (aEngine, aIndex) {
> > +    if (defaults.indexOf(aEngine) !== aIndex) {
> > +      Services.search.moveEngine(aEngine, defaults.indexOf(aEngine));
> > +    }
> > +  });
> 
> This restore of sorting doesn't make sense anymore since we no longer can
> change the engines' order
> 

Yes, you're right removed this lines.

> @@ +150,5 @@
> >  
> > +  // Update the visibility status for each engine and reset the default engine
> > +  Services.search.restoreDefaultEngines();
> > +  Services.search.currentEngine = Services.search.defaultEngine;
> > +}
> 
> The restoreDefaultEngines from Services.search is only updating the
> visibility of the engines, does not remove them.Maybe we should stick to
> that behavior and don't remove the non-default engines from the list, just
> mark them as hidden.
> 

I think this function is a usefull addition to the library, I don't think we should remove something which we have and is functional, this could be used for example cleaning up after a test.

I have addded a new function restoreDefaultEngines, which is a wrapper for Services.search.restoreDefaultEngines().

> ::: lib/tests/testSearch.js
> @@ +40,5 @@
> > +
> > +  // Check restore defaults
> > +  search.restoreDefaults();
> > +  assert.ok(!search.isEngineInstalled("IMDb"),
> > +            "Search engines have been restored to default");
> 
> If restore defaults only hides the engines from the list (doesn't also
> remove them), you should check for the visibility instead of installation
> here

No good way to do that now, there is no API call which we could use to test this.

Fixed all the reported nits, added blank lines/spaces, removed blank lines/spaces, added missing documentation, updated comments.

Thanks, Mihaela for the review.

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8534927 [details] [diff] [review]
betav2.2.patch

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

The patch doesn't apply anymore.

(In reply to Teodor Druta from comment #11)
> Created attachment 8534927 [details] [diff] [review]
> betav2.2.patch
> 
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #10)
> > Comment on attachment 8533148 [details] [diff] [review]
> > betav2.patch
> > 
> > Review of attachment 8533148 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch is v1 :)
> > 
> 
> The first version wasn't uploaded for review, as the 2.1 version.
> So the new patch is v2.2.
> 

That was the first version uploaded to this bug so it should have been v1.
The next patch you upload will be v2.

> > ::: firefox/lib/toolbars.js
> > @@ +7,5 @@
> > >   * The ToolbarAPI adds support for accessing and interacting with toolbar elements
> > >   *
> > >   * @version 1.0.0
> > >   */
> > >  
> > 
> > Add "use strict";
> > 
> > @@ +14,2 @@
> > >  // Include required modules
> > >  var { assert } = require("../../lib/assertions");
> > 
> > We don't need this anymore
> > 
> 
> This is not under the scope of this bug, I didn't modified this lines, and I
> think I shouldn't do it under this patch.

We need to have "use strict" in our libs and tests. Please log a new bug for those updates.

::: firefox/lib/tests/testSearchBar.js
@@ +65,5 @@
> +  // If this fails we will know that the getSuggestions() method needs improvement
> +  assert.waitFor(() => (suggestions.length === 10),
> +                 "There are 10 suggestions, got: " + suggestions.length);
> +  // Check that the searched string appears in the suggestions from the list
> +  assert.waitFor(() => (suggestions[0].search(TEST_DATA.searchTerm) !== -1),

>> Shouldn't we check this for all suggestions? 

> We don't know for sure what suggestions will return a certain search engine, this depends on a lot of factors, such as Geographical Location, Language settings, previous searches done from > the same public IP (search engines store that kind of data) etc. For all I know maybe we will get suggestions for "abc", which don't even contain "abc", so I think it's safer to just test the first suggestion.

In this case, I wonder if it's safe to check this at all.

::: firefox/lib/tests/testSearchPane.js
@@ +45,5 @@
> +  /**
> +   * Check if default search engine list corresponds with the one click search engine list
> +   */
> +  // The default selected engine from engine list should not
> +  // be in the one click search engines list

// The default selected engine from engine list should not be in the one click 
// search engines list

@@ +62,5 @@
> +  /**
> +   * Set a new default search engine
> +   */
> +  var initialSuggestionsState = searchPane.selectedEngine;
> +  searchPane.selectedEngine = TEST_DATA.searchEngines[0];

>> You may want to check if searchEngines[0] is already set as default and use
>> other engine if so. Also, you'll have to update the test everywhere else in
>> this case.

> "Google" is still the default search engine for me on latest beta (OSX and Ubuntu), I'll update this code once this patch will be in a more production ready state.

You should do that check so you don't miss it later. Besides, on our test machines the default engine on Nightly is Yahoo.

::: firefox/lib/toolbars.js
@@ +1301,5 @@
> +   *
> +   * @param {string} aSearchTerm
> +   *        Text which should be checked for
> +   */
> +  checkSearchResultPage : function searchBar_checkSearchResultPage(aSearchTerm) {

checkSearchResultURL would be more relevant name for what this function does.

::: firefox/lib/ui/pref-window.js
@@ +208,5 @@
>    }
>  };
>  
>  /**
> + * Constructor

* SearcPane class
*
* @constructor
* @param...

Comment 13

[Teodor Druta]

Attachment: nightly_v1.patch

I actually had to refactor a lot to make this work for the default using the new AboutPreferences lib. Let's finish this ASAP, because it blocks all the search related tests.

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8558427 [details] [diff] [review]
nightly_v1.patch

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

There are some failures in the lib tests with this patch: testPrefs, testAboutPreferences

::: firefox/lib/tests/manifest.ini
@@ +14,5 @@
>  [testMenuPanel.js]
>  [testPageInfoWindow.js]
>  [testPrefs.js]
> +[testSearchBar.js]
> +[testPaneSearch.js]

This test doesn't exist.

::: firefox/lib/tests/testSearchBar.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var toolbars = require("../toolbars");
> +var utils = require("../../../lib/utils");

You don't use these 2 libs.

@@ +31,5 @@
> +}
> +
> +/**
> + * Test search bar UI
> + *

Please remove this line

::: firefox/lib/toolbars.js
@@ +14,4 @@
>  // Include required modules
>  var { assert } = require("../../lib/assertions");
>  var domUtils = require("../../lib/dom-utils");
> +var prefs = require("../../lib/prefs");

This is not used, please remove

@@ +14,5 @@
>  // Include required modules
>  var { assert } = require("../../lib/assertions");
>  var domUtils = require("../../lib/dom-utils");
> +var prefs = require("../../lib/prefs");
> +// var search = require("../../lib/search");

Please remove this line

@@ +1209,5 @@
>    }
>  }
>  
>  /**
> + * Constructor

* SearchBar class
*
* @constructor
* @param ...

@@ +1575,5 @@
> +  /**
> +   * Opens the search settings pane
> +   *
> +   * @return {object} searchPane
> +   *         searchPane class instance

nit: about:preferences page instance

@@ +1633,5 @@
> +  /**
> +   * Selects an entry from the context menu
> +   *
> +   * @param {object} aSpec
> +   * @param {string} [aSpec.method = "menu"]

Please move this optional param after aSpec.name

@@ +1641,5 @@
> +   */
> +  selectContextMenuEntry : function searchBar_selectContextMenuEntry(aSpec={}) {
> +    var method = aSpec.method || "menu";
> +    var name = aSpec.name;
> +    if (method === "menu") {

Add a blank line before the if block

::: firefox/lib/ui/about-preferences-page.js
@@ +171,5 @@
> +* @param {string} aName
> +*        Name of the search provider to set the state to
> +* @return {boolean} aState
> +*         State of the specified search provider
> +*/

nit: Bad indentation for lines 169-175, they should have a blank space at the beginning.

@@ +202,5 @@
> +  var tree = providersList.getNode();
> +
> +  for (var ii = 0; ii < tree.view.rowCount; ii ++) {
> +    if (tree.view.getCellText(ii, tree.columns.getColumnAt(1)) === aName) {
> +      tree.view.setCellValue(ii, tree.columns.getColumnAt(0), aState);

You could use a break here after setting the value, so that it won't go throug the rest of the items

::: firefox/lib/ui/browser.js
@@ +336,5 @@
> +  }
> +  callback = aCallback || callback;
> +
> +  return prefs.open(callback);
> +}

nit: missing ; at the end of the line

::: firefox/lib/ui/pref-window.js
@@ +11,5 @@
>   */
>  
>  // Include required modules
>  var { assert, expect } = require("../../../lib/assertions");
> +var domUtils = require("../../../lib/dom-utils");

You don't use this lib

::: lib/search.js
@@ +15,5 @@
> + * Initializes the search service (API call)
> + * https://developer.mozilla.org/en-US/docs/Mozilla/Tech/
> + *                               XPCOM/Reference/Interface/nsIBrowserSearchService#init
> + *
> + * @return {boolean} initReady

I think you can remove the return value (see bellow)

@@ +30,5 @@
> +    initReady = true;
> +  });
> +
> +  assert.waitFor(() => initReady, "Search service has been initialized");
> +  return initReady;

You never use the value this function returns, so I guess you can remove this line

@@ +42,5 @@
> + *
> + * @return {object} engine
> + *         Engine object of the specified name
> + */
> +

nit: extra blank line

@@ +55,5 @@
> + *
> + * @return {array} visibleEngine
> + *         List of visible engines objects
> + */
> +

nit: extra blank space

@@ +127,5 @@
> + *        Name of the search engine to remove
> + */
> +function removeEngine(aName) {
> +  init();
> +  if (this.isEngineInstalled(aName)) {

No need for "this" here

::: lib/tests/testSearch.js
@@ +5,5 @@
> +"use strict";
> +
> +// Include required modules
> +var search = require("../search");
> +var toolbars = require("../../firefox/lib/toolbars");

This lib is not used

Comment 15

[Teodor Druta]

Attachment: nightly_v2.patch

Thanks for the review, Mihaela !
I still that we should finish this and unblock mozmill search tests.

Comment 16

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8559788 [details] [diff] [review]
nightly_v2.patch

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

Looks fine to me, only a couple of nits.

::: firefox/lib/toolbars.js
@@ +1205,5 @@
>    }
>  }
>  
>  /**
> + * Constructor

Please update the jsdoc as requested in the previous review:

* SearchBar class
*
* @constructor
* @param ...

::: lib/search.js
@@ +49,5 @@
> + *
> + * @return {array} visibleEngine
> + *         List of visible engines objects
> + */
> +

You have an extra blank line here

Comment 17

[Teodor Druta]

Attachment: nightly_v2.1.patch

Fixed all nits.

Comment 18

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8560452 [details] [diff] [review]
nightly_v2.1.patch

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

Thanks for the updates, Teodor!

Comment 19

[Teodor Druta]

I won't be working on this bug any more, the latest patch still applies and works correctly at the time of writing this comment. Maybe this patch will help by serving as a reference for porting the search related libs to marionette.

Comment 20

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

Mozmill tests have been superseded by Marionette tests.