629063 - Mozmill test for litmus smoketest: Panorama opens and initial group is viewable

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Litmus ID:
https://litmus.mozilla.org/show_test.cgi?id=12645

Comment 1

[Aaron Train [:aaronmt]]

For what it's worth, the above Litmus test is disabled because the feature doesn't exist anymore (was a video introduction).

Comment 2

[Al Billings [:abillings - ex-MoCo]]

I was turning it into a smoketest. I don't think it was disabled the other day. It will be a "go into panorama, check list of tabs, and exit" test.

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Attachment: Mozmill test for Litmus test case #12645

This is the proposed testcase for litmus test case #12645.

It does have a hardcoded 200 ms sleep in it that was necessary for panorama to populate. Once tabview.js has been updated to detect that panorama is completely open and populated, we can remove the sleep.

Comment 4

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

Comment on attachment 523719 [details] [diff] [review]
Mozmill test for Litmus test case #12645

Anthony is the first reviewer.

Comment 5

[u279076]

Comment on attachment 523719 [details] [diff] [review]
Mozmill test for Litmus test case #12645

>+function setupModule(module) {
>+  controller = mozmill.getBrowserController();
>+  tabBrowser = new tabs.tabBrowser(controller);
>+  activeTabView = new tabview.tabView(controller);

Can you insert a newline here?

>+  activeTabView.reset();
>+  tabBrowser.closeAllTabs();
>+}

>+var testBasicTabView = function() {
>+  /* This test opens up a set of tabs in Firefox, grabs their titles,
>+   * shifts into panorama, and verifies that that the number of tabs opened
>+   * and their titles matches the tabs from before panorama opening.
>+   */

Can you simplify this comment and move it to just before the function signature? Something like...

/**
 * Tests the initial opening of the Tab Groups view
 */
function testBasicTabView() {
 
>+    // Get the text of the tab's actual title after it is opened
>+    var myTabTitle = tabBrowser.getTab().getNode().label;
>+    
>+    // Push the tab's title to the tabTitles array
>+    tabTitles.push(myTabTitle);

Can you use the variable name "tabLabel" instead?

>+  // Open Tabview
>+  activeTabView.open();
>+  controller.sleep(200); // Needed or the panorama tabs don't populate in time

Does this test fail without the sleep? Does changing the following assert() to a waitFor() not take this into consideration? If not, we should probably put something into the shared module to account for this.

>+   controller.assert(function () {
>+    return activeGroupTabs.length === LOCAL_TEST_PAGES.length;
>+  }, "Expected tab count to be" + LOCAL_TEST_PAGES.length + " and got " + activeGroupTabs.length);

You should use the same model as your previous assert message for all asserts:
"Something has happened - got currentState, expected expectedState"
  
>+   // Set up array to hold tabview tab/thumbnail titles
>+   var panoramaTabTitles = new Array();
>+   
>+   // Populate the new array with titles of tabview tabs.
>+   activeGroupTabs.forEach(function (tabItem) {  
>+    var individualTabTitleBox = activeTabView.getTabTitleBox({tab: tabItem});
>+    
>+    // Get the actual title (value) within the individualTabTitleBox
>+    var individualTabTitle = individualTabTitleBox.getNode().textContent;
>+    
>+    // Push the tab title into the array
>+    panoramaTabTitles.push(individualTabTitle);
>+   });
>+   
>+    // Compare the values in each of the two arrays of titles to check match
>+  for (var i = 0; i < tabTitles.length; i++) {
>+    controller.assert(function() {
>+      return tabTitles[i] === panoramaTabTitles[i] }
>+      , "Failure: first title is '" + tabTitles[i] + "' and second title is '" + panoramaTabTitles[i] + "'");
>+  };

Can you simplify this so that everything happens within the for loop? (eliminate the need for the panoramaTabTitles[] array)

>+/**
>+ * Map test functions to litmus tests
>+ */
>+// testTabGroupCreation.meta = {litmusids : [12645]};

You don't need this mapping anymore.

>\ No newline at end of file

Please make sure to add a newline to the end of the file.

Thanks

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Attachment: Mozmill test for Litmus test case #17092.

Incorporated Anthony's feedback (mostly). I kept my array as I found it simpler to do it that way versus trying to overload the later looping.

Without the sleep() and using waitFor(), the case will not pass. The tabview list of thumbnails is a rendered set of divs with odd properties and they don't popular fast enough for the labels to be found without the very brief pause that I have (unless someone can think of another way to wait for those labels).

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Oh, and I tested this against both 4.0.1 and the nightly Aurora build and it works with both. I updated the listed testcase to be the Aurora branch case.

Comment 8

[u279076]

Comment on attachment 532087 [details] [diff] [review]
Mozmill test for Litmus test case #17092.

>+/**
>+ * Tests the initial opening of the Tab Groups view
>+ */
>+var testBasicTabView = function() {

nit: please make this a named function
function functionName() {

>+  // Open Tabview
>+  activeTabView.open();
>+  controller.sleep(200); // Needed or the panorama tabs don't populate in time

> Without the sleep() and using waitFor(), the case will not pass. The tabview list of thumbnails 
> is a rendered set of divs with odd properties and they don't popular fast enough for the labels 
> to be found without the very brief pause that I have (unless someone can think of another way to 
> wait for those labels).

Henrik, can anything be incorporated in the shared module so that we don't need this sleep? Any advice on what we can try?
  
>+   controller.assert(function () {
>+    return activeGroupTabs.length === LOCAL_TEST_PAGES.length;
>+  }, "Number of tags do not match - got: " + activeGroupTabs.length +
>+    ", expected: " + LOCAL_TEST_PAGES.length);

nit: Please reword this to a positive message.
"Number of tags match: <got>, <expected>"

>+    // Compare the values in each of the two arrays of titles to check match
>+  for (var i = 0; i < tabTitles.length; i++) {
>+    controller.assert(function() {
>+      return tabTitles[i] === panoramaTabTitles[i] }
>+      , "Failure: first title is '" + tabTitles[i] + "' and second title is '" + panoramaTabTitles[i] + "'");
>+  };

nit: Please reword this to a positive message.
"Tab titles match Panorama titles: <got>, <expected>"

Thanks Al, almost there. I have a feeling we will probably check in your patch with the sleep(200) once the nits are addressed. If anything needs to be added to the shared module, we can address that in another bug. Please wait for word from Henrik before proceeding along that assumption. Thanks.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #8)
> >+   controller.assert(function () {
> >+    return activeGroupTabs.length === LOCAL_TEST_PAGES.length;
> >+  }, "Number of tags do not match - got: " + activeGroupTabs.length +
> >+    ", expected: " + LOCAL_TEST_PAGES.length);
> 
> nit: Please reword this to a positive message.
> "Number of tags match: <got>, <expected>"
> 
> >+    // Compare the values in each of the two arrays of titles to check match
> >+  for (var i = 0; i < tabTitles.length; i++) {
> >+    controller.assert(function() {
> >+      return tabTitles[i] === panoramaTabTitles[i] }
> >+      , "Failure: first title is '" + tabTitles[i] + "' and second title is '" + panoramaTabTitles[i] + "'");
> >+  };
> 
> nit: Please reword this to a positive message.
> "Tab titles match Panorama titles: <got>, <expected>"

 You realize that you gave me different instructions in the previous code review on this, right? :-)

Comment 10

[u279076]

(In reply to comment #9)
>  You realize that you gave me different instructions in the previous code
> review on this, right? :-)

Yeah, sorry I wasn't clear in my original review.

Comment 11

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

(In reply to comment #8)
> > Without the sleep() and using waitFor(), the case will not pass. The tabview list of thumbnails 
> > is a rendered set of divs with odd properties and they don't popular fast enough for the labels 
> > to be found without the very brief pause that I have (unless someone can think of another way to 
> > wait for those labels).
> 
> Henrik, can anything be incorporated in the shared module so that we don't
> need this sleep? Any advice on what we can try?

You should ask one of the Panorama developers if there is another event available we can listen for. If that's not the case you should really use waitFor instead of assert.

Comment 12

[u279076]

(In reply to comment #11)
> The tabview list of thumbnails is a rendered set of divs with odd properties 
> and they don't popular fast enough for the labels to be found without the 
> very brief pause that I have (unless someone can think of another way to 
> wait for those labels).

CCing Ian Gilman on this bug. Ian, is there an event we can be listening for with regards to the tab view being completely finished loading?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #11)
> You should ask one of the Panorama developers if there is another event
> available we can listen for. If that's not the case you should really use
> waitFor instead of assert.
I tried using a waitFor and it didn't have an effect (well, the case started failing but not the desired effect).

Comment 14

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

Well, I can't help out with the waitFor issue because I don't know which conditions you have checked.

Comment 15

[u279076]

(In reply to comment #14)
> Well, I can't help out with the waitFor issue because I don't know which
> conditions you have checked.

Hey guys, let's not go back and forth like this. Let's just wait for Ian's feedback before proceeding.

Comment 16

[Ian Gilman [:iangilman]]

I don't think we have any Panorama-specific event like that (cc-ing Tim in case he knows different). For our tests, we use a routine we wrote called afterAllTabsLoaded(): 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/tabview/head.js#95

Maybe you could use something similar? There might be other useful routines in that file as well.

Comment 17

[u279076]

(In reply to comment #16)
> I don't think we have any Panorama-specific event like that (cc-ing Tim in
> case he knows different). For our tests, we use a routine we wrote called
> afterAllTabsLoaded(): 
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> tabview/head.js#95

Henrik, it seems like there is something we can learn from this code and implement in the shared module.

In Henrik's absence, ccing Geo on this bug. Geo, what do you think?

Comment 18

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Looks like a good start. I'm not quite sure how to integrate it into the existing shared module (literally, as I haven't analyzed its code). 

My guess is we're talking about something considerably harder for me to do while Henrik's gone than if he's back, so some sense of the urgency would be good.

(and for the record, I'd have checked this in with the sleep() and then filed a bug for the waitFor. Standards are good and everything, but "no obvious way" would include this situation when allowing for a sleep call IMO).

Comment 19

[u279076]

(In reply to comment #18)
> (and for the record, I'd have checked this in with the sleep() and then
> filed a bug for the waitFor. Standards are good and everything, but "no
> obvious way" would include this situation when allowing for a sleep call
> IMO).

I'm in favour of this approach. Al, can you resubmit your patch for review with the sleep() back in place? Once checked in we can file a follow-up bug for refactoring it to a waitFor() utilizing the code in comment 17.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Sure. I expect I'll get to this tomorrow given the 1.9.2 related tasks (and others) queued for today.

Comment 21

[u279076]

(In reply to comment #20)
> Sure. I expect I'll get to this tomorrow given the 1.9.2 related tasks (and
> others) queued for today.

Perfect, thanks.

Comment 22

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

(In reply to comment #17)
> > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> > tabview/head.js#95
> 
> Henrik, it seems like there is something we can learn from this code and
> implement in the shared module.

Please file a new bug and I can take care of it in the next days.

(In reply to comment #20)
> Sure. I expect I'll get to this tomorrow given the 1.9.2 related tasks (and
> others) queued for today.

Any update on it Al? Nearly two weeks have been passed.

Comment 23

[u279076]

(In reply to comment #22)
> (In reply to comment #17)
> > > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/
> > > tabview/head.js#95
> > 
> > Henrik, it seems like there is something we can learn from this code and
> > implement in the shared module.
> 
> Please file a new bug and I can take care of it in the next days.

See bug 666299

Comment 24

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

Bug 836758 will remove Panorama from Firefox soon and make it available as add-on. That means no new tests are necessary. Closing as WONTFIX.