711129 - Mozmill Endurance test for Open & Close a popup window

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Alex Lakatos[:AlexLakatos]]

Mozmill Endurance test for Open & Close a popup window

*Setup Module
  **Open page
*Main Test Checkpoints
  **Trigger X popups
  **Close X popups
*Teardown Module
  **Force close tabs

Comment 1

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.1

This is a wip patch. I would appreciate some feedback. Thanks.

Comment 2

[u279076]

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

>+/**
>+* Test opening popups and closing popups
>+*/
>+function testOpenMultiplePopups() {
>+  controller.open(LOCAL_TEST_PAGE + enduranceManager.entities);
>+  controller.waitForPageLoad();
>+
>+  enduranceManager.run(function () {
>+    for each (window in mozmill.utils.getWindows("navigator:browser")) {
>+      if (!window.toolbar.visible)
>+        window.close();
>+    }
>+  });
>+}

I think it looks good. One thing I think we should do is have an assertion between waitForPageLoad() and enduranceManager.run(), just to make sure that the expected number of popups have been loaded and waitForPageLoad() is not returning prematurely.

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Added the assert.

Comment 4

[u279076]

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

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

testPopups_OpenAndClose/

>+/**
>+* Test opening popups and closing popups
>+*/

Test opening and closing popups

>+function testOpenMultiplePopups() {

testOpenAndClosePopups()

>+  var widows = mozmill.utils.getWindows("navigator:browser");

s/widows/windows

>+  var openedWindows = widows.length; 
>+  assert.equal(openedWindows - 1, enduranceManager.entities,
>+               enduranceManager.entities + " popups have been opened")

I think it's simple enough to just use windows.length; no need to assign it to it's own variable.

NOTE: make sure you fill out the requestee field when asking for review. I almost missed this one.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.1

Fixed naming

Comment 6

[u279076]

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

Looks good to me. Dave can you provide some quick feedback before I check it in?

Comment 7

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

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

>+  prefs.openPreferencesDialog(controller, prefDialogAllowPopupsCallback);

Can we set this preference directly rather than using the preferences pane?

>+  prefs.openPreferencesDialog(controller, prefDialogBlockPopupsCallback);

As above, can we set this directly instead?

Otherwise, this looks great.

Comment 8

[Andreea Matei [:AndreeaMatei]]

I have a question here, the steps are not very clear. I assume we want to have iterations that open multiple popups and close them? Cause as it is now, only the closing part it's under these iterations.

Comment 9

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #8)
> I have a question here, the steps are not very clear. I assume we want to
> have iterations that open multiple popups and close them? Cause as it is
> now, only the closing part it's under these iterations.

That's not true, the count parameter in the URL should control the number of popups opened.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Yes, but as it is now, we open all popups = number of entities (lets say 10) and in the first iteration (out of 10), we close them all. The others iterations then have no point, the number of windows for them is 1 (the main window).
This is why I'm in doubt, shouldn't we wrap the whole function in enduranceManager.run so at each iteration we would open the page with number of entities popups and close them?

Comment 11

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #10)
> Yes, but as it is now, we open all popups = number of entities (lets say 10)
> and in the first iteration (out of 10), we close them all. The others
> iterations then have no point, the number of windows for them is 1 (the main
> window).
> This is why I'm in doubt, shouldn't we wrap the whole function in
> enduranceManager.run so at each iteration we would open the page with number
> of entities popups and close them?

Yes, that makes sense to me. Sorry, I misunderstood your original question.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated test, here you can find the report:
http://mozmill-crowd.blargon7.com/#/endurance/report/25ad365ca7bcf4905e9b700b4fc45fe6

Comment 13

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

Comment on attachment 732323 [details] [diff] [review]
patch v2

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

Looks good, just an assert and a couple of checkpoints to be added.

::: tests/endurance/testPopups_OpenAndClose/test1.js
@@ +43,5 @@
> +
> +    var windows = mozmill.utils.getWindows("navigator:browser");
> +    assert.equal(windows.length - 1, enduranceManager.entities,
> +                 enduranceManager.entities + " popups have been opened");
> +

Could you add a checkpoint here with the label: enduranceManager.entities + " popup(s) opened"

@@ +47,5 @@
> +
> +    windows.forEach(function (window) {
> +      if (!window.toolbar.visible)
> +        window.close();
> +    });

Could you add an assert here to check all popups have been closed. Also, add a checkpoint with the label: enduranceManager.entities + " popup(s) closed"

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2.1

Updated as requested.

Comment 15

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

Comment on attachment 733324 [details] [diff] [review]
patch v2.1

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

::: tests/endurance/testPopups_OpenAndClose/test1.js
@@ +28,5 @@
> +function teardownModule(aModule) {
> +  prefs.preferences.clearUserPref(PREF_MAX_POPUPS);
> +  prefs.preferences.clearUserPref(PREF_DISABLE_POPUPS);
> +
> +  aModule.tabBrowser.closeAllTabs();

So what happens when an assert fails in the test? We will not close those remaining popups. `closeAllTabs` isn't necessary given that we do not open tabs in this test but we should close all other windows except the one for the controller.

@@ +42,5 @@
> +
> +    var windows = mozmill.utils.getWindows("navigator:browser");
> +    assert.equal(windows.length - 1, enduranceManager.entities,
> +                 enduranceManager.entities + " popups have been opened");
> +    enduranceManager.addCheckpoint(enduranceManager.entities + " popup(s) opened");

I would put this before the assert.equal() call to have the measurement closer the actual task it measures.

@@ +46,5 @@
> +    enduranceManager.addCheckpoint(enduranceManager.entities + " popup(s) opened");
> +
> +    windows.forEach(function (aWindow) {
> +      if (!aWindow.toolbar.visible)
> +        aWindow.close();

nit: please add brackets.

Comment 16

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Since we decided in bug 800872 to not have a new library anymore, I created a helper method in the test to close popups windows which we use in the test itself and in teardown to make sure all are closed.

http://mozmill-crowd.blargon7.com/#/endurance/report/16dc29d7a411d565992bbd7dc85020af

Comment 17

[Andrei Eftimie]

Comment on attachment 8394754 [details] [diff] [review]
patch v3

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

In general this looks good.

There's only one thing bothering me, see inline.

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +31,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function teardownModule(aModule) {
> +  closePopupWindows();

This doesn't really work well if something unexpected happens during the test.
If the opened popups doesn't exactly match the entities, this function fails.

We should either have another method that closes all popups reliably or pass in a force argument.
I would rather see another method that doesn't include any checkpoints.

@@ +80,5 @@
> +        Services.obs.addObserver(observer, DOM_WINDOW_CLOSED, false);
> +        aWindow.close();
> +        assert.waitFor(function () {
> +          windows = mozmill.utils.getWindows("navigator:browser");
> +          return (windowClosed && (windows.length === counter));

You could remove both brace pairs here, but it's fine either-way.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.1

Yes, you were right about the entities, I left the general number of windows as the couter and we're closing all the windows without a toolbar.

Comment 19

[Andrei Eftimie]

Comment on attachment 8397841 [details] [diff] [review]
patch v3.1

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

Henrik, what do you think we should do with the below mentioned function?

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +62,5 @@
> +
> +/**
> + *  Close all popup windows helper method
> + */
> +function closePopupWindows() {

I've thought more about this function, and I'm not sure if this wouldn't be better suited in the tabs library.
We have a similar function named `closeAllTabs`. This method closes all tabs and opens "about:blank" in the last one leaving us in a default position.

Could we rename this method to `closeAllWindows`, move it into tabs (not sure if tabs.js would actually be appropriate since we're not dealing with tabs).
We do have some specific things we do here, as we're only closing "popups" (and we test this by assuming popups don't have the `toolbar` visible).

Would this still work if we've made this function more general to close _all_ windows besides 1 (or maybe better to close all of them and reopen 1 window)?

@@ +65,5 @@
> + */
> +function closePopupWindows() {
> +  var windows = mozmill.utils.getWindows("navigator:browser");
> +  var counter = windows.length;
> +  windows.forEach(function (aWindow) {

This might be better to use while and check for windows.length at each step instead of relying on a `counter` variable.

Comment 20

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

Comment on attachment 8397841 [details] [diff] [review]
patch v3.1

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

The direction looks fine but yes, we have to get this out of this test given that it is a shared method.

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +62,5 @@
> +
> +/**
> + *  Close all popup windows helper method
> + */
> +function closePopupWindows() {

This should actually be part of a windows module. We should not call it closePopupWindows but closeWindows and closeAllWindows. If we really need a method which only closes specific windows, we could add a filter for it.

Comment 21

[Andreea Matei [:AndreeaMatei]]

Attachment: v4.patch

Updated to use windows.js, removed the method previously created in the test to close all windows and did a testrun with 10 entities, just this test. If everything is fine i'll do a full testrun. 

http://mozmill-crowd.blargon7.com/#/endurance/report/41b4fea479d86c7288732f3ea85c727e

Comment 22

[Andrei Eftimie]

Comment on attachment 8499541 [details] [diff] [review]
v4.patch

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

This looks really nice compared to previous versions with the new `closeAllWindows` method!
There are a few minor things to address, otherwise the test looks fine to me.

Also interesting from the graph is that Firefox might now properly clean-up for all closed windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/78c2fb649de7649b6e328c246612193a
Memory footprint going steadily up. (It's possible the FF didn't had time to clean up in this restricted testrun)

::: firefox/tests/endurance/manifest.ini
@@ +11,5 @@
>  [include:testFlash_SWFVideoEmbed/manifest.ini]
>  [include:testFlash_SWFVideoIframe/manifest.ini]
>  [include:testFlash_SWFVideoObject/manifest.ini]
>  [include:testFlash_SWFVideoURL/manifest.ini]
> +[include:testPopups_OpenAndClose/manifest.ini]

Doesn't apply cleanly. I think because of bug 987565.

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +6,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +// Include the required modules
> +var { assert } = require("../../../../lib/assertions");

We should remove this.

@@ +12,5 @@
> +var prefs = require("../../../lib/prefs");
> +var tabs = require("../../../lib/tabs");
> +var windows = require("../../../../lib/windows");
> +
> +const BASE_URL = collector.addHttpResource('../../../../data/');

nit: please use double quotes

@@ +32,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function teardownModule(aModule) {
> +  windows.closeAllWindows(controller.window);

aModule.controller.window

*If it's simpler, we could also pass along only the controller, it works either way.

@@ +42,5 @@
> +/**
> + * Test opening and closing popups
> + */
> +function testOpenAndClosePopups() {
> +  enduranceManager.run(function () {

nit: we can use `() => {}`

Comment 23

[Andreea Matei [:AndreeaMatei]]

Attachment: v4_1.patch

Updated and did full testrun:
http://mozmill-crowd.blargon7.com/#/endurance/report/78c2fb649de7649b6e328c246639f535

Comment 24

[Andrei Eftimie]

Comment on attachment 8501052 [details] [diff] [review]
v4_1.patch

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

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +14,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_URL = BASE_URL + "popups/popup_trigger.html?count=";
> +
> +const DOM_WINDOW_CLOSED = "domwindowclosed";

This is not used anymore. I think we can remove it.

@@ +31,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function teardownModule(aModule) {
> +  windows.closeAllWindows(aModule.controller.window);

I think we might also want to call closeAllTabs so we properly cleanup (we should reset the open tab).

@@ +48,5 @@
> +
> +    enduranceManager.addCheckpoint(enduranceManager.entities + " popup(s) opened");
> +
> +    var windowsLength = mozmill.utils.getWindows("navigator:browser").length;
> +    assert.equal(windowsLength - 1, enduranceManager.entities,

I was worried we might get into race conditions as the only thing we specifically wait for is the TEST_DATA pageLoad.
It seems though that call waits for all popups to load as well.
I'm not sure why that is. The method only waits for the main page. Seems the window opening script blocks (the main thread?) and the page is not loaded until the script finishes to run.
I'm worried this might change with e10s.

Ideally we should do an assert.waitFor and evaluate mozmill.utils.getWindows("navigator:browser").length until it satisfies the length.
This should be more safe going forward.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Attachment: v4_2.patch

Updated to use waitFor, added closeAllTabs in teardown and removed the unused const.
Run with this test only, 10 entities: http://mozmill-crowd.blargon7.com/#/endurance/report/e5ac82d610ad4751cf2efea915268fcb

Comment 26

[Andrei Eftimie]

Comment on attachment 8503069 [details] [diff] [review]
v4_2.patch

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

lgtm

::: firefox/tests/endurance/testPopups_OpenAndClose/test1.js
@@ +48,5 @@
> +    enduranceManager.addCheckpoint(enduranceManager.entities + " popup(s) opened");
> +
> +    var windowsLength = mozmill.utils.getWindows("navigator:browser").length;
> +    assert.waitFor(() => {
> +      return windowsLength - 1 === enduranceManager.entities;

With the fat arrow function syntax, if you omit the curly braces it returns automatically the first expression, so you could simplify this as:
> assert.waitFor(() => windowsLength - 1 === enduranceManager.entities, [...]

Comment 27

[Andreea Matei [:AndreeaMatei]]

Attachment: v4_3.patch

Updated as suggested and because the patch had a conflict in the manifest due to a recent skip. Also did testruns.
Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/64e3acea43a5 (default)