Timeout failure in /testTabView_SwitchTabs/test1.js | TabView is still open.

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
P2
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: mihaelav, Assigned: cosmin)

Tracking

unspecified

Firefox Tracking Flags

(firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox29 fixed, firefox30 fixed, firefox-esr10 wontfix, firefox-esr17 wontfix, firefox-esr24 fixed)

Details

(Whiteboard: [mozmill-endurance][mozmill-test-failure] s=130211 u=failure c=tabview p=1, URL)

Attachments

(4 attachments, 12 obsolete attachments)

1.15 KB, patch
davehunt
: review+
Details | Diff | Splinter Review
8.62 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
8.68 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
8.70 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
URL for the dashboard report where the failure occurs:
http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/b2f105bf5adf3d9f861770c4a991e7cb

TEST: /testTabView_SwitchTabs/test1.js::testSwitchTabs
ERROR: TimeoutError("TabView is still open.")@resource://mozmill/modules/utils.js:429 waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:467 
WHEN: 2011-09-05
FIRST: 2011-09-05
FREQ: 1
BRANCHES: aurora
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill-test-failure]
Mihaela, thank you for logging this failure. That one is a one-time failure on Nightly. So not sure if this is only related to a test-run which has been interfered by the user or another application. Means we lost the focus on the application. Is it reproducible?
OS: Mac OS X → All
Hardware: x86_64 → All
Your report mentioned that this occurred on Aurora, however the test result indicates that it was in fact on Nightly. I have attempted to replicate the failure locally on the latest Aurora and Nightly builds without success. I would like to see if this issue recurs before investigating further.
FWIW, I see an instance of this failure on the Nightly Mac OSX 10.6.7 testrun on Sept 5 and Sept 6.
Could that be a new regression specifically on OS X?
(Reporter)

Comment 5

7 years ago
Confirming same fail on Mac 10.6.7 x86_64 with build 20110906030844 from Sept 6

Comment 6

7 years ago
A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17952765
Mihaela, are you able to reproduce this failure locally? If yes, does it also happen for Aurora builds? If not can you check some older builds of Minefield so we could get a regression range? thanks.
(Reporter)

Comment 8

7 years ago
We tried different scenarios locally on Mac and Ubuntu but we didn't catch the failure.
I have executed the testrun_all.py script on qa-horus which is used by the daily script. No failures have been reported:

http://mozmill-archive.brasstacks.mozilla.com/#/endurance/report/5fdc9d8340b415e9a2e6a934ad3996c4

So not sure what's going on. We should check the results in the next couple of days. Anthony, if you could watch the daily test-run and just check for abnormalities on that box, that would be great.
(Reporter)

Comment 10

7 years ago
No failures in the results of endurance tests run on build 20110907030853 from Sept 7th
Same yesterday. So I would propose we close this bug as WFM and reopen if it reoccurs.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WORKSFORME

Comment 12

7 years ago
Anthony Hughes added a comment in Pivotal Tracker:   
   
WORKSFORME
(Reporter)

Comment 13

7 years ago
Same failure occured again for Mac 10.6 with build 20111019031031.

URL for the dashboard report where the failure occurs:
http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/7f11426eacdc891f09ffcc3839ecbdc8

TEST: /testTabView_SwitchTabs/test1.js::testSwitchTabs
ERROR: TimeoutError("TabView is still open.")@resource://mozmill/modules/utils.js:429 waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:467 
WHEN: 2011-10-19
FIRST: 2011-09-05
FREQ: 2
BRANCHES: nightly
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Have you been able to replicate this locally? It appears that it hasn't failed on Mac since the 19th October, but has failed a couple of times on other platforms. It might be worth trying to run this with more iterations to see if this is an issue of degradation over time, which we're only just hitting occasionally.
Mihaela, please investigate RE: comment 14. Thanks
This still appears to be an issue on Mac:

http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d3883a19e3
http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d38837172a
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-endurance]
Any progress here? It happened again today for 13.0.1 builds:

http://mozmill-ci.blargon7.com/#/endurance/report/e67171ea696231bb192f5656151cfd29
Dave, if we can't get this fixed we will have to disable the test. It's now happening for a long time with no progress made. There were a lot of failures for the latest release tests.
I agree, although I'm reluctant to skip if we can replicate it so frequently on our CI. Has anyone been able to replicate this locally? I can spend some time on it, but in the meantime we should get a patch to disable the test.
Created attachment 636653 [details] [diff] [review]
skip patch v1.0

Skip patch
Attachment #636653 - Flags: review?(hskupin)
the skip patch was added due to comment 20
Attachment #636653 - Flags: review?(dave.hunt)
Comment on attachment 636653 [details] [diff] [review]
skip patch v1.0

Taking this review.
Attachment #636653 - Flags: review?(hskupin)
Comment on attachment 636653 [details] [diff] [review]
skip patch v1.0

r+ with a minor nit addressed in the skip message and comment.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/793f4fdd1a93 (default)
Attachment #636653 - Flags: review?(dave.hunt) → review+
status-firefox-esr10: --- → ?
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
Whiteboard: [mozmill-test-failure][mozmill-endurance] → [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped]
Is ESR10 affected? Let's see this skipped in the next run before transplanting the skip.
Yes, esr10 is also affected:
http://mozmill-ci.blargon7.com/#/endurance/report/a7655636e327552d4750d1013c063a96

Please land the skip patch on all branches right away.
Okay, so we land skips across all affected branches immediately? Noted for future.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b3b55bfa89aa (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e66df6dcb970 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/caeba250e282 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/93b8b1cd1cb6 (mozilla-esr10)
I'm unable to replicate this on Mac OS X 10.7.4
Priority: -- → P2
status-firefox-esr10: affected → wontfix
status-firefox13: affected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
status-firefox16: affected → ---
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
Whiteboard: [mozmill-test-failure][mozmill-endurance][mozmill-test-skipped] → [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped] s=130204 u=failure c=tabview p=1

Updated

5 years ago
Assignee: nobody → andrei.eftimie
Status: REOPENED → ASSIGNED
We can't work on this bug this week given the other priorities.
Whiteboard: [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped] s=130204 u=failure c=tabview p=1 → [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped]

Updated

5 years ago
Whiteboard: [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped] → [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped] s=130211 u=failure c=tabview p=1

Updated

5 years ago
Assignee: andrei.eftimie → dpetrovici

Comment 30

5 years ago
I have run the tests on 21.0a1 build and I couldn't reproduce the issue at all (tried 400 runs over the past few days). Can we re-enable the test since it was skipped in 16.0a1?
Flags: needinfo?(dave.hunt)
Sounds good. Let's re-enable this test on default and watch for failures. I'm no longer able the backout the original patch, so please attach a backout patch for default.
Flags: needinfo?(dave.hunt)

Comment 32

5 years ago
Created attachment 713373 [details] [diff] [review]
patch v1.0 to re-enable test

Patch to re-enable test case. The report for MAC is: http://mozmill-crowd.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf30aae6
Attachment #713373 - Flags: review?(andreea.matei)
Comment on attachment 713373 [details] [diff] [review]
patch v1.0 to re-enable test

Review of attachment 713373 [details] [diff] [review]:
-----------------------------------------------------------------

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/88f34d464198 (default)
Attachment #713373 - Flags: review?(andreea.matei) → review+
status-firefox18: affected → disabled
status-firefox19: affected → disabled
status-firefox20: affected → disabled
status-firefox-esr17: affected → disabled

Comment 34

5 years ago
Reproducible in FF Nightly FR version on MAC 10.6.8:
http://mozmill-ci.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf764c42

I will try to reproduce the issue today.

Comment 35

5 years ago
I was not able to reproduce it locally or on the remote machine I have tried. I will continue on Monday to see if it reproduces remotely.
Attachment #713373 - Flags: checkin+
How often did it fail? Only once over the last couple of days?

Comment 37

5 years ago
Yes, it only reproduced on 2/15 once.
Priority: P2 → P3
Happened again on OS X 10.8.2, aurora, fr locale:
http://mozmill-ci.blargon7.com/#/endurance/report/f36358d058daf73ddbf7815016b23b1b

I looked over the code and I see in the library we have at close() method a XXX with bug 648294, but that was fixed in the mean time, so we don't need to update the '.toLocaleUpperCase()' part as well.
status-firefox22: --- → affected
Whiteboard: [mozmill-endurance][mozmill-test-failure][mozmill-test-skipped] s=130211 u=failure c=tabview p=1 → [mozmill-endurance][mozmill-test-failure] s=130211 u=failure c=tabview p=1

Comment 39

5 years ago
Actually this fails in waitForClosed() function where close() is not used. 

The close() function is not used by this test and I have logged bug no. 844787 for this issue.

Comment 40

5 years ago
Happened again today on Mac OS X 10.8.2 - Firefox 21.0a2:
http://mozmill-ci.blargon7.com/#/endurance/report/66aad0ebfa7a01580628b3af4cf49af4

Comment 41

5 years ago
And again today on Mac OS X 10.7.5 with Firefox 22.0a1 fr:
http://mozmill-ci.blargon7.com/#/endurance/report/2a6536e9db9f5f44ed48c5851004d418
(In reply to mario garbi from comment #41)
> And again today on Mac OS X 10.7.5 with Firefox 22.0a1 fr:
> http://mozmill-ci.blargon7.com/#/endurance/report/
> 2a6536e9db9f5f44ed48c5851004d418

Just to clarify, this was a test failure from Friday and not today.

Comment 45

5 years ago
Unable to reproduce with build from 03/09 on the machine it happened. And I am still unable to reproduce locally.

Comment 47

5 years ago
Happened again today on Mac OS X 10.7.5 (x86_64) with Firefox 22.0a2 en-US
http://mozmill-ci.blargon7.com/#/endurance/report/452ec32f8deec0960aea87aa06f25f09

Comment 48

5 years ago
Created attachment 770715 [details] [diff] [review]
patch v1.0 to re-enable the test for ESR17

This issue did not reproduce for two months. The test is unskipped on all versions except for ESR17. I think we should enable the test for this branch too.

The patch for default does not apply cleanly on ESR17 so I created a new one.

Reports:
MAC:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9bdfc7
Linux:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9bd5d0
Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9c63d8
Attachment #770715 - Flags: review?(andreea.matei)
Comment on attachment 770715 [details] [diff] [review]
patch v1.0 to re-enable the test for ESR17

Review of attachment 770715 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/423e8a5fe978 (esr17)
Hope it sticks now. Thanks!
Attachment #770715 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago5 years ago
status-firefox18: disabled → ---
status-firefox19: disabled → ---
status-firefox20: disabled → ---
status-firefox21: affected → ---
status-firefox22: affected → ---
status-firefox-esr17: disabled → ---
Resolution: --- → WORKSFORME

Comment 50

5 years ago
Happened again today on Windows NT 6.1.7601 with ESR17 en-US:
http://mozmill-ci.blargon7.com/#/endurance/report/593227b0561e6d84f2e52c8422215752

Updated

5 years ago
Status: RESOLVED → REOPENED
status-firefox-esr17: --- → affected
Resolution: WORKSFORME → ---
Status: REOPENED → ASSIGNED
(Assignee)

Updated

5 years ago
Assignee: daniela.p98911 → cosmin.malutan

Comment 51

5 years ago
We have 1 failure on Aurora, Linux, en-US:
http://mozmill-daily.blargon7.com/#/endurance/report/1039ea48a9d69a5a1cc4fd228c46dbfe
status-firefox26: --- → affected
Priority: P3 → P4
(Assignee)

Comment 52

5 years ago
We had one failure on Mac 10.6.8 (x64) with Nightly en-us
http://mozmill-daily.blargon7.com/#/endurance/report/6d6f6a58b02eeffc06eafa8bea616298

Comment 54

5 years ago
It seems this still fails, albeit rarely:
http://mozmill-daily.blargon7.com/#/endurance/report/56542c686e28115c2b3e8181cf1c9aee
status-firefox27: --- → affected
(Assignee)

Comment 55

5 years ago
Created attachment 8351368 [details] [diff] [review]
patch_v1.0

Hi, I couldn't reproduce this failure but under a closer look it's obvious what is wrong here.
We attach the listener after we trigger the action so in some cases the event might be already terminated at the time we attach the listener.
    
Reports:
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c126429866b
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c126428f6f7
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c12642d9272
Attachment #8351368 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #8351368 - Flags: review?(hskupin)
Attachment #8351368 - Flags: review?(andrei.eftimie)
Attachment #8351368 - Flags: review?(andreea.matei)
Attachment #8351368 - Flags: review?

Comment 56

4 years ago
Comment on attachment 8351368 [details] [diff] [review]
patch_v1.0

Review of attachment 8351368 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch!

I'm not sold on passing everything as a callback function.

Wondering if it wouldn't be cleaner to add another method `initClose` that would set up the listener.
So we'll have:
> initClose()
> [action for closing]
> waitForClosed()

I'll give feedback+ for finding the issue, but I'll leave the other reviews flag on to gather more info on how to proceed.

::: firefox/lib/tabview.js
@@ +144,5 @@
>      var self = { closed: false };
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // The command should be executed after we attache the listener

nit: attach
Attachment #8351368 - Flags: review?(andrei.eftimie) → feedback+
Comment on attachment 8351368 [details] [diff] [review]
patch_v1.0

Review of attachment 8351368 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabview.js
@@ +141,2 @@
>     */
> +  waitForClosed : function tabView_waitForClosed(aCallback) {

Please kill this method. It doesn't make sense to keep it any longer. Waiting for events should always happen right next to the code which triggered an action.
Attachment #8351368 - Flags: review?(hskupin)
Attachment #8351368 - Flags: review?(andreea.matei)
Attachment #8351368 - Flags: review-
(Assignee)

Comment 58

4 years ago
Created attachment 8358357 [details] [diff] [review]
patch_v2.0

(In reply to Henrik Skupin (:whimboo) from comment #57)
> > +  waitForClosed : function tabView_waitForClosed(aCallback) {
> Please kill this method. It doesn't make sense to keep it any longer.
> Waiting for events should always happen right next to the code which
> triggered an action.
Right, I removed the method and also the waitForOpen, for consistency.
 
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2b913f
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2c4833
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2c67cc

http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2cf2e4
http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2e1fe4
http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2e0fec
Attachment #8358357 - Flags: review?(hskupin)
Attachment #8358357 - Flags: review?(andrei.eftimie)
Attachment #8358357 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Blocks: 933163
(Assignee)

Comment 59

4 years ago
We need this fix as it causes other test to fail too. So I reprioritize the bug.
Priority: P4 → P2

Comment 60

4 years ago
Comment on attachment 8358357 [details] [diff] [review]
patch_v2.0

Review of attachment 8358357 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good.

Just a few minor issues.

::: firefox/lib/tabview.js
@@ +72,5 @@
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);
> +
> +    // Open the view via command key

nit: please move the whole block before the `cmdKey` assignment.
Right now we have this comment duplicated // via keyboard shortcut and via command key

@@ +126,5 @@
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // Close the view via command key
> +    this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +

nit: same thing here. please move the cmdKey assignment right before this call and preserve only 1 comment line

@@ +427,5 @@
> +      this._controller.window.removeEventListener("tabviewhidden", checkClosed, false);
> +    }
> +
> +    this._groupItemsObject = null;
> +    this._tabItemsObject = null;

I'm wondering: should we also reset `_tabView` and  `_tabViewDoc` as we do in `close()` method?
Opening a new tab does close the TabView, so we might as well. Please check if this is the case.
Attachment #8358357 - Flags: review?(hskupin)
Attachment #8358357 - Flags: review?(andrei.eftimie)
Attachment #8358357 - Flags: review?(andreea.matei)
Attachment #8358357 - Flags: review-
(Assignee)

Comment 61

4 years ago
Created attachment 8365033 [details] [diff] [review]
patch_v2.1

Thanks for review Andrei.

(In reply to Andrei Eftimie from comment #60)
> I'm wondering: should we also reset `_tabView` and  `_tabViewDoc` as we do
> in `close()` method?
> Opening a new tab does close the TabView, so we might as well. Please check
> if this is the case.
I didn't saw a difference, and I wouldn't change something else that this patch fixes.


http://mozmill-crowd.blargon7.com/#/endurance/report/f892d42cad846376757d785401442830
Attachment #713373 - Attachment is obsolete: true
Attachment #770715 - Attachment is obsolete: true
Attachment #8351368 - Attachment is obsolete: true
Attachment #8358357 - Attachment is obsolete: true
Attachment #8365033 - Flags: review?(hskupin)
Attachment #8365033 - Flags: review?(andrei.eftimie)

Comment 62

4 years ago
Comment on attachment 8365033 [details] [diff] [review]
patch_v2.1

Review of attachment 8365033 [details] [diff] [review]:
-----------------------------------------------------------------

> I didn't saw a difference, and I wouldn't change something else that this patch fixes.
Great, we won't change these.

Please make the changes I requested in the previous review so we can get this landed.

::: firefox/lib/tabview.js
@@ +70,5 @@
> +
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

You still didn't move this whole block before the cmdKey assignment.

That assignment and the keypress on that element should be one above the other, and should only have 1 comment (now you have them separated, and the same comment twice)

@@ +125,5 @@
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // Close the view via command key
> +    this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});

Same issue, this needs to be right below the cmdKey assignment.
Since the action needs to be _after_ we attach the event listener, we should move the cmdKey assignment here.
Attachment #8365033 - Flags: review?(hskupin)
Attachment #8365033 - Flags: review?(andrei.eftimie)
Attachment #8365033 - Flags: review-
(Assignee)

Comment 63

4 years ago
Created attachment 8366657 [details] [diff] [review]
patch_v2.2

I might uploaded the same patch last time.
Attachment #8365033 - Attachment is obsolete: true
Attachment #8366657 - Flags: review?(hskupin)
Attachment #8366657 - Flags: review?(andrei.eftimie)
Comment on attachment 8366657 [details] [diff] [review]
patch_v2.2

Review of attachment 8366657 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabview.js
@@ +67,5 @@
>    open : function tabView_open() {
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

Adding the event listener cannot happen outside of the try/finally. If there is an exception in between, we will leak this listener.

@@ +118,4 @@
>      // Add event listener to wait until the tabview has been closed
>      var self = { closed: false };
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

Same here as above. Lets get this moved into the try block.

@@ +398,5 @@
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

And here again.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +73,5 @@
> +      }, "TabView is still open.");
> +    }
> +    finally {
> +      controller.window.removeEventListener("tabviewhidden", checkChanged, false);
> +    }

Why do we have to use the listener in a test? All this should be done in the lib.
Attachment #8366657 - Flags: review?(hskupin)
Attachment #8366657 - Flags: review?(andrei.eftimie)
Attachment #8366657 - Flags: review-
(Assignee)

Comment 65

4 years ago
Comment on attachment 8366657 [details] [diff] [review]
patch_v2.2

Review of attachment 8366657 [details] [diff] [review]:
-----------------------------------------------------------------

I updated the library, please let me know how would you like me to approach this in test.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +73,5 @@
> +      }, "TabView is still open.");
> +    }
> +    finally {
> +      controller.window.removeEventListener("tabviewhidden", checkChanged, false);
> +    }

Then I should add a method in the library which will take a command as an parameter, please see comment 57.
In comment 57 I never talked that event listening should happen in the test. We have our abstraction in the libraries to help testers writing their tests. Why should we make it harder for them, and require them to copy and paste the similar code dozen of times?
(Assignee)

Comment 67

4 years ago
Thanks for input here, I will add a method change tab in library and use that in test.
(Assignee)

Comment 68

4 years ago
Created attachment 8368575 [details] [diff] [review]
patch_v3.0

I added an method selectTabAtIndex in library and use that in test.

Report:
http://mozmill-crowd.blargon7.com/#/endurance/report/0906e2c8b6f223d6588987bcd157ab4b
Attachment #8366657 - Attachment is obsolete: true
Attachment #8368575 - Flags: review?(hskupin)
Attachment #8368575 - Flags: review?(andrei.eftimie)
Attachment #8368575 - Flags: review?(andreea.matei)
Comment on attachment 8368575 [details] [diff] [review]
patch_v3.0

Review of attachment 8368575 [details] [diff] [review]:
-----------------------------------------------------------------

As we agreed on in the meeting, you should not request review from myself immediately. Only once you get a r+ from your team mates. Please keep this in mind. Thanks
Attachment #8368575 - Flags: review?(hskupin)
Comment on attachment 8368575 [details] [diff] [review]
patch_v3.0

Review of attachment 8368575 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabview.js
@@ +76,5 @@
> +      this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +
> +      assert.waitFor(function () {
> +        return self.opened == true;
> +      }, "TabView is not open.");

We can simply return self.opened right? Also, the messages should be positive, in this case "TabView has been opened". We'll know it's the expected result which has failed.

@@ +128,3 @@
>        assert.waitFor(function () {
>          return self.closed == true;
>        }, "TabView is still open.");

Same here.

@@ +397,5 @@
>      var type = aEventType || "menu";
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }

checkOpened

@@ +401,5 @@
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
> +
> +    try {
> +      // Close the tabview

Please remove this comment.

@@ +416,5 @@
> +      }
> +
> +      assert.waitFor(function () {
> +        return self.closed == true;
> +      }, "TabView is still open.");

These lines also need updating as mentioned above. The message should be about the new opened tab.

@@ +434,5 @@
> +   *        Index of tab we switch too
> +   * @param {string} aFilter
> +   *        Type of filter to apply
> +   *        (active, title)
> +   *        [optional - default: ""]

Just put this at the parameter name: {string} [aFilter=""]

@@ +445,5 @@
> +    var self = { hidden: false };
> +    function checkChanged() { self.hidden = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkChanged, false);
> +
> +

There are 2 blank lines here.

@@ +452,5 @@
> +
> +      // Wait for the selected tab to display
> +      assert.waitFor(function () {
> +        return self.hidden == true;
> +      }, "TabView is still open.");

Please update these too.
Attachment #8368575 - Flags: review?(andrei.eftimie)
Attachment #8368575 - Flags: review?(andreea.matei)
Attachment #8368575 - Flags: review-
(Assignee)

Comment 71

4 years ago
Created attachment 8369904 [details] [diff] [review]
patch_v3.1

Here is the updated patch.

(In reply to Andreea Matei [:AndreeaMatei] from comment #70)
> @@ +397,5 @@
> >      var type = aEventType || "menu";
> >  
> > +    // Add event listener to wait until the tabview has been closed
> > +    var self = { closed: false };
> > +    function checkClosed() { self.closed = true; }
> 
> checkOpened
Here we actually check if a tabview has been closed, as opening a new tab should close the active tabview.
Attachment #8369904 - Flags: review?(andrei.eftimie)
Attachment #8369904 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Attachment #8368575 - Attachment is obsolete: true
Comment on attachment 8369904 [details] [diff] [review]
patch_v3.1

Review of attachment 8369904 [details] [diff] [review]:
-----------------------------------------------------------------

Just a small update on my part. Please request from Henrik as well with that fixed.

::: firefox/lib/tabview.js
@@ +429,5 @@
> +  /**
> +   * Select a tab view at Index
> +   *
> +   * @param {number} aIndex
> +   *        Index of tab we switch too

nit: "Index of the tab we switch to"
Attachment #8369904 - Flags: review?(andrei.eftimie)
Attachment #8369904 - Flags: review?(andreea.matei)
Attachment #8369904 - Flags: review+
(Assignee)

Comment 73

4 years ago
Created attachment 8371333 [details] [diff] [review]
patch_v3.2

Thanks Andreea for review.
Attachment #8369904 - Attachment is obsolete: true
Attachment #8371333 - Flags: review?(hskupin)

Comment 74

4 years ago
Comment on attachment 8371333 [details] [diff] [review]
patch_v3.2

Review of attachment 8371333 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but there's a misleading JSdoc comment / default argument logic which I'd like to see addressed before we land this.

::: firefox/lib/tabview.js
@@ +432,5 @@
> +   * @param {number} aIndex
> +   *        Index of the tab we switch to
> +   * @param {string} [aFilter=""]
> +   *        Type of filter to apply
> +   *        (active, title)

This should be "active" and "group"
(title is for the "group" type not for the "tab" type)

@@ +435,5 @@
> +   *        Type of filter to apply
> +   *        (active, title)
> +   */
> +  selectTabAtIndex : function tabView_selectTabAtIndex(aIndex, aFilter) {
> +    var tab =  this.getTabs({filter: aFilter})[aIndex];

This is a bit misleading.
JSdoc says aFilter is optional and will default to empty string.
If you don't set it this will be sent forward as undefined to the getTabs method.
This will not filter at all (which is what we want if we don't set the aFilter argument).
So it will work fine.

But we should either update the JSdoc (to cater for undefined) or the code here to make an empty string as the JSdoc suggests.
Comment on attachment 8371333 [details] [diff] [review]
patch_v3.2

Review of attachment 8371333 [details] [diff] [review]:
-----------------------------------------------------------------

Additional to Andrei's comments you will get mine. Please see inline.

::: firefox/lib/tabview.js
@@ +67,5 @@
>    open : function tabView_open() {
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

We can omit the false parameter, which is not necessary at this point.

@@ +82,5 @@
> +      this._groupItemsObject = this._tabViewObject._window.GroupItems;
> +      this._tabItemsObject = this._tabViewObject._window.TabItems;
> +    }
> +    finally {
> +      this._controller.window.removeEventListener("tabviewshown", checkOpened, false);

Same here.

@@ +382,5 @@
>      });
>    },
>  
>    /**
> +   * Open a new tab, this will determine the current tab view to close

Don't mix up the title and descriptions. The second part of this line should get its own line after a blank one.

@@ +390,5 @@
>     *     <dt>menu</dt>
>     *     <dd>The main menu is used</dd>
>     *     <dt>shortcut</dt>
>     *     <dd>The keyboard shortcut is used</dd>
>     *   </dl>

Can you please update the doc for the sub property usage?

@@ +398,5 @@
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

We should be able to remove false here.

@@ +423,4 @@
>      }
>  
> +    this._groupItemsObject = null;
> +    this._tabItemsObject = null;

This is mostly duplicated code from the close() method. That's one reason why I think having a callback for the initial action instead of a type string would be better. Can you please file a follow-up bug? This would require a bit amount of work so not sure when we can tackle that. For now it is fine.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +54,5 @@
>      var allTabs = activeTabView.getTabs();
>  
>      // Select the tab which is NOT active
>      if (activeTab.getNode() === allTabs[0].getNode()) {
> +      activeTabView.selectTabAtIndex(1, "active");

Something is wrong here. Whether its the comment or the filter value.
Attachment #8371333 - Flags: review?(hskupin) → review-
(Assignee)

Comment 76

4 years ago
Created attachment 8379696 [details] [diff] [review]
patch_v3.3

I updated the patch, thanks for review.

(In reply to Henrik Skupin (:whimboo) [away 02/24 - 02/28] from comment #75)
> @@ +390,5 @@
> >     *     <dt>menu</dt>
> >     *     <dd>The main menu is used</dd>
> >     *     <dt>shortcut</dt>
> >     *     <dd>The keyboard shortcut is used</dd>
> >     *   </dl>
> 
> Can you please update the doc for the sub property usage?
Those are options, and from what I saw in other tests, this is the only way we document them, what do you have in mind for updating this? I can file a new bug for updating all cases. 

Thanks
Attachment #8371333 - Attachment is obsolete: true
Attachment #8379696 - Flags: review?(andrei.eftimie)
Attachment #8379696 - Flags: review?(andreea.matei)
You should look at newer modules like the assertions.js one:
http://hg.mozilla.org/qa/mozmill-tests/file/4e10acdf95ca/lib/assertions.js#l576
(Assignee)

Comment 78

4 years ago
Well there is an object sent as parameter with multiple subtypes, here we only have a string that can take two different values. Since this is not something I touched I would't change it in this patch.
Oh, right. Ok, so let it as it is.

Comment 80

4 years ago
Comment on attachment 8379696 [details] [diff] [review]
patch_v3.3

Review of attachment 8379696 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Checked in: http://hg.mozilla.org/qa/mozmill-tests/rev/905bc0ba7a1e (default)
Attachment #8379696 - Flags: review?(andrei.eftimie)
Attachment #8379696 - Flags: review?(andreea.matei)
Attachment #8379696 - Flags: review+
Attachment #8379696 - Flags: checkin+

Updated

4 years ago
status-firefox26: affected → wontfix
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → fixed
status-firefox-esr17: affected → wontfix
status-firefox-esr24: --- → affected

Comment 81

4 years ago
Cosmin, please check backports, we'll want to land this across branches.
(In reply to Henrik Skupin (:whimboo) from comment #75)
> This is mostly duplicated code from the close() method. That's one reason
> why I think having a callback for the initial action instead of a type
> string would be better. Can you please file a follow-up bug? This would
> require a bit amount of work so not sure when we can tackle that. For now it
> is fine.

Cosmin, has this been filed as a bug yet?
Flags: needinfo?(cosmin.malutan)
(Assignee)

Comment 83

4 years ago
Created attachment 8385287 [details] [diff] [review]
patch_v3.3[release]

I just filled bug 979223 for that.
The patch applies cleanly on aurora and beta, here is the patch for release, and I will return with the patch for esr24.
Thanks
Attachment #8385287 - Flags: review?(andrei.eftimie)
Flags: needinfo?(cosmin.malutan)

Comment 85

4 years ago
Comment on attachment 8385287 [details] [diff] [review]
patch_v3.3[release]

Review of attachment 8385287 [details] [diff] [review]:
-----------------------------------------------------------------

Just a small nit.

::: firefox/lib/tabview.js
@@ +446,5 @@
> +    function checkChanged() { self.hidden = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkChanged, false);
> +
> +    try {
> +     tab.click();

nit: this is not properly indented
This is good in the original patch.
Attachment #8385287 - Flags: review?(andrei.eftimie) → review-

Comment 86

4 years ago
Comment on attachment 8385304 [details] [diff] [review]
patch_v3.3[esr24]

Review of attachment 8385304 [details] [diff] [review]:
-----------------------------------------------------------------

You ran reports but didn't properly checked the results. This patch produces an error.

::: firefox/lib/tabview.js
@@ +131,1 @@
>      } finally {

nit: the finally was moved to a newline in the original patch

@@ +440,5 @@
> +    var tab =  this.getTabs({filter: aFilter})[aIndex];
> +    assert.ok(tab.exists(), "Tab has been found.");
> +
> +    // Add event listener to wait until the tabview has been changed
> +   var self = { hidden: false };

nit: indentation

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +59,1 @@
>      } else {

nit: the else was moved to a newline in the original patch

@@ +61,4 @@
>      }
>  
>      // Wait for the selected tab to display
>      activeTabView.waitForClosed();

You forgot to remove this call.
This has been removed in the original patch.

This fails since this function doesn't exist anymore.
Attachment #8385304 - Flags: review?(andrei.eftimie) → review-

Comment 87

4 years ago
Landed backports for:
http://hg.mozilla.org/qa/mozmill-tests/rev/f62b007b19ee (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ca037aa84c8f (mozilla-beta)
status-firefox28: affected → fixed
status-firefox29: affected → fixed
(Assignee)

Comment 88

4 years ago
Created attachment 8385946 [details] [diff] [review]
patch_v3.3[release]

Fixed the nit.
Attachment #8385287 - Attachment is obsolete: true
Attachment #8385946 - Flags: review?(andrei.eftimie)
(Assignee)

Comment 89

4 years ago
Created attachment 8385952 [details] [diff] [review]
patch_v3.3[esr24]

I fixed the backport patch for esr24
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda952a505
Attachment #8385304 - Attachment is obsolete: true
Attachment #8385952 - Flags: review?(andrei.eftimie)
(Assignee)

Comment 90

4 years ago
Created attachment 8385963 [details] [diff] [review]
patch_v3.4[esr24]

I uploaded the wrong patch, here is the fixed one
Attachment #8385952 - Attachment is obsolete: true
Attachment #8385952 - Flags: review?(andrei.eftimie)
Attachment #8385963 - Flags: review?(andrei.eftimie)

Comment 91

4 years ago
Comment on attachment 8385952 [details] [diff] [review]
patch_v3.3[esr24]

Review of attachment 8385952 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Landed both:
http://hg.mozilla.org/qa/mozmill-tests/rev/d7418b387a3f (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/0c6f2c094a2a (mozilla-esr24)
Attachment #8385952 - Flags: review+
Attachment #8385952 - Flags: checkin+

Updated

4 years ago
Attachment #8385963 - Flags: review?(andrei.eftimie)
Attachment #8385963 - Flags: review+
Attachment #8385963 - Flags: checkin+

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago4 years ago
status-firefox27: affected → fixed
status-firefox-esr24: affected → fixed
Resolution: --- → FIXED

Updated

4 years ago
Attachment #8385946 - Flags: review?(andrei.eftimie)
Attachment #8385946 - Flags: review+
Attachment #8385946 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.