Closed Bug 751832 Opened 9 years ago Closed 9 years ago

Addons tests are failing with "Selected category has been loaded"

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

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

RESOLVED FIXED
Tracking Status
firefox12 --- fixed
firefox13 --- fixed
firefox14 --- fixed
firefox15 --- fixed
firefox-esr10 --- fixed
status1.9.2 --- unaffected

People

(Reporter: vladmaniac, Assigned: vladmaniac)

Details

(Keywords: regression, Whiteboard: [mozmill-test-failure][qa-])

Attachments

(3 files, 3 obsolete files)

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
Whiteboard: [mozmill-test-failure]
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
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 :)
(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.
Keywords: regression
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.
(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!
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.
(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?
(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.
(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?
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
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.
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
(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.
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.
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?
(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
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.
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
Attached patch wip patch v1.0 (obsolete) — Splinter Review
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
Attachment #622298 - Flags: feedback?(hskupin)
Attachment #622298 - Flags: feedback?(hskupin) → feedback+
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!
Attachment #622298 - Flags: review?(hskupin)
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.
Attachment #622298 - Flags: review?(hskupin) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
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
Attachment #622298 - Attachment is obsolete: true
Attachment #622360 - Flags: review?(hskupin)
> 
> >-
> >+    
> 

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 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.
Attachment #622360 - Flags: review?(hskupin) → review-
Attached patch patch v1.2 (obsolete) — Splinter Review
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
Attachment #622360 - Attachment is obsolete: true
Attachment #622652 - Flags: review?(hskupin)
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.
Attachment #622652 - Flags: review?(hskupin) → review+
Attached patch patch v1.3Splinter Review
Modified the rest of enabled tests to make us of the new setDiscoveryPaneURL().
Attachment #622652 - Attachment is obsolete: true
Attachment #622660 - Flags: review?(hskupin)
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.
Attachment #622660 - Flags: review?(hskupin) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Sure, on it!
patch for mozilla-esr10 branch
Attachment #622700 - Flags: review?(hskupin)
Attachment #622700 - Flags: review?(hskupin) → review+
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][qa-]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.