Increase the timeout in waiting for discovery pane to load

RESOLVED FIXED

Status

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

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Tracking

unspecified

Firefox Tracking Flags

(firefox12 fixed, firefox13 fixed, firefox14 fixed, firefox15 fixed, firefox-esr10 fixed, status1.9.2 unaffected)

Details

(Whiteboard: [lib][qa-])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

5 years ago
We need to increase the timeout in waiting for disc pane loading

This will apply for all the tests which depend on that, or specifically test the discovery pane in any way. 
Right now, we are talking about remote tests, located under /mozmill-tests/remote/ folder in our repo and addons manager tests located under /mozmill-tests/functional/restartTests

Of course we are going to change the timeout value in the API, so the change will apply to the addons.js module. 

At this initial moment I do not estimate to have any changes in the tests themselves.
(Assignee)

Updated

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

Updated

5 years ago
Whiteboard: [lib]
(Assignee)

Comment 1

5 years ago
More information about why we need this is located here 

https://bugzilla.mozilla.org/show_bug.cgi?id=751832#c20
(Assignee)

Comment 2

5 years ago
Created attachment 622685 [details] [diff] [review]
initial patch (v1.0)

Solution to have a particular case handled for opening disc pane on AOM startup with timeout increased to 30s using the already defined TIMEOUT_REMOTE constant
Attachment #622685 - Flags: review?(hskupin)
(Assignee)

Comment 3

5 years ago
Created attachment 622686 [details]
simplified test

Attaching test
Comment on attachment 622685 [details] [diff] [review]
initial patch (v1.0)

>       if (waitFor) {
>-        this.controller.waitFor(function () {
>-          return self.loaded;
>-        }, "Selected category has been loaded.", TIMEOUT, undefined, this);
>+        // Wait longer if loading the discovery pane on startup
>+        if (discoveryPaneURL === AMO_DISCOVER_URL_DEFAULT_VALUE) {
>+          this.controller.waitFor(function () {
>+            return self.loaded;
>+          }, "Discovery pane has been loaded.", TIMEOUT_REMOTE, undefined, this);
>+        } else {
>+          this.controller.waitFor(function () {
>+            return self.loaded;
>+          }, "Category has been loaded.", TIMEOUT, undefined, this);
>+        }

First you will probably be able to get the selected category even if the pane hasn't been loaded yet and use that one to check if 'Get Addons' is selected. Further there is no need to have an if/else. Just define a new timeout variable and assign the corresponding value to it.
Attachment #622685 - Flags: review?(hskupin) → review-
(Assignee)

Comment 5

5 years ago
As I see it we need to 

* check if AOM loads at startup with 'GetAddons' pane
      * if yes, check if it loads with the disc pane
           * if yes, increase timeout
           * if no, let timeout to 5s 
      * if no, let timeout to 5s 

This would be the thorough approach IMHO
It's not only for startup. It's all the time the 'Get Add-ons' tab gets opened.
(Assignee)

Comment 7

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #6)
> It's not only for startup. It's all the time the 'Get Add-ons' tab gets
> opened.

Right, my bad there, bad english
(Assignee)

Comment 8

5 years ago
So do we agree with the steps in comment5 with your additions in comment6?
Well you should better come up with an update because I don't think both comments make it clear enough for you. If it wouldn't have been ok for me, I had said something.
(Assignee)

Comment 10

5 years ago
Created attachment 623631 [details] [diff] [review]
wip patch v1.0

It seems that we cannot get the category before its loaded. 

Added a wip patch to demonstrate that, also the simplified test file is still available for testing the helper function refactor
Attachment #622685 - Attachment is obsolete: true
Attachment #623631 - Flags: feedback?(hskupin)
Comment on attachment 623631 [details] [diff] [review]
wip patch v1.0

>+    var timeout = spec.timeout || TIMEOUT;

There is no spec.timeout and we do not want to have this given that it is handled within this module gracefully. So please don't introduce a new property.

>       if (waitFor) {
>         this.controller.waitFor(function () {
>           return self.loaded;
>-        }, "Selected category has been loaded.", TIMEOUT, undefined, this);
>+        }, "Selected category has been loaded.", timeout, undefined, this);
> 
>         tab = this.waitForOpened();
>       }

What you want is to modify this code something like that:

        tab = this.waitForOpened();

        var categoryID = this.getCategoryId({category: this.selectedCategory})
        var timeout = (categoryID === "discover") ? TIMEOUT_REMOTE : TIMEOUT;

        this.controller.waitFor(function () {
          return self.loaded;
        }, "Selected category has been loaded.", timeout);


The reason why it wasn't working is that we were waiting for the add-ons manager tab to be open *after* the check if the category has been loaded. 

The same code also would have to be added/updated in the waitForCategory() method.
Attachment #623631 - Flags: feedback?(hskupin) → feedback-
(Assignee)

Comment 12

5 years ago
Gee, thanks Henrik!

> 
> The same code also would have to be added/updated in the waitForCategory()
> method.

From what I see the waitForCategory() method does this already 
Pasted a code snippet from the method, bellow 

// For the panes 'discover' and 'search' we have to increase the timeout
    // because the ViewChanged event is sent after the remote data has been loaded.
    if (categoryId === 'discover' || categoryId === 'search')
      timeout = TIMEOUT_REMOTE;
(Assignee)

Comment 13

5 years ago
Created attachment 624354 [details] [diff] [review]
patch v2.0

* Modified the "if" clause from waitForCategory() method 
* Added proposed changes 
* Tested patch with tests from /mozmill-tests/functional/restartTests folder
Attachment #623631 - Attachment is obsolete: true
Attachment #624354 - Flags: review?(hskupin)
Comment on attachment 624354 [details] [diff] [review]
patch v2.0

>-      if (waitFor) {
>-        this.controller.waitFor(function () {
>-          return self.loaded;
>-        }, "Selected category has been loaded.", TIMEOUT, undefined, this);
>+      tab = this.waitForOpened();

Why do you remove this if construct? It's necessary because we do not want to execute that code all the time, but only if we want to wait for the tab and pane loaded.

>   waitForCategory : function AddonsManager_waitForCategory(aSpec, aActionCallback) {
>     var spec = aSpec || { };
>     var category = spec.category;
>     var categoryId = this.getCategoryId({category: category});

Mind combining the last two lines into a single one?

>-    var timeout = spec.timeout || TIMEOUT;

You will also have to update the JSDoc comment.

>+    var timeout = (categoryId === "discover" || categoryId === "search") ? TIMEOUT_REMOTE : TIMEOUT;

Please wrap that line by moving the colon down under the question mark.
Attachment #624354 - Flags: review?(hskupin) → review-
(Assignee)

Comment 15

5 years ago
Created attachment 624370 [details] [diff] [review]
patch v3.0

Fixed. 

One thing: 
If you really want those lines merged we should do it in another bug. 
That was not intended for this patch and will affect all tests if we do that, so I would not recommend it here.
Attachment #624354 - Attachment is obsolete: true
Attachment #624370 - Flags: review?(hskupin)
Comment on attachment 624370 [details] [diff] [review]
patch v3.0

>+        }, "Selected category has been loaded.", timeout);
>+      }
> 
>-        tab = this.waitForOpened();
>-      }

nit: please remove the empty line introduced here.

>-   *                  timeout - Duration to wait for the target state
>-   *                            [optional - default: 5s]
>+   *        
>    * @param {Function} aActionCallback

Same here.

>     var category = spec.category;
>     var categoryId = this.getCategoryId({category: category});
>-    var timeout = spec.timeout || TIMEOUT;
> 
>     if (!category)
>       throw new Error(arguments.callee.name + ": Category not specified.");

Wait. My fault. You should move the line with the categoryId declaration down below this check. So we can leave them separated and at the same time fix another bug.

Otherwise it's fine.
Attachment #624370 - Flags: review?(hskupin) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 624378 [details] [diff] [review]
patch v4.0

nits fixed
Attachment #624370 - Attachment is obsolete: true
Attachment #624378 - Flags: review?(hskupin)
Comment on attachment 624378 [details] [diff] [review]
patch v4.0

>+    var categoryId = this.getCategoryId({category: category});
>+
>     // For the panes 'discover' and 'search' we have to increase the timeout
>     // because the ViewChanged event is sent after the remote data has been loaded.
>-    if (categoryId === 'discover' || categoryId === 'search')
>-      timeout = TIMEOUT_REMOTE;
>+    var timeout = (categoryId === "discover" || categoryId === "search") ? TIMEOUT_REMOTE 
>+                                                                         : TIMEOUT;

Well, below the comment please so that it's near the timeout declaration. Otherwise it's fine and we could land it.
Attachment #624378 - Flags: review?(hskupin) → review-
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
(Assignee)

Comment 19

5 years ago
Henrik, can you please clarify comment 18? I'm missing the point in "please so that it's near the timeout declaration". Then I can upload the fix immediately and you can land. Thanks :)
status1.9.2: unaffected → ---
status-firefox-esr10: affected → ---
status-firefox12: affected → ---
status-firefox13: affected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
(Assignee)

Updated

5 years ago
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
Please declare categoryId right above timeout so it is below the comment. Always think about to combine blocks and that those are not intersected by comments especially long ones as this one.
(Assignee)

Comment 21

5 years ago
Created attachment 625018 [details] [diff] [review]
patch v5.0

fixed
Attachment #624378 - Attachment is obsolete: true
Attachment #625018 - Flags: review?(hskupin)
(Assignee)

Comment 22

5 years ago
Comment on attachment 625018 [details] [diff] [review]
patch v5.0

Asking for r from Dave. 

Dave - please check previous review (from Henrik) to see if the changes are the ones we want
Attachment #625018 - Flags: review?(hskupin) → review?(dave.hunt)
Comment on attachment 625018 [details] [diff] [review]
patch v5.0

Looks good. I'll land this on default.
Attachment #625018 - Flags: review?(dave.hunt) → review+
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/1c5f598a157d

Let's see this passing before transplanting to other branches and then working on re-enabling the remote tests.
Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/ed0971ff5f30 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/90d228db3fcd (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/8dff52a3cc21 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/dfc787a5c212 (mozilla-esr10)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
status-firefox15: affected → fixed
Whiteboard: [lib] → [lib][qa-]
You need to log in before you can comment on or make changes to this bug.