1102821 - Refactor search tests for new Search Bar UI functionality

Status: RESOLVED INVALID

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

Description

[Andreea Matei [:AndreeaMatei]]

Fails on beta only, starting today, maybe something changed related to search. All platforms affected. 
Affected line:
http://hg.mozilla.org/qa/mozmill-tests/file/mozilla-beta/firefox/lib/search.js#l493

Andrei will investigate this.

Comment 1

[Andrei Eftimie]

Attachment: skip.patch

Skip patch for Beta

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8526655 [details] [diff] [review]
skip.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/ce860ebeb9ca (beta)

We'll have to check the other branches too once we get today's builds.

Comment 3

[Andrei Eftimie]

Beta pushlog:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=c07f2b49fcaf&tochange=2faac5336c1e

I can only reproduce the failure on beta 34.

There's bug 1088660 which only landed on Beta!

I'll check what we need to update on our tests to support the changes.

Comment 4

[Andrei Eftimie]

The changes are significant. We will leave these tests disabled for now, as we'll need new testcases for the different functionality and changed UI.

There's no "fix" here. We'll need to investigate what is still feasible from the existing tests, and rewrite them for the new SearchBar implementation.

Comment 5

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

(In reply to Andrei Eftimie from comment #3)
> Beta pushlog:
> https://hg.mozilla.org/releases/mozilla-beta/
> pushloghtml?fromchange=c07f2b49fcaf&tochange=2faac5336c1e
> 
> I can only reproduce the failure on beta 34.
> 
> There's bug 1088660 which only landed on Beta!

This is very bad! First that it didn't land on the other branches first, and second that it happened that late in beta! Please do not forget to CC the appropriate QA person in charge for the release, which will be Liz. She is even not CC'ed on the search engine bug!

So lets get feedback from here, what is planned there.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

Liz and the rest of QE are aware of the search changes that are landing on Beta. These changes are specifically targeted at Beta at this point in support of the upcoming change to the default in en_US builds on Dec 1. I agree that it is less than great that we need to land significant changes like this late in Beta. I can tell you that these changes are not being made lightly and have been made after significant discussion with gavin, madhava, chadw, johnath, eric, kev, mconnor and others.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi there whimboo!  I am aware of these bugs (as of yesterday) but didn't cc myself to them all. I can fill you in over email or on IRC. 

Here is the query for them: https://bugzilla.mozilla.org/buglist.cgi?o5=substring&v5=1101642&f5=blocked&list_id=11607427

Comment 8

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

Thank you both for the information. We were totally not aware of those, and the massive rate of failures for the upcoming beta across all locales were simply overwhelming us.

Who-ever will investigate this test failure early next week, please keep the list of regressions as marked as blockers for bug 1088660 in mind. There could still be a valid problem in Firefox.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Switching this over to block fx34 as I don't think 108860 is the right bug for this. Do you all want to make an automated tests meta bug for anything else that turns up in automation related to the search UI changes?

Comment 10

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

It's not me but Andreea who manages the test creation process. Moving ni to her.

Comment 11

[Teodor Druta]

We should use this bug for all the investigations related to the new Search Bar UI and mozmill tests.

Comment 12

[Teodor Druta]

For the new Search UI there is no engine manager dialog, all the engine manager functionality is located under search tab in preferences dialog. I think we should move all the relevant methods from engineManager to the prefs library.

Comment 13

[Teodor Druta]

(In reply to Teodor Druta from comment #12)
> For the new Search UI there is no engine manager dialog, all the engine
> manager functionality is located under search tab in preferences dialog. I
> think we should move all the relevant methods from engineManager to the
> prefs library.

The following methods in our engineManager class no longer apply:
- removeEngine 
- restoreDefaults
- moveDownEngine
- moveUpEngine
- editKeyword
- close

The following methods should remain in the current engineManager class in search lib:
- installFromURL
- _handleEngineInstall

And this methods could be moved in prefs lib with some changes:
- get engines - requires refactoring
- get selectedEngine - major refactoring
- set selectedEngine - major refactoring
- get selectedIndex - requires refactoring
- set selectedIndex - requires refactoring
- get suggestionsEnabled - requires refactoring
- set suggestionsEnabled - requires refactoring
- getElements - requires refactoring
- getMoreSearchEngines - requires refactoring

Comment 14

[Teodor Druta]

Also we will need some new methods to the newly engineManager "placeholder"/class:

> /**
> * Enables an one-click search engine
> *
> * @param {string} aName
> *        Search engine name
> */
> enableOneClickSearchEngine(aName)

> /**
> * Disables an one-click search engine
> *
> * @param {string} aName
> *        Search engine name
> */
> disableOneClickSearchEngine(aName)

> /**
> * Get the enabled search engines name
> *
> * @return {array} Enabled search engines name
> */
> getEnabledOneClickSearchEngines()

> /**
> * Get the disabled search engines name
> *
> * @return {array} Disabled search engines name
> */
> getDisabledOneClickSearchEngines()

Comment 15

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

If all search engine handling is located in the add-on manager now, there will be no engine manager class anymore. All has to be done through the add-on manager class.

Comment 16

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> If all search engine handling is located in the add-on manager now, there
> will be no engine manager class anymore. All has to be done through the
> add-on manager class.

Actually all the search engine handling is located in the preferences dialog.

Comment 17

[Teodor Druta]

For the searchBar class:

The following methods no longer apply:
 - get installableEngines
 - set selectedEngine
 - get visibleEngines
 - openEngineManager

All the other methods can be refactored, this should also include the API calls methods, I couldn't find a bug which stated any change in those.

Also we will need more methods to cover all the searchbar ui changes:

> /**
> * Get all enabled one-click search engines
> *
> * return {array} Names of the enabled one-click search engines
> */
> function getEnabledOneClickSearchEngines()

> /**
> * Get all disabled one-click search engines
> *
> * return {array} Names of the disabled one-click search engines
> */
> function getDisabledOneClickSearchEngines()


> /**
> * Get all available one-click search engines (enabled + disabled)
> *
> * return {array} Names of the available one-click search engines
> */
> function getAvailableOneClickSearchEngines()
>

Comment 18

[Andrei Eftimie]

I want to get some info from QA in regards of testcases for this feature.

Petruța, do you have some testcases available at this time for the new Search Bar implementation?

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, you might find some automatable testcases here: https://etherpad.mozilla.org/34.  

I collected some other information and links here: https://wiki.mozilla.org/Releases/Firefox_34/Test_Plan#New_Search_UI

Comment 20

[Teodor Druta]

For our mozmill-tests search lib I propose the next setup:
* Moving the engineManager class from search lib to prefs lib and refactor everything (that applies) and rename the class to an appropiate name, eg. "enginePane".
* Leaving the SearchBar class in the search lib and refactor its methods
* Leaving the _handleEngineInstall and installFromUrl methods in the search lib and rename the engineManager class to a name that applies, eg. "engineInstall" or we could move these methods to the addons lib.

(In reply to Liz Henry :lizzard from comment #19)
> Andrei, you might find some automatable testcases here:
> https://etherpad.mozilla.org/34.  
> 

That link is not working for me.

Comment 21

[Teodor Druta]

Regarding our functional search tests.
The search tests which can not be tested with the new search UI:
* testRemoveSearchEngine
* testReorderSearchEngine
* testRestoreDefaults
* testSearchSelection

All the other tests should be implementable with the refactored search lib.

Comment 22

[Cosmin Malutan, [:cosmin-malutan]]

I think you should:
 - leave all back-end methods in search.js and export them directly like we do for utils.js.
 - Move and refactor the SearchBar class to tollbars.js
 - For the EngineManager functionality, it has been moved to the preferences dialog, perhaps it will need a little enhancement to handle the functionality of EngineManeger class for the search pane.
 - In SearchBar class leave only the UI related methods and attributes and add an method for opening the Preferences with the search pane opened.

Comment 23

[Teodor Druta]

(In reply to Cosmin Malutan from comment #22)
> I think you should:
>  - leave all back-end methods in search.js and export them directly like we
> do for utils.js.
>  - Move and refactor the SearchBar class to tollbars.js
>  - For the EngineManager functionality, it has been moved to the preferences
> dialog, perhaps it will need a little enhancement to handle the
> functionality of EngineManeger class for the search pane.
>  - In SearchBar class leave only the UI related methods and attributes and
> add an method for opening the Preferences with the search pane opened.

After we discussed with the team, we all agreed with the following proposal from Cosmin's comment.

To wrap things up:

 - 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 24

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

(In reply to Teodor Druta from comment #23)

That all sounds fine to me. It will be indeed a bit of work necessary here.

>  - engineManager class will move from search.js and become a refactored
> searchPane class in prefs.js

Would such an extra class be necessary? We do not have such thing for any other pane yet.

>  - 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.

I assume, you mean global and not static given that there will be no object/class defined? With the next step it would be good to see which methods would fall into that category.

>  - 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.

Looks to be the right approach.

Thanks!

Comment 25

[Teodor Druta]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> (In reply to Teodor Druta from comment #23)
> 
> That all sounds fine to me. It will be indeed a bit of work necessary here.
> 
> >  - engineManager class will move from search.js and become a refactored
> > searchPane class in prefs.js
> 
> Would such an extra class be necessary? We do not have such thing for any
> other pane yet.
> 

As the UI for the panes in preferences dialog is very different among them with very few to none common things we should separate the methods of each pane in "sub"classes. 
I think preferencesDialog class should be similar to the NavBar class which we have in the toolbars.js now. 
We just didn't had any real UI implementation till now for the preferences dialog, I think we could start by adding the searchPane for now, and in the future add the functionality for the rest of the panes (general, content etc) if we will need them for our tests.
Objects from the new searchPane class inside prefs.js will instantiate similar to how they are instantiated now from the downloadsPanel class inside toolbars.js. 
I was thinking of something like this:

In the search.js lib:
> // constructor
> function preferencesDialog() {
>   ...
>   this._searchPane = null;
>   ...
> }
> 
> // Inside preferencesDialog class
> function searchPane() {
>   return new SearchPane(...);
> }

In our tests:
> prefs.openPreferencesDialog(.., function(aController) {
>   var prefDialog = new prefs.preferencesDialog(aController);
>   prefDialog.selectPane = "searchPane";
>   var searchPane = prefDialog.searchPane;
> }

> >  - 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.
> 
> I assume, you mean global and not static given that there will be no
> object/class defined? With the next step it would be good to see which
> methods would fall into that category.
> 

I couldn't find any reference anywhere on mozilla that the old search API methods have changed or have been removed, I think all the methods we have now in search.js should be all right, but I'll investigate this issue more, maybe we will also need to add a few new API methods to the search.js which will be implemented / are implemented with the new search ui.

Comment 26

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

Sounds good to me. Lets do it then!

Comment 27

[Teodor Druta]

I've filed a new bug 1105780 for the search lib refactoring. This bug will be used for refactoring the search tests after we will finish refactoring the search lib.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Skipped for the other branches too:
http://hg.mozilla.org/qa/mozmill-tests/rev/4a99e6c14f28 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/c86402890ad4 (aurora)

Not sure if we'll have this in esr31.

Comment 29

[Andreea Matei [:AndreeaMatei]]

Liz, sorry it took longer to answer. So the etherpad you added is not working for us, but we got it somehow from Petruta: https://etherpad.mozilla.org/ep/pad/view/34/latest. Hopefully it'll get added in moztrap too.
This link is fine: https://wiki.mozilla.org/Releases/Firefox_34/Test_Plan#New_Search_UI

I think we can add here any blocking bugs we'll be aware of next, as this bug involves current search tests that need refactoring and are affected by those changes.
As of new tests, we are going to file new bugs for each of them, when we'll have them prioritized with Florin.

Comment 30

[Petruta Horea [:phorea], Desktop Test Engineering]

I started to add manual testcases in the Moztrap suite: https://moztrap.mozilla.org/manage/cases/?filter-suite=808 .
Some more tests will be added during this cycle.

Comment 31

[Teodor Druta]

I will not be able to finish the work on this bug, so I'm unassigning myself, I think these tests will be handled with marionette.

Comment 32

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

Mozmill tests have been superseded by Marionette tests.