710245 - Mozmill endurance test for Open and Close Panorama with lots of tabs

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.1

Mozmill endurance test for Open and Close Panorama with lots of tabs

Comment 1

[Alex Lakatos[:AlexLakatos]]

Comment on attachment 581293 [details] [diff] [review]
patch v0.1

I would appreciate some feedback here. Is the test doing what we want?
Anyone, please feel free to jump in with feedback.

Comment 2

[u279076]

Comment on attachment 581293 [details] [diff] [review]
patch v0.1

>+++ b/tests/endurance/testPanorama_openPanorama/test1.js

To be in line with functional tests, call this testTabView_openTabViewWithTabs

>+  //Open lots of tabs

Open 10 tabs

>+  for(var i=1; i<10; i++) {

Start at 0 

>+  };

Remove ;

>+  controller.open(LOCAL_TEST_PAGE + enduranceManager.entities);
>+  controller.waitForPageLoad();

Separate this code block from the for loop with a newline.

>+/**
>+ * Test opening Panorama with lots of tabs
>+ **/

Replace "lots" with "multiple"

>+function testOpenPanorama() {

testOpenTabViewWithTabs() {

NOTE: Anywhere you use the word Panorama, replace with TabView


>+  enduranceManager.run(function () {
>+    // Open Panorama
>+    enduranceManager.addCheckpoint("Open Panorama");
>+    activeTabView.open();
>+    enduranceManager.addCheckpoint("Panorama has been opened");
>+    activeTabView.close();
>+    enduranceManager.addCheckpoint("Panorama has been closed");
>+  });

Separate each of these checkpoints with a newline

I'm wondering if this needs a delayed checkpoint to wait for loading the view.

Comment 3

[u279076]

One more thing I just thought of -- can you update the for loop to use the number of entities?

Comment 4

[Alex Lakatos[:AlexLakatos]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #2)
> Comment on attachment 581293 [details] [diff] [review]
> patch v0.1
> >+  for(var i=1; i<10; i++) {
> 
> Start at 0 
If I start at 0, i'll have to go until i<9. Because there is a new tab already opened when the loop starts, i'm opening the last tab outside the loop. Is that ok?

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #3)
> One more thing I just thought of -- can you update the for loop to use the
> number of entities?
I used 10 for testing with the "mozmill" command, because entities=1 there. But the final test will use enduranceManager.entities

Comment 5

[u279076]

I think that makes sense, thanks.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 581293 [details] [diff] [review]
patch v0.1

>diff --git a/tests/endurance/testPanorama_openPanorama/test1.js b/tests/endurance/testPanorama_openPanorama/test1.js

To be consistent with other tests, can we name this testTabView_OpenAndCloseTabView

>+  //Open lots of tabs

Simply 'Open test pages in tabs' because there could be 1 tab.

>+  for(var i=1; i<10; i++) {
>+    controller.open(LOCAL_TEST_PAGE + i);
>+    controller.waitForPageLoad();
>+    tabBrowser.openTab();
>+  };
>+  controller.open(LOCAL_TEST_PAGE + enduranceManager.entities);
>+  controller.waitForPageLoad();
>+}

We only need to open a new tab if entities is > 1.

for (var i = 0; i < enduranceManager.currentEntity; i++) {
  if (i > 1) {
    tabBrowser.openTab();
  }
  controller.open(LOCAL_TEST_PAGE + enduranceManager.currentEntity);
  controller.waitForPageLoad();
});

>+function testOpenPanorama() {
>+  enduranceManager.run(function () {
>+    // Open Panorama

The following checkpoint covers this comment, so we probably don't need it.

>+    enduranceManager.addCheckpoint("Open Panorama");
>+    activeTabView.open();
>+    enduranceManager.addCheckpoint("Panorama has been opened");
>+    activeTabView.close();
>+    enduranceManager.addCheckpoint("Panorama has been closed");

I'd like to standardise on naming (file, test method, comments, and checkpoints). Looking at the previous two panorama tests we've used 'Panorama', 'Tab View', and 'Tab Groups View'. Anthony: could you give your thoughts on what we should use across all such tests?

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sorry, I've just seen the earlier comments from Anthony. I think we align on most points. I guess that's the problem with asking for feedback from multiple people at the same time. :)

Comment 8

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Sort of combined both feedbacks

Comment 9

[u279076]

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

>+  //Open test pages in tabs
>+  for(var i=1; i<enduranceManager.entities; i++) {
>+    controller.open(LOCAL_TEST_PAGE + i);
>+    controller.waitForPageLoad();
>+    if (i > 1) {
>+      tabBrowser.openTab();
>+    }
>+  }

I wonder if this could be made simpler with a while()

Comment 10

[Alex Lakatos[:AlexLakatos]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #9)
> Comment on attachment 588075 [details] [diff] [review]
> patch v1.0
> 
> 
> I wonder if this could be made simpler with a while()
I don't see a way to simplify this with a while(), just a way to add an extra line incrementing i. What did you have in mind, Anthony?

Comment 11

[u279076]

Yeah, you are right; I guess it's fine as it is. Can you please retest this patch since it's been a couple of weeks. Assuming it passes that, send it over to Dave Hunt for final review. I'll change my r- to an r+.

Comment 12

[Alex Lakatos[:AlexLakatos]]

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

Retested the patch and it still works. Over to Dave for r?

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

>new file mode 100644
>--- /dev/null
>+++ b/tests/endurance/testTabView_openTabViewWithTabs/test1.js

Please rename directory to testTabView_OpenTabViewWithTabs (capitalized for consistency).

>+  //Open test pages in tabs
>+  for(var i=1; i<enduranceManager.entities; i++) {

Nit: Can we have a space either side of the = and <

>+    controller.open(LOCAL_TEST_PAGE + i);
>+    controller.waitForPageLoad();
>+    if (i > 1) {
>+      tabBrowser.openTab();
>+    }
>+  }
>+
>+  controller.open(LOCAL_TEST_PAGE + enduranceManager.entities);
>+  controller.waitForPageLoad();

This is duplication. Please modify the previous loop to start at i = 0, open a new tab if i > 0, and then load the test page. If you like you can use i+1 for the tab number.

Otherwise, this looks good.

Comment 14

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.1

Addressed nits.

Comment 15

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

+  //Open test pages in tabs
+  for(var i = 0; i < enduranceManager.entities; i++) {
+    controller.open(LOCAL_TEST_PAGE + i);
+    controller.waitForPageLoad();
+    if ((i > 0) || (i == enduranceManager.entities - 1)) {
+      tabBrowser.openTab();
+    }
+  }
+}

You need to open the new tab before you open the test page, otherwise the initial tab content will be loaded twice, and there will be a tab with no content after the setup. Also, the if statement should simply be if (i > 0).

If you want the first test page URL to end tab=0 then this code is fine, but if you want tab=1 then you'll need to use LOCAL_TEST_PAGE + (i + 1).

Comment 16

[Alex Lakatos[:AlexLakatos]]

Attachment: test v1.2 [landed]

Switched it at the top.

Comment 17

[u279076]

Comment on attachment 592129 [details] [diff] [review]
test v1.2 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/fff49c3b8e47 (default)

Comment 18

[u279076]

Alex, please verify this passes tomorrow's testrun so I can port to other branches.

Comment 19

[Alex Lakatos[:AlexLakatos]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #18)
> Alex, please verify this passes tomorrow's testrun so I can port to other
> branches.

Passed http://mozmill-release.blargon7.com/#/endurance/report/55d601cc2aabfac28f59060a8405986b

Comment 20

[u279076]

Comment on attachment 592129 [details] [diff] [review]
test v1.2 [landed]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/d7a28b539480 (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/3fd2261a9f8b (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/890f7184ee84 (mozilla-release)