671382 - Timeout failure in testSwitchPanes.js | testPreferencesPanes

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

Test: testSwitchPanes.js
Module: testPreferencesPanes
Error: controller.waitForEval: Timeout exceeded for 'subject.selector.getAttribute('pane') == subject.dlg.selectedPane.getNode().id'
Branch: Nightly (8.0a1)
Platform: Mac OS X 10.6.7

Earliest reported with 20110709030751
http://hg.mozilla.org/mozilla-central/rev/97012a02db93

http://mozmill-release.brasstacks.mozilla.com/#/functional/failure?branch=8.0&platform=Mac&from=2011-07-01&to=2011-07-13&test=%2FtestPreferences%2FtestSwitchPanes.js&func=testPreferencesPanes

Comment 1

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/16064039

Comment 2

[Maniac Vlad Florin (:vladmaniac)]

I cannot reproduce the failure. I've ran also with Mozmill Crowd extensions, no luck. 

The only thing I can find wrong in the test is const gTimeout is never used and should be disposed of, and the test passed though the Sync pane - we should also check for that. 

The full log of the error would be very much helpful. 
Any ideas, anyone?

Comment 3

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

If you can't reproduce it, the first thing we should do is to remove the assertJS and waitForEval calls and replace those with the methods we are currently using. That helps us to get at least more information about the failure.

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0

Initial patch. 

Disposed of waitForEval in prefs.js for setter ID function. Could not change 
for getter function because waitFor has a callback as parameter and 'this.' pointer is not accessible from there. (sorry if I explain badly, see the code). 
Anyway, this test uses the setter function so it will do for now. 

I hope this fixes our problem, or at least be helpful in identifying it.

Comment 5

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

Comment on attachment 547393 [details] [diff] [review]
patch v1.0

>     // Check if the correct selector is selected
>     var selector = this.getElement({type: "selector"});
>-    this._controller.waitForEval("subject.selector.getAttribute('pane') == subject.newPane", gTimeout, 100,
>-                                 {selector: selector.getNode().selectedItem, newPane: id});
>+
>+    this._controller.waitFor(function () {
>+      return selector.getNode().selectedItem.getAttribute('pane') == id;
>+    }, "Pane has been changed - got '" + selector.getNode().selectedItem.getAttribute('pane') + 
>+       "' expected '" + id + "'");
>+

You have to use '===' for the comparison here. Also update the message to only include the target id. You can't use the pane attribute here because it will not contain the right value once passed to the waitFor method.

The failure we currently have is not in this function but in the paneId() getter. That one needs an update too.

>+++ b/tests/functional/testPreferences/testSwitchPanes.js
[..]
> const gDelay = 100;
>-const gTimeout = 5000;
> 
[..]
>                "paneMain", "paneTabs", "paneContent", "paneApplications",
>-               "panePrivacy", "paneSecurity", "paneAdvanced"
>+               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced"

Please do those changes on another bug.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Thanks for r Henrik, 

This piece of code 

var panes = [
               "paneMain", "paneTabs", "paneContent", "paneApplications",
               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced"
              ];

  // Step through each of the panes
  panes.forEach(function (pane) {
    prefDialog.paneId = pane;
    controller.sleep(gDelay);
  });

makes me think we are not using the paneID getter function in this test at all. We have the ids saved in the array, and we set them with prefDialog.paneId = pane; So i tend to think we are using the setter function, which causes the error. What do you think?

Comment 7

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

Please check the error message. This string only appears in the paneId getter. So without looking at the test or module we call this probably internally in a waitFor call.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.1

Added patch with requested changes. 

Not sure about the alignment and indentation, it's a bit tricky in prefs.js

Comment 11

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

Comment on attachment 548175 [details] [diff] [review]
patch v1.1

> const gTimeout = 5000;
>+const gDelay = 100;

Both constants can be removed. They are not needed anymore.

>+    this._controller.waitFor(function () {
>+      return selector.getNode().selectedItem.getAttribute('pane') === this.selectedPane.getNode().id;
>+    }, "Pane has been changed - expected '" + this.selectedPane.getNode().id + "'", 
>+       gTimeout, gDelay, this);

... just specify undefined in both cases.

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.2

Fixed!

Comment 13

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

Comment on attachment 550973 [details] [diff] [review]
patch v1.2

In the last review I mentioned that we can remove both constants. The timeout is still there. Please check the review comments again before you upload a new patch. That will help us to reduce the amount of review cycles. Thanks.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

I've not added that constant in the first place Henrik, this is why I didn't remove it from the declarations block.

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.3

Added patch with requested changes

Comment 16

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

Comment on attachment 551026 [details] [diff] [review]
patch v1.3

Ok, looks good. I will land it across branches and we will have to check again in the next days if the failure message becomes cleaner or if we even fixed the failure (which I don't think).

Comment 17

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d336301e492d (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/0ca56f5d3a95 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/94272cc186c4 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/92b3bae02d36 (release)

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

Cool, sounds like a plan

Comment 19

[u279076]

We now get "Pane has been changed - expected 'panePrivacy'". While this is more useful than before, it would still be nice to see what pane is currently active in the message.

Vlad, can you please update the test?

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Sure, I'll see to it as soon as possible

Comment 21

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

It shouldn't matter which pane is currently set. As it looks like the 'waitThenClick' action fails. Also we can't add the current pane to the message of the waitFor call, because it doesn't have to be correct either. The message gets passed when waitFor is called, so the value is from exactly that time but not 5 seconds later when the error is thrown.

Comment 22

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> It shouldn't matter which pane is currently set.

While this may be a true assumption, I would still like to know where we came from. All this message tells me now is that we didn't get the Privacy pane (not enough to go on IMO). Knowing GOT as well as EXPECTED helps provide context and makes debugging a little bit easier. 

In other words, any time we get an error which provides "expected" I'd like to see "got" as well. Having "expected" without "got" is useless to me as there is no basis for comparison.

Vlad, please make this change as requested.

Comment 23

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

Please r? ashughes then pass to hskupin if necessary, since ashughes requested this change :) thanks !

Comment 24

[u279076]

Comment on attachment 552649 [details] [diff] [review]
patch v2.0

>     this._controller.waitFor(function () {
>       return selector.getNode().selectedItem.getAttribute('pane') === this.selectedPane.getNode().id;
>-    }, "Pane has been changed - expected '" + this.selectedPane.getNode().id + "'", 
>-       undefined, undefined, this);
>+    }, "Pane has been changed - expected '" + this.selectedPane.getNode().id + "'got '" + 
>+       selector.getNode().selectedItem.getAttribute('pane') + "'" , undefined, undefined, this);

I don't think "undefined, undefined, this" is required, is it Henrik? Can we clean this up?

>     this._controller.waitFor(function () {
>       return selector.getNode().selectedItem.getAttribute('pane') === id;
>-    }, "Pane has been changed - expected '" + id + "'");    
>+    }, "Pane has been changed - expected '" + id + "'got '" + 
>+       selector.getNode().selectedItem.getAttribute('pane') + "'");    

Can we use the assertions module for both of these waitFor()? 

Not going to r- this because this patch could be r+ if my questions are proven wrong.

Comment 25

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #24)
> >     this._controller.waitFor(function () {
> >       return selector.getNode().selectedItem.getAttribute('pane') === this.selectedPane.getNode().id;
> [..]
> >+       selector.getNode().selectedItem.getAttribute('pane') + "'" , undefined, undefined, this);
> 
> I don't think "undefined, undefined, this" is required, is it Henrik? Can we
> clean this up?

As you can see we are using this inside of the waitFor callback. Whenever that's happening the 4th parameter has to be specified. Since we can't skip parameters we also have to use undefined for timeout and delay.

> Can we use the assertions module for both of these waitFor()? 

No, because we still have to wait for the pane. This action is delayed and an immediate assert will probably always fail here.

Comment 26

[u279076]

Comment on attachment 552649 [details] [diff] [review]
patch v2.0

Given the previous comment, I see no problems with this patch.

Comment 27

[u279076]

I will check this in as soon as Firefox 7.0b2 testing finishes. If someone wants to take it and check it in early, please do so. This was originally reported on 8.0 Nightly. I'm assuming this patch should be checked in on default and aurora.

Comment 28

[u279076]

I was just about to check this in when I noticed the dashboard report:
http://mozmill-release.brasstacks.mozilla.com/#/functional/failure?branch=All&platform=All&from=2011-07-01&to=2011-08-31&test=%2FtestPreferences%2FtestSwitchPanes.js&func=testPreferencesPanes

It appears that this failure is only happening on 8.0a1 (Nightly) and has ceased since August 19th (nearly 2 weeks ago). Does this still need to be checked-in?

Comment 29

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

Well, up to you. You wanted to have the got/expected information which is part of v2.0 of the patch. If you have changed your mind we can drop it. As said earlier it will even not give us the correct information.

Comment 30

[u279076]

I'm going to stand on the side of "if it ain't broke, don't fix it". Resolving WORKSFORME. We can re-address this if it starts failing again.

Comment 31

[Maniac Vlad Florin (:vladmaniac)]

Failling again - reopening 

http://mozmill-release.blargon7.com/#/functional/failure?branch=12.0&platform=Mac&from=2012-02-04&to=2012-02-06&test=%2FtestPreferences%2FtestSwitchPanes.js&func=testSwitchPanes.js%3A%3AtestPreferencesPanes

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

Pane 'Applications' is loaded lazily than the other panes, this is visible with the eye. 
Right now the test uses a controller.sleep(gDelay) which is a fixed time at 1/10 seconds. 

The error "Pane has been changed - expected 'panePrivacy'" is clearly happening because of that. We will then try to loose the sleep or at least increase 'gDelay'

Comment 33

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix test patch v1.0

This is a fix for the test file - 

Replaced the sleep with a waitFor, returning 'true' when the currently selected pane maches the current pane from the list of string elements in the test - no need for sleep anymore

Comment 34

[u279076]

Comment on attachment 594711 [details] [diff] [review]
fix test patch v1.0

>+    controller.waitFor(function () {
>+      var sel = controller.window.document.documentElement.getAttribute("lastSelected");

Please use a more meaningful variable name than "sel".

>+      return (sel === pane); 
>+    }, "Pane '" + pane + "' has been selected");

"'sel' pane has been selected"

Comment 35

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix v1.1 [landed:default,aurora]

Fixed

Comment 36

[u279076]

Comment on attachment 594928 [details] [diff] [review]
fix v1.1 [landed:default,aurora]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d978189ae4a0 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/07415e3e4874 (mozilla-aurora)

Comment 37

[u279076]

Landed on aurora and default as the failure got merged forward last week. Please verify this is now passing in tomorrow's testruns.

Comment 38

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

Sorry to say but the code which has been added here should really be in the prefs shared module. That's nothing a test itself should ever face. See also  bug 671379 which has the same issue.

Comment 39

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

Backed out patches for an streamlined fix across branches:

http://hg.mozilla.org/qa/mozmill-tests/rev/a6ff454cdf1e (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/72e7479c3684 (aurora)

Comment 40

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v2.0

Added a new helpler function in prefs.js to wait for pane selection.

Comment 41

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

Comment on attachment 595384 [details] [diff] [review]
fix patch v2.0

>+  waitForPaneSelection: function preferencesDialog_waitForPaneSelection(paneId) {
>+    var lastSelected = this._controller.window.document.documentElement.getAttribute("lastSelected");
>+
>+    this._controller.waitFor(function () {
>+      return (lastSelected === paneId); 
>+    }, "'" + lastSelected + "' pane has been selected");
>+  },

We already have code like this in the paneId setter. So what's wrong with PREF_DIALOG_SELECTOR's selectedItem.getAttribute('pane') element?

Comment 42

[Maniac Vlad Florin (:vladmaniac)]

It is a bit redundant I know, but there is a difference. When we call this waitFor a pane was already selected. When switching to a new pane, if the content from the previous pane is not loaded the expected pane id would remain the same and not change. 

What is wrong here, and i figured it out while writing is that its useless to call this because of the waitFor in paneId setter. That one will be called first and fail -  it would not get to wait for pane loading. 

The failure for this specific test (and others) is because of the 'paneApplications' which loads a list lazily. This is why the error is "expected panePrivacy" which is next. 

Need to optimize the waitFor in the paneId setter somehow. 

Thanks for reviewing, i know what i have to do next.just ignore this patch please

Comment 43

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #42)
> What is wrong here, and i figured it out while writing is that its useless
> to call this because of the waitFor in paneId setter. That one will be
> called first and fail -  it would not get to wait for pane loading. 

So if the current check doesn't work it has to be replaced by the waitFor you propose. But please don't add another waitFor which seems to be clearly not necessary.

Comment 44

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #43)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #42)
> > What is wrong here, and i figured it out while writing is that its useless
> > to call this because of the waitFor in paneId setter. That one will be
> > called first and fail -  it would not get to wait for pane loading. 
> 
> So if the current check doesn't work it has to be replaced by the waitFor
> you propose. But please don't add another waitFor which seems to be clearly
> not necessary.

Investigated further - and waited for the attributes which change when a pane is selected - "selected" and "focused" What I find really strange is that the first pane, 'paneMain' is not selected in the first place. 

http://pastebin.mozilla.org/1483592

Comment 45

[Maniac Vlad Florin (:vladmaniac)]

The 'paneMain' is the pane which is implicitly shown when we open preferences. If we want to check for 'selected' attribute, we would want to check for the 'paneMaine' last.

Comment 46

[Maniac Vlad Florin (:vladmaniac)]

Also, 

    var button = this.getElement({type: "selector_button", value: id});
    this._controller.waitThenClick(button);

the waitThenClick is not reliable at all, it misses sometimes to select panes. ('paneAdvanced') for example, as listed here 

    http://pastebin.mozilla.org/1483603

Given the data here, what do you suggest?

Comment 47

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

Why do we miss the advanced pane? We should have a test failure in such a case. If selected is not reliable there should probably another property to look for in the JS object or the DOM. If there is nothing we probably would have to fallback to focused. What happens if Firefox is runinng in the background? Is focus still true in those cases?

Comment 48

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #47)
> Why do we miss the advanced pane? We should have a test failure in such a
> case. 

I cannot explain why at the moment :(

If selected is not reliable there should probably another property to
> look for in the JS object or the DOM. If there is nothing we probably would
> have to fallback to focused. 

Only those two properties apply "selected" and "focused" 

What happens if Firefox is runinng in the
> background? Is focus still true in those cases?

Will get back to you asap with the result for this one

Comment 49

[Maniac Vlad Florin (:vladmaniac)]

> 
> What happens if Firefox is runinng in the
> > background? Is focus still true in those cases?
> 
> Will get back to you asap with the result for this one

The test fails if running anything on Firefox in background. The pref pane looses focus and the test fails.

Comment 50

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

Is there a difference between a Nightly and 10.0 Release build?

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> Is there a difference between a Nightly and 10.0 Release build?

None

Comment 52

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v3.0

According to the results of my investigation, this code has the best results 
in handling pref panes. 

We are waiting now for the changed attribute "focused" when visiting a pane, and, we are addressing the fact that 'paneMain' is the default selected pane when pref dialog is opened, so we are checking for it last, not first, so there would be no conflicts. 

The patch is tested for all branches

Comment 53

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

Comment on attachment 596943 [details] [diff] [review]
fix patch v3.0

>+var {assert} = require("assertions");
[..]
>+     
>+    // Wait for the pane to be selected 
>+    this._controller.waitFor(function () { 
>+      return (button.getNode().getAttribute("focused") === "true");
>+    }, "Pane '" + this.selectedPane.getNode().id + "' has been selected", 
>+       undefined, undefined, this);
[..]
>+    assert.equal(selector.getNode().selectedItem.getAttribute('pane'), id, 
>+                 "Pane has been changed - expected '" + id + "'");

Please do not assert for the pane id separately. Instead combine it with the condition of the waitFor call.

At the same time can we remove the now obsolete waitFor call in the paneId getter?

>+++ b/tests/functional/testPreferences/testSwitchPanes.js
>-               "paneMain", "paneTabs", "paneContent", "paneApplications",
>-               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced"
>+               "paneTabs", "paneContent", "paneApplications",
>+               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced", "paneMain"

You don't know that the main pane is selected, because the test will be executed after other tests could already have changed the pane or not. So this change is a no-op and can bring back the test failures. Set a fixed initial condition instead.

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #53)
> Comment on attachment 596943 [details] [diff] [review]
> fix patch v3.0
> 
> >+var {assert} = require("assertions");
> [..]
> >+     
> >+    // Wait for the pane to be selected 
> >+    this._controller.waitFor(function () { 
> >+      return (button.getNode().getAttribute("focused") === "true");
> >+    }, "Pane '" + this.selectedPane.getNode().id + "' has been selected", 
> >+       undefined, undefined, this);
> [..]
> >+    assert.equal(selector.getNode().selectedItem.getAttribute('pane'), id, 
> >+                 "Pane has been changed - expected '" + id + "'");
> 
> Please do not assert for the pane id separately. Instead combine it with the
> condition of the waitFor call.

Why do we want this? We wait for the pane to be selected, and then assert if we selected the right pane id. Seems right to me. Its a sanity check, but please help me understand why do we need this change. 

> 
> At the same time can we remove the now obsolete waitFor call in the paneId
> getter?

The waitFor call in the getter is not obsolete. The getter simply extracts the paneId, it does not set it, so we don;t want to wait for the changing attribute there. Again, IMHO. 
> 
> >+++ b/tests/functional/testPreferences/testSwitchPanes.js
> >-               "paneMain", "paneTabs", "paneContent", "paneApplications",
> >-               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced"
> >+               "paneTabs", "paneContent", "paneApplications",
> >+               "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced", "paneMain"
> 
> You don't know that the main pane is selected, because the test will be
> executed after other tests could already have changed the pane or not. So
> this change is a no-op and can bring back the test failures. Set a fixed
> initial condition instead.

Good catch! This is right, I missed considering the fact that Firefox remembers the last pane state - this is rusty code and needs changing. Thanks

Comment 55

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #54)
> Why do we want this? We wait for the pane to be selected, and then assert if
> we selected the right pane id. Seems right to me. Its a sanity check, but
> please help me understand why do we need this change. 

Well it could become a raise condition. If switching the panes takes longer the button can already be focused but the pane has not been switched. In those cases we would fail. It's safer just to wait a bit longer and not to fail.

> > At the same time can we remove the now obsolete waitFor call in the paneId
> > getter?
> 
> The waitFor call in the getter is not obsolete. The getter simply extracts
> the paneId, it does not set it, so we don;t want to wait for the changing
> attribute there. Again, IMHO. 

No, the waitFor call is not necessary anymore. See your patch, it's not where it should be in the setter. The getter should directly return the pane id.

Comment 56

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v4.0

All change requests addressed. 

- Getter now simply returns the pane id, without waiting 
- Included the assert as a condition for the return in waitFor, as requested. 
- Set initial condition in the test to select the mainPane first, then browser through the others using the forEach 

Tested across branches and its a local pass

Comment 57

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

Comment on attachment 601963 [details] [diff] [review]
fix patch v4.0

>+var {assert} = require("assertions");

Not necessary anymore.

>+    this._controller.waitFor(function () { 
>+      var selector = this.getElement({type: "selector"});

You do not want to retrieve the selector for each call of the callback.

>+      return ((button.getNode().getAttribute("focused") === "true") && 
>+              (selector.getNode().selectedItem.getAttribute('pane') === id));
>+    }, "Pane '" + this.selectedPane.getNode().id + "' has been selected", 
>+       undefined, undefined, this);

Why you specify this as parameter here? I do not see that it is getting used.

>+const mainPaneId = "paneMain";
> var prefDialogCallback = function(controller) {
>   var prefDialog = new prefs.preferencesDialog(controller);
> 
>+  // Select the main pane first, as an initial condition
>+  prefDialog.paneId = mainPaneId;
>+  controller.sleep(gDelay);  
>+
>   // List of all available panes inside the Preferences window
>   var panes = [
>-               "paneMain", "paneTabs", "paneContent", "paneApplications",
>+               "paneTabs", "paneContent", "paneApplications",
>                "panePrivacy", "paneSecurity", "paneSync", "paneAdvanced"
>               ];

That would skip the test for the main pane. I suggest you read paneMain to the panes array and initially select the advanced pane. Also constants have capital letters and in this case it should be named like INITIAL_PANE. 

>     prefDialog.paneId = pane;
>     controller.sleep(gDelay);

Why do you still call sleep? I thought this issue has been fixed with the additional check for focus?

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

> 
> Why do you still call sleep? I thought this issue has been fixed with the
> additional check for focus?

Removing sleep causes a disconnect error 

http://pastebin.mozilla.org/1503265

Comment 59

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

For which reason?

Comment 60

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #59)
> For which reason?

Not sure right now, the patch will be delayed a bit because of this disconnect

Comment 61

[Maniac Vlad Florin (:vladmaniac)]

The disconnect error is caused by my API change. I removed the sleep() from the test in a clean repo, meaning testPreferences/testSwitchPanes.js and it does not cause a disconnect, so the reason for the sleep being there is for sanity probably, just to get some extra-time. 

I will continue tomorrow and hopefully fix it

Comment 62

[Maniac Vlad Florin (:vladmaniac)]

From 15.02.2012 to 07.03.2012 the test is failing on Mac Os X 10.6.8 only 

http://mozmill-release.blargon7.com/#/functional/failure?branch=12.0&platform=All&from=2012-02-15&to=2012-03-07&test=%2FtestPreferences%2FtestSwitchPanes.js&func=testSwitchPanes.js%3A%3AtestPreferencesPanes

http://mozmill-release.blargon7.com/#/functional/failure?branch=10.0&platform=All&from=2012-02-15&to=2012-03-07&test=%2FtestPreferences%2FtestSwitchPanes.js&func=testSwitchPanes.js%3A%3AtestPreferencesPanes 

this is on mozilla-aurora and mozilla-release branch. 

The panes have a callback on 'onload' event which is called depending on what OS are we on, and this callback handles pane initialization. 

For example gTabsPane.init() is called only for win xp according to 
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/tabs.js#40
and tested this manually also.

Comment 63

[u279076]

Could we not implement a listener in the shared module to handle these events?

Comment 64

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

AFAIR there is no event which gets thrown when the pane has been fully loaded. But feel free to check again to make sure it is that way. Btw. when will the in-content prefs land? Will that already happen for Firefox 14?

Comment 65

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #64)
> AFAIR there is no event which gets thrown when the pane has been fully
> loaded. But feel free to check again to make sure it is that way. 

Vlad, could you look into this?

> Btw. when will the in-content prefs land? Will that already happen for Firefox 14?

Currently targeting Firefox 15:
https://wiki.mozilla.org/In-content_preferences

Comment 66

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #65)
> (In reply to Henrik Skupin (:whimboo) from comment #64)
> > AFAIR there is no event which gets thrown when the pane has been fully
> > loaded. But feel free to check again to make sure it is that way. 
> 
> Vlad, could you look into this?
> 
> > Btw. when will the in-content prefs land? Will that already happen for Firefox 14?
> 
> Currently targeting Firefox 15:
> https://wiki.mozilla.org/In-content_preferences

Thanks for your fast answer on this Anthony. 

As I said in the e-mails, this one of my P1 failure but was postponed 1 day because of my mac machine. I will really want to reproduce the failure manually first and observe the behavior before making the final statement. 

The plan is to get a VM on the mac mini today and make a test environment as close as qa-set is possible see if I can reproduce locally. 

Also, need to take a look of what's changing in that feature so I get prepared in time for the change.

Comment 67

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

Not sure if this was the right fix or if we should listen for an event which gets triggered. Remus was investigating this so lets get him added here.

Comment 68

[Remus Pop (:RemusPop)]

The paneId setter will be refactored in bug 757340.
I don't see it failing anymore. Do we need to keep this open?

Comment 69

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

I think we can safely close this bug, yes.