753705 - Increase the timeout in waiting for discovery pane to load

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

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.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

More information about why we need this is located here 

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

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Attachment: simplified test

Attaching test

Comment 4

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

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.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 6

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

It's not only for startup. It's all the time the 'Get Add-ons' tab gets opened.

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

(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

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

So do we agree with the steps in comment5 with your additions in comment6?

Comment 9

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

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.

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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

Comment 11

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

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.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

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;

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

* Modified the "if" clause from waitForCategory() method 
* Added proposed changes 
* Tested patch with tests from /mozmill-tests/functional/restartTests folder

Comment 14

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

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.

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: 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.

Comment 16

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

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.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.0

nits fixed

Comment 18

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

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.

Comment 19

[Maniac Vlad Florin (:vladmaniac)]

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 :)

Comment 20

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

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.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.0

fixed

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

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

Comment 23

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

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

Looks good. I'll land this on default.

Comment 24

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

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.

Comment 25

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

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)