Last Comment Bug 751832 - Addons tests are failing with "Selected category has been loaded"
: Addons tests are failing with "Selected category has been loaded"
Status: RESOLVED FIXED
[mozmill-test-failure][qa-]
: regression
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-04 01:59 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2012-08-14 15:00 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed
fixed
fixed
fixed
unaffected


Attachments
simplified testcase to demonstrate how we can ignore disc pane loading (699 bytes, application/javascript)
2012-05-04 04:33 PDT, Maniac Vlad Florin (:vladmaniac)
no flags Details
wip patch v1.0 (5.85 KB, patch)
2012-05-08 23:34 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
hskupin: feedback+
Details | Diff | Review
patch v1.1 (5.38 KB, patch)
2012-05-09 06:21 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review-
Details | Diff | Review
patch v1.2 (5.72 KB, patch)
2012-05-10 01:13 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Review
patch v1.3 (10.23 KB, patch)
2012-05-10 02:26 PDT, Remus Pop (:RemusPop)
hskupin: review+
Details | Diff | Review
[mozilla-esr10] patch v1.0 (13.23 KB, patch)
2012-05-10 05:56 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Review

Description Maniac Vlad Florin (:vladmaniac) 2012-05-04 01:59:33 PDT
Firefox versions: all
OS: all 

Error:
TimeoutError@resource://mozmill/modules/utils.js:429 waitFor@resource://mozmill/modules/utils.js:467 @resource://mozmill/modules/controller.js:648 AddonsManager_open@resource://mozmill/stdlib/securable-module.js -> file:///tmp/tmpiP1DK4.mozmill-tests/lib/addons.js:155 testDisableExtension@resource://mozmill/modules/frame.js -> file:///tmp/tmpiP1DK4.mozmill-tests/tests/functional/restartTests/testAddons_enableDisableExtension/test2.js:22 @resource://mozmill/modules/frame.js:557 @resource://mozmill/modules/frame.js:626 @resource://mozmill/modules/frame.js:669 @resource://mozmill/modules/frame.js:506 @resource://mozmill/modules/frame.js:681 @resource://jsbridge/modules/server.js:179 @resource://jsbridge/modules/server.js:183 @resource://jsbridge/modules/server.js:283 

Tests affected: 
Addons Manager tests 

Report: 
http://mozmill-release.blargon7.com/#/functional/report/c2b72632f20450b6d99d14c7095a92d3
Comment 1 Maniac Vlad Florin (:vladmaniac) 2012-05-04 02:38:29 PDT
This timeout is intermittent and its caused by the fact that lately, the discovery pane is loaded more lazily than usual. 

To reproduce this locally without any problems, just set the TIMEOUT value to 1s (default is 5s) 

I would propose to increase the timeout to 10s, due to this situation. Please approve or reject my proposal by case, so I know what to do next :)
Comment 2 Henrik Skupin (:whimboo) 2012-05-04 02:47:21 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #1)
> This timeout is intermittent and its caused by the fact that lately, the
> discovery pane is loaded more lazily than usual. 

Dave, Blair, or Jake, are you aware of any regression in loading the discovery pane lately? This happens in MV, and Europe at least.
Comment 3 Henrik Skupin (:whimboo) 2012-05-04 02:49:39 PDT
Vlad, beside that I wonder if we could add an option to our addons module to skip the loading of the discovery pane if it's not really necessary to wait for.
Comment 4 Maniac Vlad Florin (:vladmaniac) 2012-05-04 02:53:03 PDT
(In reply to Henrik Skupin (:whimboo) from comment #3)
> Vlad, beside that I wonder if we could add an option to our addons module to
> skip the loading of the discovery pane if it's not really necessary to wait
> for.

We can load something else instead of discovery pane (a local test page) for e.g. 
This will not affect the test at all because we are working with the extensions category here, and just change from disc pane to that. Good idea!
Comment 5 Henrik Skupin (:whimboo) 2012-05-04 03:58:10 PDT
So it should already work with skipping the waitForPageLoad by using the waitFor flag for open():

addonsManager.open({type: "shortcut", waitFor: false});

Please check which tests would benefit from.
Comment 6 Maniac Vlad Florin (:vladmaniac) 2012-05-04 04:03:18 PDT
(In reply to Henrik Skupin (:whimboo) from comment #5)
> So it should already work with skipping the waitForPageLoad by using the
> waitFor flag for open():
> 
> addonsManager.open({type: "shortcut", waitFor: false});
> 
> Please check which tests would benefit from.

Two questions: 
Should we load nothing instead of the discovery pane?
We can change this pref 'extensions.webservice.discoverURL' in order to skip loading disc pane, but do we practice pref changing in the API?
Comment 7 Henrik Skupin (:whimboo) 2012-05-04 04:08:36 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #6)
> Should we load nothing instead of the discovery pane?
> We can change this pref 'extensions.webservice.discoverURL' in order to skip
> loading disc pane, but do we practice pref changing in the API?

Not sure what you mean with the last part. But having an empty discovery pane for tests which absolutely don't test anything in it would make sense.
Comment 8 Maniac Vlad Florin (:vladmaniac) 2012-05-04 04:16:41 PDT
(In reply to Henrik Skupin (:whimboo) from comment #7)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #6)
> > Should we load nothing instead of the discovery pane?
> > We can change this pref 'extensions.webservice.discoverURL' in order to skip
> > loading disc pane, but do we practice pref changing in the API?
> 
> Not sure what you mean with the last part. But having an empty discovery
> pane for tests which absolutely don't test anything in it would make sense.

To make this happen we need to change that pref. And at the end of the test, we need to clear all prefs, so other tests in the run would not be affected. 

If I make the change in the addons module, how do I clear the pref?
Comment 9 Maniac Vlad Florin (:vladmaniac) 2012-05-04 04:29:47 PDT
Wait 

this line of code 
addonsManager.open({type: "shortcut", waitFor: false}); 

just ignores waiting for disc pane to load, and its not necessary to eliminate the disc pane by setting that pref, just replace 

addonsManager.open(); 

with 

addonsManager.open({type: "shortcut", waitFor: false});  

and the test will just ignore that load, and moves to the next category with a pass
Comment 10 Maniac Vlad Florin (:vladmaniac) 2012-05-04 04:33:14 PDT
Created attachment 621007 [details]
simplified testcase to demonstrate how we can ignore disc pane loading

Simplified testcase to demonstrate my last comment

We do not need to set prefs and eliminate loading of the discovery pane, we just need to ignore loading of the discovery pane if its not an interest to us in the test.
Comment 11 Maniac Vlad Florin (:vladmaniac) 2012-05-04 04:40:04 PDT
Ups! 

If we ignore the waitFor and disc pane is not loaded, the category is not found. 
So we do need to set the pref, but in the tests IMHO, not in the API
Comment 12 Jake Maul [:jakem] 2012-05-04 09:32:30 PDT
(In reply to Henrik Skupin (:whimboo) from comment #2)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #1)
> > This timeout is intermittent and its caused by the fact that lately, the
> > discovery pane is loaded more lazily than usual. 
> 
> Dave, Blair, or Jake, are you aware of any regression in loading the
> discovery pane lately? This happens in MV, and Europe at least.

We have a test set up for this in our new trial Catchpoint monitoring system... it does appear to have gotten slightly worse (performance-wise) around midnight PT. It's only a slightly higher average response time compared to the previous 12 hours, but becomes rather spikey.

I'm not sure if this is useful/helpful information, but there you go.
Comment 13 Henrik Skupin (:whimboo) 2012-05-04 11:28:58 PDT
Thanks Jake. Given that it will not fix our tests I would say that we do the following two steps:

1. We increase the timeout to 10s for all tests. That should also make the remote discovery pane tests work again.

2. For tests which do not test the discovery pane we set the preference to a local page which gets loaded immediately. We could consider a method in addons.js which set/resets the preference as needed, e.g.

.setDiscoveryPaneURL(aUrl)

If no url is specified we fall back to the default value means we reset the preference.
Comment 14 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-04 19:54:25 PDT
Using a timeout for something that hits the network seems like a very reliable way to have unreliable tests. Can it not wait for one of the various events instead?
Comment 15 Maniac Vlad Florin (:vladmaniac) 2012-05-07 00:18:21 PDT
(In reply to Blair McBride (:Unfocused) from comment #14)
> Using a timeout for something that hits the network seems like a very
> reliable way to have unreliable tests. Can it not wait for one of the
> various events instead?

Its always better to add an event listener than  just go blind with timeouts. 
Blair, can you name some events? I'll also look into this myself today
Comment 16 Blair McBride [:Unfocused] (mostly unavailable, needinfo open, reviews not) 2012-05-07 02:54:51 PDT
I should look at the code before blindly making suggestions :) Seems the timeout is the amount of time to wait for the ViewChanged event before giving up. But for the Disco pane, unless that timeout is excessively long, it will fail at some point. What Henrik suggests in comment 30 seems good, though if there's a way to special-case the Disco pane, I'd be tempted to have a timeout of 30sec (or more) if it's hitting the network.
Comment 17 Maniac Vlad Florin (:vladmaniac) 2012-05-08 04:10:15 PDT
So what would be the final decision here? Is it comment 13?

I would really like to get this fixed since it has an impact on our addons manager tests
Comment 18 Maniac Vlad Florin (:vladmaniac) 2012-05-08 23:34:21 PDT
Created attachment 622298 [details] [diff] [review]
wip patch v1.0

This is a proposed version of the fixed. 

1. The timeout was raised to 10s
2. I propose two methods, one setDiscoveryPaneURL() and resetDiscoveryPaneURL(), because in the same test you need to also cleanup the modified prefs. 
3. There are two enabled addons manager tests which I've changed to use the new helpers 
4. The skipped tests are not touched, they will be refactored once they are re-enabled 

Given there is no final decision clear to me at the moment about what are we going to do here, please approve or reject the patch
Comment 19 Maniac Vlad Florin (:vladmaniac) 2012-05-09 03:15:46 PDT
Comment on attachment 622298 [details] [diff] [review]
wip patch v1.0


Henrik, I see there are no comments on the feedback. 
Should this be ok, can you please review the patch and land? Would kindly appreciate that, thanks!
Comment 20 Henrik Skupin (:whimboo) 2012-05-09 04:29:28 PDT
Comment on attachment 622298 [details] [diff] [review]
wip patch v1.0

Damn, looks like those get lost somehow :( Once again...

>-const TIMEOUT = 5000;
>+const TIMEOUT = 10000;

Please don't increase the general TIMEOUT value. What you want here is to special case for the discovery pane and use the TIMOUT_REMOTE constant which is set to 30s. We can do that in a follow-up patch on this bug. For now it would be important to get the new code for changing the pref in and to update our tests accordingly.

>-
>+    

There are a couple of those unnecessary whitespace changes.

>+  setDiscoveryPaneURL: function AddonsManager_setDiscoveryPaneURL(aUrl) {
>+    prefs.preferences.setPref(AMO_DISCOVER_URL, aUrl);
>+  },
[..]
>+  resetDiscoveryPaneURL: function AddonsManager_resetDiscoveryPaneURL() {
>+    prefs.preferences.clearUserPref(AMO_DISCOVER_URL);
>+  },

Please stick by the alphabetical order in this module. While this looks fine we should figure out how to better use the AMO preferences array in the future so that we could also allow local test pages.

>+++ b/tests/functional/restartTests/testAddons_enableDisableExtension/test2.js
> function setupModule() {
>   controller = mozmill.getBrowserController();
>   addonsManager = new addons.AddonsManager(controller);
>+  
>+  addonsManager.setDiscoveryPaneURL(LOCAL_TEST_PAGE);

Use a blank line before the addonsManager instantiation but not that way. It's just about grouping code. A single line was ok but with this addition we should build out a new block.
Comment 21 Maniac Vlad Florin (:vladmaniac) 2012-05-09 06:21:10 PDT
Created attachment 622360 [details] [diff] [review]
patch v1.1

I left the general timeout to 5000. Good catch! 

Fixed nits. 

We will have a follow up patch to insert the pref in the AMO preferences array somehow, but I agree with you, if this is urgent, it can be landed asap. 

Thanks
Comment 22 Maniac Vlad Florin (:vladmaniac) 2012-05-09 06:23:30 PDT
> 
> >-
> >+    
> 

The unnecessary whitespace changes I had no luck removing that from the patch...I was using Xcode as an editor, maybe this is the cause. Hope we can live with them
Comment 23 Henrik Skupin (:whimboo) 2012-05-09 06:44:23 PDT
Comment on attachment 622360 [details] [diff] [review]
patch v1.1

>   /**
>+   * Reset discovery pane URL to default 
>+   */
>+  resetDiscoveryPaneURL: function AddonsManager_resetDiscoveryPaneURL() {
>+    prefs.preferences.clearUserPref(AMO_DISCOVER_URL);
>+  },
>+    
>+  /**
>+   * Set discovery pane URL 
>+   * @param {object} aUrl
>+   *        Custom defined URL to load within the 'GetAddons' section
>+   */
>+  setDiscoveryPaneURL: function AddonsManager_setDiscoveryPaneURL(aUrl) {
>+    prefs.preferences.setPref(AMO_DISCOVER_URL, aUrl);
>+  },

Not noticed before but those methods should not be on the AddonsManager class. Those are utility methods in this module. So move them to the global scope.

>+    

As mentioned please remove those extra whitespaces. If XCode doesn't work for you try Komodo or Pycharm. But I think that XCode also has a setting to show you whitespace characters.
Comment 24 Maniac Vlad Florin (:vladmaniac) 2012-05-10 01:13:00 PDT
Created attachment 622652 [details] [diff] [review]
patch v1.2

Good old gedit in Ubuntu never fails me :) 

Unnecessary spaces are gone. 
The helper functions were moved to the global scope and exported as methods
Comment 25 Henrik Skupin (:whimboo) 2012-05-10 01:48:05 PDT
Comment on attachment 622652 [details] [diff] [review]
patch v1.2

I absolutely love this patch! It will save us really a lot of time we do not have to spend anymore in waiting for the discovery pane loaded.

Vlad, before I want to land this, please make sure to update all enabled tests which make use of the add-ons manager. As I can see you missed at least the ones in functional/testAddons. Once this is done we can land it. Would you be able to do it in the next 2 hours? That would be awesome.
Comment 26 Remus Pop (:RemusPop) 2012-05-10 02:26:14 PDT
Created attachment 622660 [details] [diff] [review]
patch v1.3

Modified the rest of enabled tests to make us of the new setDiscoveryPaneURL().
Comment 27 Henrik Skupin (:whimboo) 2012-05-10 03:03:12 PDT
Comment on attachment 622660 [details] [diff] [review]
patch v1.3

Looks great. I will land it on default and if it proves to be working we will backport it to all the other branches.
Comment 28 Henrik Skupin (:whimboo) 2012-05-10 03:05:38 PDT
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/c91790647002

I will call this fixed. The remaining work for the timeout value special to the discovery pane we should handle separately on another bug. Vlad, please file one when you are back.
Comment 29 Henrik Skupin (:whimboo) 2012-05-10 04:20:26 PDT
All those failures are gone now:
http://mozmill-ci.blargon7.com/#/functional/report/c2b72632f20450b6d99d14c709b1ae1b

Landed backports for:
http://hg.mozilla.org/qa/mozmill-tests/rev/e01e2f9ad8db (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ab226d9a7604 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/59a9ea5778aa (release)

Vlad, can you please come up with a new patch for esr10?
Comment 30 Maniac Vlad Florin (:vladmaniac) 2012-05-10 04:42:10 PDT
Sure, on it!
Comment 31 Maniac Vlad Florin (:vladmaniac) 2012-05-10 05:56:57 PDT
Created attachment 622700 [details] [diff] [review]
[mozilla-esr10] patch v1.0

patch for mozilla-esr10 branch
Comment 32 Henrik Skupin (:whimboo) 2012-05-10 06:25:58 PDT
Also landed on ESR branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/958a56b3b1b3

Thanks everyone for your help!

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