508669 - [mozmill] Test for checking the warning when viewing an encrypted page

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

This is a placeholder bug for creating a Mozmill test script for https://litmus.mozilla.org/show_test.cgi?id=7675 - Warn about viewing encrypted page.

Comment 1

[u279076]

Attachment: WIP Testcase

I'm having a problem with this test...

Here is what I am trying to accomplish:
1. I open the prefs dialog and make sure "Warn about entering an encrypted site"
2. I create a listener for the warning dialog
3. I load an encrypted page
4. Assert the Warning modal dialog which appears

Here is the basic code (see the attachment for more detailed code):
   PrefsAPI.handlePreferencesDialog(prefDialogCallback);
   var md = new ModalDialogAPI.modalDialog(handleSecurityWarningDialog);
   md.start();
   controller.open("https://www.verisign.com");
  
The problem is that before the listener has a chance to capture the modal dialog, teardownModule is launched.  I expect this behaviour because controller.open is basically the last command in my test function.

I've tried controller.sleep but this only delays the problem described above.  It just delays the dialog from appearing by the time indicated by controller.sleep.  In other words, the time the listener has to grab the dialog is the same with or without the sleep.

I've also tried controller.waitForPageLoad as well but this does not work because the dialog is an intermediary between Firefox and the page -- its purpose is to allow the user to prevent pageLoad if they want.  In other words, the dialog appears once a connection is made to the web server.

Aakash had a theory that it may be due to the time delay in modalDialog.start (testModalDialogAPI.js#124) but setting this to 0 yeilds the same result.

Whimboo, can you please review my code and ModalDialogAPI?  I need some help figuring out what is going on here.  If this is a bug in Mozmill (which I suspect because we've never really dealt with modal dialogs triggered after an open()), I will file it.  FYI, I suspect we will run into similar issues with the HTTP Authentication prompt which appears for websites like intranet.mozilla.org.

Comment 2

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

Comment on attachment 395459 [details]
WIP Testcase

Please ask for review when it is ready. Meanwhile I can still give an answer which is upcoming...

Comment 3

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

Comment on attachment 395459 [details]
WIP Testcase

I don't see any problem with the test and controller.waitForPageLoad() used. As you say the modal warning sheet will be opened right after the page load has been started. Using waitForPageLoad will stick the main Mozmill thread into sleep until the modal dialog has been closed. 

Just some drive-by comments...


>var prefDialogCallback = function(controller) {
>  // Get the Security Pane
>  var pane = new elementslib.Lookup(controller.window.document, '/id("BrowserPreferences")/anon({"orient":"vertical"})/anon({"anonid":"selector"})/{"pane":"paneSecurity","label":"Security"}');
>  controller.click(pane);

Please use waitThenClick here and remove the label attribute.

>var getWindowByLocation = function(aURL) {

For what is that function?

>var handleSecurityWarningSettingsDialog = function(controller) {
>  // Get the Security Window controller
>  var secWinController = new mozmill.controller.MozMillController(mozmill.utils.getWindowByTitle("Security Warnings"));

You have the controller as parameter. No need to to create a new one.

>  // Make sure the "encrypted page" pref is checked
>  var prefs = new Array(5);
>  prefs[0] = new elementslib.ID(secWinController.window.document, "warn_entering_secure");
>  prefs[1] = new elementslib.ID(secWinController.window.document, "warn_entering_weak");
>  prefs[2] = new elementslib.ID(secWinController.window.document, "warn_leaving_secure");
>  prefs[3] = new elementslib.ID(secWinController.window.document, "warn_submit_insecure");
>  prefs[4] = new elementslib.ID(secWinController.window.document, "warn_viewing_mixed");

Please use something like: new Array("warn_entering_secure", "warn_entering_weak" ...). That will make the code more readable.

>  for (var i=0; i<5; i++) {

i < prefs.length

>    // Check the "encrypted page" pref if it isn't already
>    if (prefs[i].nodeID == "warn_entering_secure") {

Because we use a hard-coded array you can simply compare with the index.

>var handleSecurityWarningDialog = function(controller) {
>  controller.window.alert("test");
>}

I see it!

Comment 4

[u279076]

> I don't see any problem with the test and controller.waitForPageLoad() used. As
> you say the modal warning sheet will be opened right after the page load has
> been started. Using waitForPageLoad will stick the main Mozmill thread into
> sleep until the modal dialog has been closed. 

Maybe I didn't explain myself clearly.  If I use waitForPageLoad(), the test just hangs.  No Modal dialog appears and pageLoad never happens because it's waiting for the Modal dialog to appear so the user can action it.  It creates a state of limbo.

Comment 5

[u279076]

(In reply to comment #4)
> > I don't see any problem with the test and controller.waitForPageLoad() used. As
> > you say the modal warning sheet will be opened right after the page load has
> > been started. Using waitForPageLoad will stick the main Mozmill thread into
> > sleep until the modal dialog has been closed. 
> 
> Maybe I didn't explain myself clearly.  If I use waitForPageLoad(), the test
> just hangs.  No Modal dialog appears and pageLoad never happens because it's
> waiting for the Modal dialog to appear so the user can action it.  It creates a
> state of limbo.
Strangely, this appears to be working now.  I'll comment further if I run into the issue again...

(I hate these random Mozmill bugs)

Comment 6

[u279076]

Attachment: Initial patch

Comment 7

[u279076]

Attachment: Rev1

Cleaned up some commenting...

Comment 8

[Aakash Desai [:aakashd]]

Comment on attachment 396007 [details] [diff] [review]
Rev1

>+/**
>+ * Litmus test #7675: Encrypted page warning [1.9.1]
>+ * Litmus test #9292: Encrypted page warning [1.9.2]
>+ */

Reference the litmus testcases on top of each testmodule rather than at the top of the testscript from now on


>+var teardownModule = function(module) {
>+  try {
>+    // Reset the warning prefs
>+	var prefs = new Array("security.warn_entering_secure",
>+	                      "security.warn_entering_weak",
>+                          "security.warn_leaving_secure",
>+                          "security.warn_submit_insecure",
>+                          "security.warn_viewing_mixed");
>+    for each (p in prefs) {
>+	  PrefsAPI.preferences.branch.clearUserPref(p);
>+	}
>+  } catch(e) {
>+  }

We've been making a mistake here for awhile on using try/catch statements. We're going to vet this as fine for now, but check out whimboo's comments about this in another testscript once it's implemented. What we'll do for the time being is vet this as fine, but keep an eye out for changes to this.


>+/**
>+ * Call-back handler for preferences dialog
>+ */
>+var prefDialogCallback = function(controller) {
>+  // Get the Security Pane
>+  var pane = new elementslib.Lookup(controller.window.document, 
>+                                    '/id("BrowserPreferences")/anon({"orient":"vertical"})/' +
>+                                    'anon({"anonid":"selector"})/{"pane":"paneSecurity"}');
>+  controller.waitThenClick(pane);
>+  controller.sleep(gDelay);
>+
>+  // Create a listener for the Warning Messages Settings dialog
>+  var md = new ModalDialogAPI.modalDialog(handleSecurityWarningSettingsDialog);
>+  md.start();
>+  
>+  // Click the Warning Messages Settings button
>+  controller.click(new elementslib.ID(controller.window.document, "warningSettings"));
>+  controller.sleep(gDelay);
>+  

Instead of using sleeps, I'd suggest taking them both out and using a waitThenClick on the md click instead. 


>+  // Make sure the "encrypted page" pref is checked
>+  for each (p in prefs) {
>+    var element = new elementslib.ID(controller.window.document, p);
>+    controller.sleep(gDelay);
>+    // Check the "encrypted page" pref if it isn't already
>+    if (p == "warn_entering_secure") {
>+      if (!element.getNode().checked) {
>+        controller.click(element);
>+      }
>+    // Uncheck all other prefs
>+    } else {
>+      if (element.getNode().checked) {
>+        controller.click(element);


Can waitThenClicks work rather than putting a sleep in the for loop?


>+/**
>+ * Helper function to handle interaction with the Security Warning modal dialog
>+ */
>+var handleSecurityWarningDialog = function(controller) {
>+  // Wait for the content to load
>+  controller.waitForElement(new elementslib.ID(controller.window.document, "info.body"));
>+  
>+  // Verify the message text
>+  controller.assertProperty(new elementslib.ID(controller.window.document, "info.body"),
>+                            "textContent", 
>+                            "You have requested an encrypted page. The web site has " +
>+                            "identified itself correctly, and information you see or " +
>+                            "enter on this page can't easily be read by a third party.");
>+		

Gotta stay away from asserts on literal text due to localization problems for users who might be running these testscripts on other locales. As a solution, try an assertValue of the property against what's seen on the dialog box. I'm not sure if that's going to work, but see what you can do instead of this.


>+  controller.waitThenClick(new elementslib.Lookup(controller.window.document, 
>+                                                  '/id("commonDialog")/anon({"anonid":"buttons"})/' +
>+                                                  '{"dlgtype":"accept","xbl:inherits":"disabled=buttondisabledaccept",' +
>+                                                  '"icon":"accept","default":"true"}')); 
>+}


Remove everything after "dlgtype":"accept"as they're unnecessary.


A couple of quick comments:
1. This testscript fails via the command line due to the timers, so make sure to run and verify the testscript works through that method from now on.
2. We're going to talk about this later, but keep in mind we're going to implement @param comments to the top of methods for the testscripts we have. So, think about that when you're creating functions as well.

Comment 9

[u279076]

Attachment: Rev2

Comment 10

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

(In reply to comment #8)
> (From update of attachment 396007 [details] [diff] [review])
> >+/**
> >+ * Litmus test #7675: Encrypted page warning [1.9.1]
> >+ * Litmus test #9292: Encrypted page warning [1.9.2]
> >+ */
> 
> Reference the litmus testcases on top of each testmodule rather than at the top
> of the testscript from now on

Looks like there is some confusion about the naming conventions. The script file itself is the module while it holds a couple of test functions. The Litmus tests should always be listed beneath the license block. But the functions itself should also get a meaningful comment which should/can be the litmus test description.

> >+var teardownModule = function(module) {
> >+  try {
> >+    // Reset the warning prefs
> >+	var prefs = new Array("security.warn_entering_secure",
> >+	                      "security.warn_entering_weak",
> >+                          "security.warn_leaving_secure",
> >+                          "security.warn_submit_insecure",
> >+                          "security.warn_viewing_mixed");
> >+    for each (p in prefs) {
> >+	  PrefsAPI.preferences.branch.clearUserPref(p);
> >+	}
> >+  } catch(e) {
> >+  }
> 
> We've been making a mistake here for awhile on using try/catch statements.
> We're going to vet this as fine for now, but check out whimboo's comments about
> this in another testscript once it's implemented. What we'll do for the time
> being is vet this as fine, but keep an eye out for changes to this.

That way is correct on 1.9.1 because clearUserPref fires an assertion when the pref hasn't been changed. That's not wanted and has been fixed for 1.9.2.

> >+  // Click the Warning Messages Settings button
> >+  controller.click(new elementslib.ID(controller.window.document, "warningSettings"));
> >+  controller.sleep(gDelay);
> >+  
> 
> Instead of using sleeps, I'd suggest taking them both out and using a
> waitThenClick on the md click instead. 

Mmh, gDelay is only used to slow down the test. Eventually we can stop using it since we are getting more familiar with all the available functions and element references.

> >+  // Verify the message text
> >+  controller.assertProperty(new elementslib.ID(controller.window.document, "info.body"),
> >+                            "textContent", 
> >+                            "You have requested an encrypted page. The web site has " +
> >+                            "identified itself correctly, and information you see or " +
> >+                            "enter on this page can't easily be read by a third party.");
> >+		
> 
> Gotta stay away from asserts on literal text due to localization problems for
> users who might be running these testscripts on other locales. As a solution,
> try an assertValue of the property against what's seen on the dialog box. I'm
> not sure if that's going to work, but see what you can do instead of this.

It's a property. You can retrieve it via the StringBundle:
http://mxr.mozilla.org/mozilla-central/source/security/manager/locales/en-US/chrome/pipnss/security.properties#40

Comment 11

[Aakash Desai [:aakashd]]

Comment on attachment 396906 [details] [diff] [review]
Rev2

Looks fantastic., r+. Push it over to henrik to get it pushed.

Comment 12

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

Comment on attachment 396906 [details] [diff] [review]
Rev2

Please request an addl. review the next time.

Comment 13

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

Comment on attachment 396906 [details] [diff] [review]
Rev2

>+var RELATIVE_ROOT = '../../shared-modules';
>+var MODULE_REQUIRES = ['PrefsAPI', 'UtilsAPI', 'ModalDialogAPI'];

>+var teardownModule = function(module) {
>+  try {
>+    // Reset the pop-up blocking pref
>+	var prefs = new Array("security.warn_entering_secure",
>+	                      "security.warn_entering_weak",
>+                          "security.warn_leaving_secure",
>+                          "security.warn_submit_insecure",
>+                          "security.warn_viewing_mixed");
>+    for each (p in prefs) {
>+	  PrefsAPI.preferences.branch.clearUserPref(p);
>+	}
>+  } catch(e) {
>+  }

You have to wrap the try/catch block directly around the clearUserPref call. Otherwise only the first pref will be resetted on the 1.9.1 branch. Instead of iterating through all the prefs you can also reset all the prefs for the "security.*" branch at once. See the URL below for the interface: http://mxr.mozilla.org/mozilla-central/source/modules/libpref/public/nsIPrefBranch.idl#350

>+  // Close the page because the warnings don't appear if you are on the page
>+  // where the warning was triggered
>+  UtilsAPI.closeAllTabs(controller);

That one I don't understand. If other tests rely on the start page they should set in on their own. Given that you should better move it to the beginning of your test function.

>+/**
>+ * Call-back handler for preferences dialog
>+ */

Can you please add the usual @param line please? Just c&p it from other tests.

>+  controller.waitThenClick(pane);
>+
>+  // Create a listener for the Warning Messages Settings dialog
>+  var md = new ModalDialogAPI.modalDialog(handleSecurityWarningSettingsDialog);
>+  md.start();
>+  
>+  // Click the Warning Messages Settings button
>+  controller.waitThenClick(new elementslib.ID(controller.window.document, "warningSettings"));

That could fail so the modal dialog API gets the pref dialog again instead of the settings dialog. It's better to have a waitForElement call before starting the modal dialog observer. Then do a simple click. Otherwise the .start function takes a delay parameter now. I would prefer the former way which is safer.

>+  // Close the preferences dialog
>+  if (mozmill.isWindows) {
>+    okButton = new elementslib.Lookup(controller.window.document, 
>+	                                  '/id("BrowserPreferences")/anon({"anonid":"dlg-buttons"})/{"dlgtype":"accept"}');
>+    controller.click(okButton);
>+  } else {
>+    controller.keypress(null, 'VK_ESCAPE', {});
>+  }
>+}

We should really move this lines to a shared function for the preferences dialog. I'll take care of it in bug 514401.

>+/**
>+ * Helper function to handle interaction with the Security Warning Settings modal dialog
>+ */

Please also add the @param line.

>+  // All the prefs in the dialog
>+  var prefs = new Array("warn_entering_secure",
>+	                    "warn_entering_weak",

nit: indentation

>+    // Check the "encrypted page" pref if it isn't already

already what? :)

>+    if (p == "warn_entering_secure") {
>+      if (!element.getNode().checked) {
>+        controller.waitThenClick(element);
>+      }
>+    
>+	// Uncheck all other prefs
>+    } else {
>+      if (element.getNode().checked) {
>+        controller.waitThenClick(element);
>+      }
>+    }

Seeing this block I feel we have to enhance the controller.check function. Given it a parameter which tells about the final state would make things much easier in all the tests. Would you mind filing a bug for it?

>+var handleSecurityWarningDialog = function(controller) {
>+  // Wait for the content to load
>+  controller.waitForElement(new elementslib.ID(controller.window.document, "info.body"));

Please move the element creation to its own line and...

>+  controller.assertProperty(new elementslib.ID(controller.window.document, "info.body"),
>+                            "textContent", 
>+                            UtilsAPI.getProperty("chrome://pipnss/locale/security.properties",
>+                                                 "EnterSecureMessage"));

reuse it here. Further also separate the getProperty function.

Comment 14

[u279076]

Some of the revisions requested in the previous comment rely on changes made in bug 514401 and bug 512789.

Comment 15

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

Please don't block your test on bug 512789. We can change the used code later.

Comment 16

[u279076]

Attachment: WIP Test Case

I've made the requested revisions using changes to the PrefsAPI but I can't get this to work.  The Prefs dialog opens then closes quickly.  Inserting a sleep only delays the closing of the prefs dialog.  I can't seem to get the Warning Settings dialog to appear at all.  Whimboo, Aakash, could you please have a look at this test and see if there is something I may be doing wrong?  I'm guessing there have been changes made to the API of which I am unaware.

As a hack suggestion, I could probably use setPref() to set the preferences I want.  However, setting it this way defeats the purpose of user-based testing (i.e. mouse clicks in the UI).

Comment 17

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

Comment on attachment 403056 [details]
WIP Test Case

>var prefDialogCallback = function(controller) {
>  // Get the Security Pane  
>  PrefsAPI.preferencesDialog.setPane(controller, "paneSecurity");
>  
>  // Create a listener for the Warning Messages Settings dialog
>  var md = new ModalDialogAPI.modalDialog(handleSecurityWarningSettingsDialog);
>  md.start();
>  
>  // Click the Warning Messages Settings button
>  var warningSettingsButton = new elementslib.ID(controller.window.document,
>						 "warningSettings");
>  controller.click(warningSettingsButton);

I believe that it is this line when your test is failing. Calling a modal dialog from a modal window/dialog itself can cause this situation when you do not give the code enough time to create the dialog and initialize the content. In such a situation the modal dialog API returns the calling dialog again instead of the called one. You should add a delay value like 200 to md.start() which should be enough. 

I hope that will do the trick. I don't see another problem in the code.

Comment 18

[u279076]

I tried adding a delay of 200 to md.start()...ie. md.start(200);
This did not solve my problem. I had to use a delay of 2000 (aka 2 seconds).

I believe I have resolved all issues.  Patch forthcoming.

Comment 19

[u279076]

Attachment: Rev3

Here is my patch.  Pending review of this patch, I'll work on the patch for Submit Encrypted Info Warning.

Comment 20

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

So far it looks good. I only had to make smaller changes. I will check-in the test even we cannot test yet if a modal dialog is really opened when opening the website. We need bug 519714 for the remaining part.

Comment 21

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

Comment on attachment 403776 [details] [diff] [review]
Rev3

Had to make a couple of changes to get it working. We even can check for the modal dialog now.

Comment 22

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

Attachment: Rev4

Comment 23

[u279076]

Comment on attachment 403792 [details] [diff] [review]
Rev4

This looks good to me.  It's interesting to see that you were able to get this working with an md.start delay of 500.  Mine would only work if it is 2000 or larger.

Comment 24

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

The problem is mainly the waitThenClick call. Just try to start the modal dialog observer as late as possible right before the appropriate click. In situations when the call happens from a modal dialog itself (e.g. preferences dialog) it's better to separate it to waitForElement and click. We have to come up with a better solution for it at some point to make it more robust. But that's out of scope at the moment.

I think if you update the other two tests accordingly those should work promptly too. So we can land them soon. Thanks for the work!

Comment 25

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8f84504e5312
http://hg.mozilla.org/qa/mozmill-tests/rev/d33205940ae5

Comment 26

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

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.