Last Comment Bug 753705 - Increase the timeout in waiting for discovery pane to load
: Increase the timeout in waiting for discovery pane to load
Status: RESOLVED FIXED
[lib][qa-]
:
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-10 03:37 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2012-08-14 15:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed
unaffected


Attachments
initial patch (v1.0) (2.35 KB, patch)
2012-05-10 04:39 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
simplified test (899 bytes, application/javascript)
2012-05-10 04:40 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details
wip patch v1.0 (3.23 KB, patch)
2012-05-14 04:38 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: feedback-
Details | Diff | Splinter Review
patch v2.0 (2.69 KB, patch)
2012-05-16 05:29 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v3.0 (2.94 KB, patch)
2012-05-16 06:55 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v4.0 (3.13 KB, patch)
2012-05-16 07:37 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Splinter Review
patch v5.0 (3.13 KB, patch)
2012-05-18 00:05 PDT, Maniac Vlad Florin (:vladmaniac)
dave.hunt: review+
Details | Diff | Splinter Review

Description Maniac Vlad Florin (:vladmaniac) 2012-05-10 03:37:25 PDT
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) 2012-05-10 03:41:24 PDT
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) 2012-05-10 04:39:26 PDT
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
Comment 3 Maniac Vlad Florin (:vladmaniac) 2012-05-10 04:40:00 PDT
Created attachment 622686 [details]
simplified test

Attaching test
Comment 4 Henrik Skupin (:whimboo) 2012-05-10 05:27:43 PDT
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) 2012-05-10 06:24:34 PDT
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) 2012-05-10 06:27:15 PDT
It's not only for startup. It's all the time the 'Get Add-ons' tab gets opened.
Comment 7 Maniac Vlad Florin (:vladmaniac) 2012-05-10 06:28:22 PDT
(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) 2012-05-10 06:28:51 PDT
So do we agree with the steps in comment5 with your additions in comment6?
Comment 9 Henrik Skupin (:whimboo) 2012-05-10 07:39:45 PDT
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) 2012-05-14 04:38:36 PDT
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
Comment 11 Henrik Skupin (:whimboo) 2012-05-16 01:50:34 PDT
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) 2012-05-16 04:37:02 PDT
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) 2012-05-16 05:29:33 PDT
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
Comment 14 Henrik Skupin (:whimboo) 2012-05-16 06:12:57 PDT
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) 2012-05-16 06:55:47 PDT
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.
Comment 16 Henrik Skupin (:whimboo) 2012-05-16 07:23:54 PDT
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) 2012-05-16 07:37:25 PDT
Created attachment 624378 [details] [diff] [review]
patch v4.0

nits fixed
Comment 18 Henrik Skupin (:whimboo) 2012-05-16 08:57:59 PDT
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) 2012-05-17 01:53:39 PDT
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) 2012-05-17 12:48:22 PDT
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) 2012-05-18 00:05:23 PDT
Created attachment 625018 [details] [diff] [review]
patch v5.0

fixed
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-05-18 01:20:38 PDT
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) 2012-05-18 06:51:24 PDT
Comment on attachment 625018 [details] [diff] [review]
patch v5.0

Looks good. I'll land this on default.
Comment 24 Dave Hunt (:davehunt) 2012-05-18 06:58:06 PDT
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.

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