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

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

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

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][⌚️UTC+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.

Comment 3

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

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

(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][⌚️UTC+2]]

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

(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][⌚️UTC+2]]

(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)]

(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)]

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

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

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]]

(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][⌚️UTC+2]]

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] (UNAVAILABLE)]

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

(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] (UNAVAILABLE)]

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

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

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

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][⌚️UTC+2]]

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

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

> 
> >-
> >+    
> 

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][⌚️UTC+2]]

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

Attachment: 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][⌚️UTC+2]]

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

Attachment: patch v1.3

Modified the rest of enabled tests to make us of the new setDiscoveryPaneURL().

Comment 27

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

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][⌚️UTC+2]]

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][⌚️UTC+2]]

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

Sure, on it!

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [mozilla-esr10] patch v1.0

patch for mozilla-esr10 branch

Comment 32

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

Also landed on ESR branch:
http://hg.mozilla.org/qa/mozmill-tests/rev/958a56b3b1b3

Thanks everyone for your help!