Refactor search tests for new Search Bar UI functionality

RESOLVED INVALID

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED INVALID
3 years ago
10 months ago

People

(Reporter: AndreeaMatei, Unassigned)

Tracking

(Blocks: 1 bug, {regression, reproducible})

Version 2
regression, reproducible
Dependency tree / graph

Firefox Tracking Flags

(firefox34- disabled, firefox35 disabled, firefox36 disabled, firefox37 disabled)

Details

(Whiteboard: [mozmill-test-failure][mozmill-test-skipped], URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
status-firefox34: --- → affected
Keywords: regression, regressionwindow-wanted
Whiteboard: [mozmill-test-failure]

Comment 1

3 years ago
Created attachment 8526655 [details] [diff] [review]
skip.patch

Skip patch for Beta
Attachment #8526655 - Flags: review?(andreea.matei)
(Reporter)

Comment 2

3 years ago
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.
Attachment #8526655 - Flags: review?(andreea.matei) → review+
(Reporter)

Updated

3 years ago
status-firefox34: affected → disabled
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]

Comment 3

3 years ago
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.

Updated

3 years ago
Blocks: 1088660

Updated

3 years ago
Keywords: regressionwindow-wanted → reproducible

Comment 4

3 years ago
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.
(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.
Flags: needinfo?(lhenry)
Hardware: x86 → All
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.
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
Flags: needinfo?(lhenry)
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.
[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?
Blocks: 1101642
No longer blocks: 1088660
status-firefox34: disabled → ---
tracking-firefox34: --- → -
Flags: needinfo?(hskupin)
Blocks: 1088660
No longer blocks: 1101642
It's not me but Andreea who manages the test creation process. Moving ni to her.
Flags: needinfo?(hskupin) → needinfo?(andreea.matei)

Updated

3 years ago
Assignee: andrei.eftimie → teodor.druta
Priority: -- → P2
Summary: Test failure "Search engines drop down open state has been changed. Expected: true" in search tests → Refactor search tests for new Search Bar UI functionality

Comment 11

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

Comment 12

3 years ago
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.

Updated

3 years ago
Blocks: 941477

Comment 13

3 years ago
(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

3 years ago
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()
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

3 years ago
(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

3 years ago
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

3 years ago
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?
Flags: needinfo?(petruta.rasa)
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

3 years ago
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.
Flags: needinfo?(hskupin)

Comment 21

3 years ago
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.
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

3 years ago
(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.
(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!
Flags: needinfo?(hskupin)

Comment 25

3 years ago
(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.

Updated

3 years ago
Flags: needinfo?(hskupin)
Sounds good to me. Lets do it then!
Flags: needinfo?(hskupin)

Updated

3 years ago
Depends on: 1105780

Comment 27

3 years ago
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.
No longer depends on: 1105780

Updated

3 years ago
Depends on: 1105780
(Reporter)

Comment 28

3 years ago
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.
status-firefox34: --- → disabled
status-firefox35: --- → disabled
status-firefox36: --- → disabled
status-firefox37: --- → disabled
(Reporter)

Comment 29

3 years ago
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.
Flags: needinfo?(andreea.matei)
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.
Flags: needinfo?(petruta.rasa)

Comment 31

3 years ago
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.
Assignee: teodor.druta → nobody
Status: ASSIGNED → NEW
Mozmill tests have been superseded by Marionette tests.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.