Ensure restoreDefaultEngines() completely resets the installed engines

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vladmaniac, Assigned: AndreeaMatei)

Tracking

unspecified

Firefox Tracking Flags

(firefox16 fixed, firefox17 fixed, firefox18 fixed, firefox19 fixed, firefox-esr10 fixed, firefox-esr17 fixed)

Details

(Whiteboard: [lib] s=121015 u=enhance c=search p=1, URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Whiteboard: [mozmill-test-failure][functional]
(Reporter)

Updated

5 years ago
Blocks: 780892
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
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.
Summary: Mozmill test failure /testSearch/testSearchViaShortcut.js | "An engine with that name already exists! " → Ensure restoreDefaultEngines() completely resets the installed engines
Whiteboard: [mozmill-test-failure][functional] → [lib]
Priority: -- → P2
Whiteboard: [lib] → [lib] s=2012-8-27 u=failure c=search p=1
I drop this from this weeks sprint. Instead we have to fix bug 790218.
Whiteboard: [lib] s=2012-8-27 u=failure c=search p=1 → [lib]
Whiteboard: [lib] → [lib] s=q3 u=enhance c=search p=1
Whiteboard: [lib] s=q3 u=enhance c=search p=1 → [lib] s=121015 u=enhance c=search p=1
(Reporter)

Updated

5 years ago
Blocks: 781547
(Reporter)

Comment 3

5 years ago
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).
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Assignee: vlad.mozbugs → andreea.matei
(Assignee)

Comment 5

5 years ago
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.
Attachment #678254 - Flags: review?(hskupin)
Attachment #678254 - Flags: review?(dave.hunt)
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.
Attachment #678254 - Flags: review?(hskupin)
Attachment #678254 - Flags: review?(dave.hunt)
Attachment #678254 - Flags: review-
(Assignee)

Comment 7

5 years ago
Created attachment 678747 [details] [diff] [review]
patch v2

Updated so we won't use UI.
Attachment #678254 - Attachment is obsolete: true
Attachment #678747 - Flags: review?(hskupin)
Attachment #678747 - Flags: review?(dave.hunt)
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?
Attachment #678747 - Flags: review?(hskupin)
Attachment #678747 - Flags: review?(dave.hunt)
Attachment #678747 - Flags: review-
(Assignee)

Comment 9

5 years ago
Created attachment 679122 [details] [diff] [review]
patch v2.1

Updated the variables and to use waitFor.

The defaults order is correct.
Attachment #678747 - Attachment is obsolete: true
Attachment #679122 - Flags: review?(hskupin)
Attachment #679122 - Flags: review?(dave.hunt)
(Assignee)

Comment 10

5 years ago
Sorry, I meant to use forEach.
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
Attachment #679122 - Flags: review?(hskupin)
Attachment #679122 - Flags: review?(dave.hunt)
Attachment #679122 - Flags: review-
(Assignee)

Comment 12

5 years ago
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.
Attachment #679632 - Flags: review?(hskupin)
Attachment #679632 - Flags: review?(dave.hunt)
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.
Attachment #679632 - Flags: review?(hskupin)
Attachment #679632 - Flags: review?(dave.hunt)
Attachment #679632 - Flags: review-
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox-esr17: --- → affected
(Assignee)

Comment 14

5 years ago
Created attachment 680026 [details] [diff] [review]
patch v3.1

Thanks Henrik! 
Here are the requested changes.
Attachment #679122 - Attachment is obsolete: true
Attachment #679632 - Attachment is obsolete: true
Attachment #680026 - Flags: review?(hskupin)
Attachment #680026 - Flags: review?(dave.hunt)
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)
Attachment #680026 - Flags: review?(hskupin)
Attachment #680026 - Flags: review?(dave.hunt)
Attachment #680026 - Flags: review+
Attachment #680026 - Flags: checkin+
status-firefox19: affected → fixed
(Assignee)

Comment 16

5 years ago
This applies cleanly to all other branches.
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: affected → fixed
status-firefox16: affected → fixed
status-firefox17: affected → fixed
status-firefox18: affected → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
No longer blocks: 781547
You need to log in before you can comment on or make changes to this bug.