Closed Bug 711129 Opened 12 years ago Closed 10 years ago

Mozmill Endurance test for Open & Close a popup window

Categories

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

defect

Tracking

(firefox36 fixed)

RESOLVED FIXED
Tracking Status
firefox36 --- fixed

People

(Reporter: AlexLakatos, Assigned: AndreeaMatei)

References

Details

(Whiteboard: [mozmill-endurance][sprint])

Attachments

(1 file, 10 obsolete files)

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
Assignee: nobody → alex.lakatos
Status: NEW → ASSIGNED
Whiteboard: [mozmill-endurance]
Depends on: 711424
Attached patch patch v0.1 (obsolete) — Splinter Review
This is a wip patch. I would appreciate some feedback. Thanks.
Attachment #593047 - Flags: feedback?(anthony.s.hughes)
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.
Attachment #593047 - Flags: feedback?(anthony.s.hughes) → feedback+
Attached patch patch v1.0 (obsolete) — Splinter Review
Added the assert.
Attachment #593047 - Attachment is obsolete: true
Attachment #593834 - Flags: review?
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.
Attachment #593834 - Flags: review? → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
Fixed naming
Attachment #593834 - Attachment is obsolete: true
Attachment #595020 - Flags: review?(anthony.s.hughes)
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?
Attachment #595020 - Flags: review?(anthony.s.hughes)
Attachment #595020 - Flags: review+
Attachment #595020 - Flags: feedback?(dave.hunt)
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.
Attachment #595020 - Flags: feedback?(dave.hunt) → feedback-
Assignee: alex.lakatos.dev → nobody
Status: ASSIGNED → NEW
Whiteboard: [mozmill-endurance] → [mozmill-endurance] s=130401 u=new c=endurance p=1
Assignee: nobody → andreea.matei
Status: NEW → ASSIGNED
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.
(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.
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?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Updated test, here you can find the report:
http://mozmill-crowd.blargon7.com/#/endurance/report/25ad365ca7bcf4905e9b700b4fc45fe6
Attachment #595020 - Attachment is obsolete: true
Attachment #732323 - Flags: review?(hskupin)
Attachment #732323 - Flags: review?(dave.hunt)
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"
Attachment #732323 - Flags: review?(hskupin)
Attachment #732323 - Flags: review?(dave.hunt)
Attachment #732323 - Flags: review-
Attached patch patch v2.1 (obsolete) — Splinter Review
Updated as requested.
Attachment #732323 - Attachment is obsolete: true
Attachment #733324 - Flags: review?(hskupin)
Attachment #733324 - Flags: review?(dave.hunt)
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.
Attachment #733324 - Flags: review?(hskupin)
Attachment #733324 - Flags: review?(dave.hunt)
Attachment #733324 - Flags: review-
Depends on: 800872
Priority: -- → P2
Attached patch patch v3 (obsolete) — Splinter Review
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
Attachment #733324 - Attachment is obsolete: true
Attachment #8394754 - Flags: review?(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.
Attachment #8394754 - Flags: review?(andrei.eftimie) → review-
Attached patch patch v3.1 (obsolete) — Splinter Review
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.
Attachment #8394754 - Attachment is obsolete: true
Attachment #8397841 - Flags: review?(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.
Attachment #8397841 - Flags: review?(andrei.eftimie) → feedback?
Attachment #8397841 - Flags: feedback? → feedback?(hskupin)
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.
Attachment #8397841 - Flags: feedback?(hskupin) → feedback+
Attached patch v4.patch (obsolete) — Splinter Review
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
Attachment #8397841 - Attachment is obsolete: true
Attachment #8499541 - Flags: review?(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 `() => {}`
Attachment #8499541 - Flags: review?(andrei.eftimie) → review-
Attached patch v4_1.patch (obsolete) — Splinter Review
Updated and did full testrun:
http://mozmill-crowd.blargon7.com/#/endurance/report/78c2fb649de7649b6e328c246639f535
Attachment #8499541 - Attachment is obsolete: true
Attachment #8501052 - Flags: review?(andrei.eftimie)
Whiteboard: [mozmill-endurance] s=130401 u=new c=endurance p=1 → [mozmill-endurance][sprint]
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.
Attachment #8501052 - Flags: review?(andrei.eftimie) → review-
Attached patch v4_2.patch (obsolete) — Splinter Review
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
Attachment #8501052 - Attachment is obsolete: true
Attachment #8503069 - Flags: review?(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, [...]
Attachment #8503069 - Flags: review?(andrei.eftimie) → review+
Attached patch v4_3.patchSplinter Review
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)
Attachment #8503069 - Attachment is obsolete: true
Attachment #8511962 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.