Timeout failure in testSwitchPanes.js | testPreferencesPanes

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: aaronmt, Assigned: vladmaniac)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure], URL)

Attachments

(3 attachments, 9 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Updated

7 years ago
See Also: → bug 671379
(Assignee)

Updated

7 years ago
Assignee: nobody → vlad.maniac
Status: NEW → ASSIGNED

Comment 1

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/16064039
(Assignee)

Comment 2

7 years ago
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?
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.
(Assignee)

Comment 4

7 years ago
Created attachment 547393 [details] [diff] [review]
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.
Attachment #547393 - Flags: review?(hskupin)
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.
Attachment #547393 - Flags: review?(hskupin) → review-
(Assignee)

Comment 6

7 years ago
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?
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.
(Assignee)

Comment 8

7 years ago
Created attachment 548167 [details]
patch v1.1
(Assignee)

Comment 9

7 years ago
Created attachment 548172 [details]
patch v1.1
(Assignee)

Comment 10

7 years ago
Created attachment 548175 [details] [diff] [review]
patch v1.1

Added patch with requested changes. 

Not sure about the alignment and indentation, it's a bit tricky in prefs.js
Attachment #547393 - Attachment is obsolete: true
Attachment #548167 - Attachment is obsolete: true
Attachment #548172 - Attachment is obsolete: true
Attachment #548175 - Flags: review?(hskupin)
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.
Attachment #548175 - Flags: review?(hskupin) → review-
(Assignee)

Comment 12

7 years ago
Created attachment 550973 [details] [diff] [review]
patch v1.2

Fixed!
Attachment #548175 - Attachment is obsolete: true
Attachment #550973 - Flags: review?(hskupin)
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.
Attachment #550973 - Flags: review?(hskupin) → review-
(Assignee)

Comment 14

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

Comment 15

7 years ago
Created attachment 551026 [details] [diff] [review]
patch v1.3

Added patch with requested changes
Attachment #550973 - Attachment is obsolete: true
Attachment #551026 - Flags: review?(hskupin)
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).
Attachment #551026 - Flags: review?(hskupin) → review+
(Assignee)

Comment 18

7 years ago
Cool, sounds like a plan
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?
(Assignee)

Comment 20

7 years ago
Sure, I'll see to it as soon as possible
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.
(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.
(Assignee)

Comment 23

7 years ago
Created attachment 552649 [details] [diff] [review]
patch v2.0

Please r? ashughes then pass to hskupin if necessary, since ashughes requested this change :) thanks !
Attachment #552649 - Flags: review?(anthony.s.hughes)
(Assignee)

Updated

7 years ago
Blocks: 671379
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.
(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 on attachment 552649 [details] [diff] [review]
patch v2.0

Given the previous comment, I see no problems with this patch.
Attachment #552649 - Flags: review?(anthony.s.hughes) → review+
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.
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][checkin-needed]
Keywords: checkin-needed
Whiteboard: [mozmill-test-failure][checkin-needed] → [mozmill-test-failure]
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?
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME
Keywords: checkin-needed
(Assignee)

Comment 32

6 years ago
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'
(Assignee)

Comment 33

6 years ago
Created attachment 594711 [details] [diff] [review]
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
Attachment #594711 - Flags: review?(anthony.s.hughes)
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"
Attachment #594711 - Flags: review?(anthony.s.hughes) → review-
(Assignee)

Comment 35

6 years ago
Created attachment 594928 [details] [diff] [review]
fix v1.1 [landed:default,aurora]

Fixed
Attachment #594711 - Attachment is obsolete: true
Attachment #594928 - Flags: review?(anthony.s.hughes)
Attachment #594928 - Flags: review?(anthony.s.hughes) → review+
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)
Attachment #594928 - Attachment description: fix test patch v1.1 → fix v1.1 [landed:default,aurora]
Landed on aurora and default as the failure got merged forward last week. Please verify this is now passing in tomorrow's testruns.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Status: REOPENED → ASSIGNED
(Assignee)

Comment 40

6 years ago
Created attachment 595384 [details] [diff] [review]
fix patch v2.0

Added a new helpler function in prefs.js to wait for pane selection.
Attachment #594928 - Attachment is obsolete: true
Attachment #595384 - Flags: review?(hskupin)
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?
Attachment #595384 - Flags: review?(hskupin) → review-
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 42

6 years ago
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
(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.
(Assignee)

Comment 44

6 years ago
(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
(Assignee)

Comment 45

6 years ago
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.
(Assignee)

Comment 46

6 years ago
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?
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?
(Assignee)

Comment 48

6 years ago
(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
(Assignee)

Comment 49

6 years ago
> 
> 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.
Is there a difference between a Nightly and 10.0 Release build?
(Assignee)

Comment 51

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

None
(Assignee)

Comment 52

6 years ago
Created attachment 596943 [details] [diff] [review]
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
Attachment #595384 - Attachment is obsolete: true
Attachment #596943 - Flags: review?(hskupin)
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.
Attachment #596943 - Flags: review?(hskupin) → review-
(Assignee)

Comment 54

6 years ago
(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
(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.
(Assignee)

Comment 56

6 years ago
Created attachment 601963 [details] [diff] [review]
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
Attachment #596943 - Attachment is obsolete: true
Attachment #601963 - Flags: review?(hskupin)
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?
Attachment #601963 - Flags: review?(hskupin) → review-
(Assignee)

Comment 58

6 years ago
> 
> 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
For which reason?
(Assignee)

Comment 60

6 years ago
(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
(Assignee)

Comment 61

6 years ago
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
(Assignee)

Comment 62

6 years ago
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.
Could we not implement a listener in the shared module to handle these events?
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?
(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
(Assignee)

Comment 66

6 years ago
(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.
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.
The paneId setter will be refactored in bug 757340.
I don't see it failing anymore. Do we need to keep this open?
Depends on: 757340
I think we can safely close this bug, yes.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.