Last Comment Bug 783197 - Ensure restoreDefaultEngines() completely resets the installed engines
: Ensure restoreDefaultEngines() completely resets the installed engines
Status: RESOLVED FIXED
[lib] s=121015 u=enhance c=search p=1
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: ---
Assigned To: Andreea Matei [:AndreeaMatei]
:
:
Mentors:
http://mozmill-ci.blargon7.com/#/func...
Depends on:
Blocks: 780892
  Show dependency treegraph
 
Reported: 2012-08-16 01:44 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2012-11-12 04:15 PST (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed
fixed


Attachments
patch v1 (1.72 KB, patch)
2012-11-05 02:36 PST, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v2 (1.85 KB, patch)
2012-11-06 07:29 PST, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Splinter Review
patch v2.1 (1.73 KB, patch)
2012-11-07 05:30 PST, Andreea Matei [:AndreeaMatei]
dave.hunt: review-
Details | Diff | Splinter Review
patch v3 (1.57 KB, patch)
2012-11-08 05:16 PST, Andreea Matei [:AndreeaMatei]
hskupin: review-
Details | Diff | Splinter Review
patch v3.1 (1.57 KB, patch)
2012-11-09 01:35 PST, Andreea Matei [:AndreeaMatei]
hskupin: review+
hskupin: checkin+
Details | Diff | Splinter Review

Description Maniac Vlad Florin (:vladmaniac) 2012-08-16 01:44:10 PDT
Mozmill: 1.5.17
Platform: All
Error: "An engine with that name already exists!"
Failing method: setupModule: testSearchViaShortcut.js::setupModule
Report crowd: http://mozmill-crowd.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee644d4dc
Report ci: http://mozmill-ci.blargon7.com/#/functional/report/d87d47fd1034f072b9bece6ee6402324

This would have to be fixed in restoreDefaultEngines, as https://bugzilla.mozilla.org/show_bug.cgi?id=780892#c8 clarifies.
Comment 1 Henrik Skupin (:whimboo) 2012-08-16 04:18:36 PDT
The affecting patch has not landed yet, so please do not file test failures against non-existing failures. You could reference the local failure but the summary should clearly state what we have to do. And that's the restoreDefaultEngines() method.
Comment 2 Henrik Skupin (:whimboo) 2012-09-11 07:57:41 PDT
I drop this from this weeks sprint. Instead we have to fix bug 790218.
Comment 3 Maniac Vlad Florin (:vladmaniac) 2012-10-17 04:41:45 PDT
We hereby wrongly assumed that restoreDefaultEngines will also delete the newly added engines, but it does not do that by design. It only restores the order in the engine list.

You can easily check this manually.
Therefore, its no job for this method to delete the additionally installed engines.
This bug is invalid and if we want to fix bug 780892 we should simply make usage of removeEngine(aName) method from our shared modules and then call restoreDefaultEngines and this will work (tested it).
Comment 4 Henrik Skupin (:whimboo) 2012-10-17 05:42:36 PDT
You are making wrong assumptions. This bug is about the addition of code which removes those installed engines by this helper method in search.js.
Comment 5 Andreea Matei [:AndreeaMatei] 2012-11-05 02:36:14 PST
Created attachment 678254 [details] [diff] [review]
patch v1

We now take on defaults the list of default engines and then check in the whole list of engines, those that are not default to remove them.
Comment 6 Henrik Skupin (:whimboo) 2012-11-06 04:49:38 PST
Comment on attachment 678254 [details] [diff] [review]
patch v1

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

::: lib/search.js
@@ +796,5 @@
>    {
> +    // Get the default engines list
> +    var defaults = this._bss.getDefaultEngines({ }).map(function (e) e.name);
> +
> +    this.openEngineManager(function (controller) {

I do not want to have ui related actions in this method. Those can fail and we will not be able to restore to the default engines.

What we should do is to use the backend API to retrieve all installed engines and remove those which are not default ones.
Comment 7 Andreea Matei [:AndreeaMatei] 2012-11-06 07:29:00 PST
Created attachment 678747 [details] [diff] [review]
patch v2

Updated so we won't use UI.
Comment 8 Dave Hunt (:davehunt) 2012-11-06 07:43:31 PST
Comment on attachment 678747 [details] [diff] [review]
patch v2

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

Does this leave the engines in the default sort order too? From what I understand we will either need to move the engines into the default order, or click the restore defaults button as we were doing before.

::: lib/search.js
@@ +793,5 @@
>     * Restore the default set of search engines (API call)
>     */
> +  restoreDefaultEngines : function searchBar_restoreDefaults() {
> +    // Get the default and installed engines list
> +    var defaults = this._bss.getDefaultEngines({ }).map(function (e) e.name),

Why are we initiating all of these variables in one go? It at least makes it hard to read.

@@ +801,3 @@
>  
> +    // Remove installed engines that are not default
> +    for (i = installedEnginesMaximumIndex; i >= 0; i--) {

Can we use installedEngines.forEach here?
Comment 9 Andreea Matei [:AndreeaMatei] 2012-11-07 05:30:00 PST
Created attachment 679122 [details] [diff] [review]
patch v2.1

Updated the variables and to use waitFor.

The defaults order is correct.
Comment 10 Andreea Matei [:AndreeaMatei] 2012-11-07 05:30:32 PST
Sorry, I meant to use forEach.
Comment 11 Dave Hunt (:davehunt) 2012-11-07 06:45:18 PST
Comment on attachment 679122 [details] [diff] [review]
patch v2.1

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

I can confirm that this does not restore the default order. You can also verify using tests/functional/testSearch/testReorderSearchEngines.js
Comment 12 Andreea Matei [:AndreeaMatei] 2012-11-08 05:16:22 PST
Created attachment 679632 [details] [diff] [review]
patch v3

Reorder through the button as before, the other option being to move the engines as Dave said, but that's also using the method from engine manager.
Comment 13 Henrik Skupin (:whimboo) 2012-11-09 01:17:51 PST
Comment on attachment 679632 [details] [diff] [review]
patch v3

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

Much better! I like that. Just a couple of nits left.

::: lib/search.js
@@ +794,5 @@
>     */
> +  restoreDefaultEngines : function searchBar_restoreDefaults() {
> +    // Get the default and installed engines list
> +    var defaults = this._bss.getDefaultEngines({ }).map(function (e) e.name);
> +    var installedEngines = this._bss.getEngines({ }).map(function (e) e.name);

Can you replace 'e' with 'engine'?

@@ +795,5 @@
> +  restoreDefaultEngines : function searchBar_restoreDefaults() {
> +    // Get the default and installed engines list
> +    var defaults = this._bss.getDefaultEngines({ }).map(function (e) e.name);
> +    var installedEngines = this._bss.getEngines({ }).map(function (e) e.name);
> +    var searchBar = this;

Instead of searchBar please use 'self'.

@@ +799,5 @@
> +    var searchBar = this;
> +
> +    // Remove installed engines that are not default
> +    installedEngines.forEach(function (engine) {
> +      if (defaults.toString().indexOf(engine) === -1)

Don't use toString() here. indexOf is also available for arrays.
Comment 14 Andreea Matei [:AndreeaMatei] 2012-11-09 01:35:51 PST
Created attachment 680026 [details] [diff] [review]
patch v3.1

Thanks Henrik! 
Here are the requested changes.
Comment 15 Henrik Skupin (:whimboo) 2012-11-09 02:28:38 PST
Comment on attachment 680026 [details] [diff] [review]
patch v3.1

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

http://hg.mozilla.org/qa/mozmill-tests/rev/0aa3cdeb140e (default)
Comment 16 Andreea Matei [:AndreeaMatei] 2012-11-09 05:55:38 PST
This applies cleanly to all other branches.

Note You need to log in before you can comment on or make changes to this bug.