Closed Bug 508669 Opened 11 years ago Closed 10 years ago

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

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ashughes, Assigned: ashughes)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Status: NEW → ASSIGNED
Depends on: 508691
Attached file WIP Testcase (obsolete) —
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.
Attachment #395459 - Flags: review?(hskupin)
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...
Attachment #395459 - Flags: review?(hskupin) → review-
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!
> 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.
(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)
Attached patch Initial patch (obsolete) — Splinter Review
Attachment #395459 - Attachment is obsolete: true
Attachment #396004 - Flags: review?
Attached patch Rev1 (obsolete) — Splinter Review
Cleaned up some commenting...
Attachment #396004 - Attachment is obsolete: true
Attachment #396007 - Flags: review?
Attachment #396004 - Flags: review?
Attachment #396007 - Flags: review? → review?(adesai)
Attachment #396007 - Flags: review?(adesai) → review-
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.
Attached patch Rev2 (obsolete) — Splinter Review
Attachment #396007 - Attachment is obsolete: true
Attachment #396906 - Flags: review?
Attachment #396906 - Flags: review? → review?(adesai)
(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 on attachment 396906 [details] [diff] [review]
Rev2

Looks fantastic., r+. Push it over to henrik to get it pushed.
Attachment #396906 - Flags: review?(adesai) → review+
Comment on attachment 396906 [details] [diff] [review]
Rev2

Please request an addl. review the next time.
Attachment #396906 - Flags: review?(hskupin)
Attachment #396906 - Flags: review?(hskupin) → review-
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.
Some of the revisions requested in the previous comment rely on changes made in bug 514401 and bug 512789.
Depends on: 514401, 512789
Please don't block your test on bug 512789. We can change the used code later.
Assignee: ashughes → anthony.s.hughes
Attached file WIP Test Case (obsolete) —
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).
Attachment #403056 - Flags: review?(hskupin)
Attachment #403056 - Flags: review?(adesai)
Attachment #403056 - Flags: review?(hskupin)
Attachment #403056 - Flags: review?(adesai)
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.
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.
Attached patch Rev3Splinter Review
Here is my patch.  Pending review of this patch, I'll work on the patch for Submit Encrypted Info Warning.
Attachment #396906 - Attachment is obsolete: true
Attachment #403056 - Attachment is obsolete: true
Attachment #403776 - Flags: review?(hskupin)
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.
Depends on: 519714
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.
Attachment #403776 - Flags: review?(hskupin) → review-
Attachment #403792 - Flags: review? → review?(anthony.s.hughes)
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.
Attachment #403792 - Flags: review?(anthony.s.hughes) → review+
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!
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.
Component: Security → Mozmill Tests
Product: Firefox → Mozilla QA
QA Contact: firefox → mozmill-tests
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.