493083 - Mozmill test for verifying pop-ups are blocked

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

This goes hand in hand with the "Pop-up Blocked" smoketest in litmus (see URL).

Comment 1

[u279076]

Attachment: TESTCASE: Rev1

This test case checks the warning and checks to make sure no new windows (pop-ups) appear.

Comment 2

[u279076]

Attachment: TESTCASE: Rev3

Comment 3

[u279076]

Attachment: TESTCASE: Rev4

Test case to check pop-ups not allowed, don't appear, and that the warning displays.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 377715 [details]
TESTCASE: Rev4

That looks good. But we have to make some modifications.

> * **** END LICENSE BLOCK ***** */
>
>
>var mozmill = {}; Components.utils.import('resource://mozmill/modules/mozmill.js', mozmill);
>var elementslib = {}; Components.utils.import('resource://mozmill/modules/elementslib.js', elementslib);

Just a nit which you can fix beside the other comments. Please always use only one empty line between code blocks.

>/** This test script is for testing 
> *  Litmus Test Case #5918: Pop-up Blocked
> *  https://litmus.mozilla.org/show_test.cgi?id=5918
> */

As what I can see we have different ways how to comment and build the tests. Right now we have created one test function for each Litmus test and put a one-liner comment in-front. If helper functions are needed we put them into the test too.

Clint, which way we wanna go? I think in this case it would be good to give a direction.

>var testPopUpBlocked = function(){
>  // Ensure the pref is checked
>  prefcontroller.click(new elementslib.Elem( prefcontroller.tabs.Content.button));
>  var pref = new elementslib.ID(prefcontroller.window.document, "popupPolicy");
>  if (!pref.getNode().checked) {
>    prefcontroller.click(pref);
>  }

You should insert a waitForElement here before accessing the pref element. The panes are slowly loaded which will end in a fail. Further please move the getPreferencesController call into this function and close the preferences dialog immediately.

>  // Check that the Window count has not changed since the pop-up test
>  // (i.e. no new windows appeared)
>  if (preWindowCount != postWindowCount) {
>    throw "Error: Failed to block pop-up";
>  }     

You can remove the Error prefix. Mozmill will automatically put a Fail in-front.

>  var panel = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})');  
>  if (panel.hasChildNodes) {
>    if (panel.firstChild.nodeName != "xul:notificationbox") {
>      throw "Error: No Warning displayed";
>    }
>  }
>}

Somehow this doesn't work when you only run the second test and the notification bar is not visible. No exception is thrown in such a case.

Comment 5

[cmtalbert]

(In reply to comment #4)
> >/** This test script is for testing 
> > *  Litmus Test Case #5918: Pop-up Blocked
> > *  https://litmus.mozilla.org/show_test.cgi?id=5918
> > */
> 
> As what I can see we have different ways how to comment and build the tests.
> Right now we have created one test function for each Litmus test and put a
> one-liner comment in-front. If helper functions are needed we put them into the
> test too.
> 
> Clint, which way we wanna go? I think in this case it would be good to give a
> direction.
> 
I think that we should just default to whatever makes a more readable and maintainable test.  Not all the Litmus tests are actually *one* test.  Several of them (https://litmus.mozilla.org/manage_testcases.cgi?testcase_id=7716 comes to mind) are actually several tests.  I think that when automating these it makes sense to use separate test functions for each testable unit in the Litmus test.

Besides, by using multiple test functions you will also have the built-in test hierarchy flexibility to do setup/teardown between the test functions to make your tests more reliable and modular.  

I'd say do whatever makes the most readable maintainable code.  We really shouldn't enforce convention just for the sake of enforcing convention.  We should always strive to create the best code possible.
> >var testPopUpBlocked = function(){
> >  // Ensure the pref is checked
> >  prefcontroller.click(new elementslib.Elem( prefcontroller.tabs.Content.button));
> >  var pref = new elementslib.ID(prefcontroller.window.document, "popupPolicy");
> >  if (!pref.getNode().checked) {
> >    prefcontroller.click(pref);
> >  }
> 
> You should insert a waitForElement here before accessing the pref element. The
> panes are slowly loaded which will end in a fail. Further please move the
> getPreferencesController call into this function and close the preferences
> dialog immediately.
> 
> >  // Check that the Window count has not changed since the pop-up test
> >  // (i.e. no new windows appeared)
> >  if (preWindowCount != postWindowCount) {
> >    throw "Error: Failed to block pop-up";
> >  }     
> 
> You can remove the Error prefix. Mozmill will automatically put a Fail
> in-front.
> 
> >  var panel = new elementslib.Lookup(controller.window.document, '/id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})');  
> >  if (panel.hasChildNodes) {
> >    if (panel.firstChild.nodeName != "xul:notificationbox") {
> >      throw "Error: No Warning displayed";
> >    }
> >  }
> >}
> 
> Somehow this doesn't work when you only run the second test and the
> notification bar is not visible. No exception is thrown in such a case.

Comment 6

[u279076]

Attachment: TEST CASE: Rev5

Here is a revised version of my test case.  I will be accompanying an HTML test site which creates Pop-ups.

Comment 7

[u279076]

Attachment: TEST CASE: HTML Rev1

Here is the HTML test site to use with Rev5 of the test case.  It must be placed in a "files" subfolder in relation to the JS testcase file.

Comment 8

[u279076]

Bug 493671 prevents me from completing this test file.

Comment 9

[u279076]

Attachment: TEST CASE: Rev5

Revisions made to only destroy the prefs dialog after the test is complete.
This test case is still blocked by bug 493671, so I am not sure what value there is in reviewing this test case as of yet.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 378224 [details]
TEST CASE: Rev5

Needs a new patch with the prefAPI changes. Canceling review request for now.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 378194 [details]
TEST CASE: HTML Rev1

I wonder if we should open at least two popups so we can make sure that we block multiple popups. This test should be checked into the litmus testcase repository when it's ready.

Comment 12

[u279076]

Attachment: Test HTML file rev2

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Will this testcase work well with waitForPageLoad? I wonder if it will already return when the page has been finished loading and not when all popups have been opened.

Comment 14

[u279076]

Attachment: Test Script

This test script passes but with two caveats:

1. testWarningAppears fails because the warning appears a little slower than the test itself (it is not instantaneous).

2. There is no intuitive way to get the warning bar itself because each warning bar is given a unique panelID based on a timestamp.  For example:

'/id("main-window")/id("browser")/id("appcontent")/id("content")/anon({"anonid":"tabbox"})/anon({"anonid":"panelcontainer"})/{"flex":"1","id":"panel1245781740411"}/{"label":"Shiretoko prevented this site from opening 10 pop-up windows.","value":"popup-blocked","image":"chrome://browser/skin/Info.png","priority":"5","type":"warning"}/anon({"class":"notification-inner outset","flex":"1","xbl:inherits":"type","type":"warning"})/anon({"anonid":"details"})/anon({"anonid":"messageText"})'

This makes it almost impossible to create an Element referencing the warning bar.  Hence why testWarningAppears is coded the way it is.

3. testWarningAppears in no way guarantees that what I am checking is actually the pop-up blocked warning bar.  Ideally, I want to check against the "pop-ups blocked" string within the warning bar.

Comment 15

[u279076]

(In reply to comment #13)
> Will this testcase work well with waitForPageLoad? I wonder if it will already
> return when the page has been finished loading and not when all popups have
> been opened.

That's exactly what happens.  WaitForPageLoad() returns when the page on the parent window is loaded, not the page of the last pop-up window.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Test which should pass

This test passes for me when running from command line or starting via the shortcut. Due to some weirdness with Mozmill it will not work if the overlays of the editor are open. I'll file a bug on that.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Cleaned up test

I have removed the debug output and also added the closing of remaining popup windows to the teardownModule function.

Anthony and Clint, does this test work for you?

Comment 18

[u279076]

Attachment: Test HTML file rev3 (checked-in)

Comment 19

[u279076]

Attachment: Patch: testPopupBlocked.js

This test script tests blocking of pop-ups using the HTML test file attached to this bug.

Comment 20

[u279076]

Attachment: Patch: testPopupBlocked.js

Made a mistake in my last patch (missing a couple lines of code).

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to comment #18)
> Created an attachment (id=384986) [details]
> Test HTML file rev3

Why 10 pop-ups? Two of them are sufficient. So we can check that pop-ups are blocked and have a number != 1.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 385021 [details] [diff] [review]
Patch: testPopupBlocked.js

>+  for each (window in mozmill.utils.getWindows()) {
>+    if (window.content && window.content.location == "http://www.mozilla.org") {

Why have you removed the '/' from the end of the URL? It was on purpose. Otherwise it will not work.

>+  // Check for the status bar icon
>+  var icon = new elementslib.ID(controller.window.document, "page-report-button");
>+  UtilsAPI.delayedAssertNode(controller, icon);

Nice catch. Yes we should check this status icon too. But this on is not sufficient because it will exist all the time. Can you please check against the CSS display attribute? It toggles between none and -moz-box.

>+  // Check that the window count has not changed
>+  if (windowCount != mozmill.utils.getWindows().length) {
>+    throw "Failed to block pop-ups";

Can you change the wording for the error to something like "Pop-ups not blocked". Failed is automatically added by Mozmill as prefix.

>+  // Make sure the pref is checked
>+  var pref = new elementslib.ID(controller.window.document, "popupPolicy");
>+  var checked = pref.getNode().checked;

Can you please add waitforElement before accessing the pref element. If another pane is selected initially it will fail that way. 

We should have it really soonish.

Comment 23

[u279076]

(In reply to comment #21)
> (In reply to comment #18)
> > Created an attachment (id=384986) [details] [details]
> > Test HTML file rev3
> 
> Why 10 pop-ups? Two of them are sufficient. So we can check that pop-ups are
> blocked and have a number != 1.

Because 10 pop-ups are better than 2.  It's called due diligence.  It really causes no problems with the testing process and there is really no reason why we cannot check against 10 pop-ups.  I don't want to be the one with egg on my face when we work with 2 pop-ups but fail with >2 pop-ups.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 384986 [details]
Test HTML file rev3 (checked-in)

Checked into Litmus repository:  http://hg.mozilla.org/webtools/litmus/rev/a7584369ddbe

Comment 25

[u279076]

Attachment: Patch: testPopupsBlocked.js

Suggested revisions made.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Comment on attachment 385169 [details] [diff] [review]
Patch: testPopupsBlocked.js

Looks good. I did a little tweak to the Ok button for the preferences dialog and added a timeout of 1000 to the delayedAssertNode against the x button. If this test fail we don't have to wait 30s.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Checkedin: http://hg.mozilla.org/qa/mozmill-tests/rev/99d0906769c3

Comment 28

[u279076]

Verified.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.