783197 - Ensure restoreDefaultEngines() completely resets the installed engines

Status: RESOLVED FIXED

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

Description

[Maniac Vlad Florin (:vladmaniac)]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

I drop this from this weeks sprint. Instead we have to fix bug 790218.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

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][⌚️UTC+1]]

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

Attachment: 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][⌚️UTC+1]]

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

Attachment: patch v2

Updated so we won't use UI.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Attachment: patch v2.1

Updated the variables and to use waitFor.

The defaults order is correct.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Sorry, I meant to use forEach.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Attachment: 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][⌚️UTC+1]]

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

Attachment: patch v3.1

Thanks Henrik! 
Here are the requested changes.

Comment 15

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

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

This applies cleanly to all other branches.

Comment 17

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

http://hg.mozilla.org/qa/mozmill-tests/rev/2b3ec796902a (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/f1d7fb826dbc (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/2c40b7499603 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/a951812cc96a (esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/6d46bd63b02a (esr10)