615592 - Test failure in testSubmitUnencryptedInfoWarning.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Aaron Train [:aaronmt]]

MODULE: testSubmitUnecnryptedInfoWarning.js
TEST: testSubmitUnencryptedInfoWarning
ERROR: {"exception": {"message": "Modal dialog has been found and processed"}}
BRANCH: 1.9.1, 1.9.2, default
PLATFORM: all

http://hg.mozilla.org/qa/mozmill-tests/file/9fa2aa8bcb44/firefox/testSecurity/testSubmitUnencryptedInfoWarning.js#l80

Preliminary stand-alone runs of this test are illustrating that the test is expecting a modal dialog to appear. A timeout occurs as a result of none triggered.

Comment 1

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

Partly correct. The waitForDialog() call should be moved further down. But this site also doesn't send unencrypted information.

Comment 2

[u279076]

(In reply to comment #1)
> Partly correct. The waitForDialog() call should be moved further down. But this
> site also doesn't send unencrypted information.

Actually, it does send unencrypted information.  The site is protected by a standard SSL cert but the stuff like searches is not protected.  This site was chosen specifically for this purpose.  As you can see, the site will give the following warning as long as security.warn_submit_insecure is set:

Although this page is encrypted, the information you have entered is to be sent over an unencrypted connection and could easily be read by a third party.

Comment 3

[Aaron Train [:aaronmt]]

As indicated, The test seems to pend on waitForDialog before giving the browser the chance to conduct the test. By running the test you can see that nothing is input to the query box, and no search is performed.

Comment 4

[u279076]

(In reply to comment #3)
> As indicated, The test seems to pend on waitForDialog before giving the browser
> the chance to conduct the test. By running the test you can see that nothing is
> input to the query box, and no search is performed.

I seem to recall us having a similar problem with the Firefox searchbox.

Comment 5

[u279076]

(In reply to comment #4)
> (In reply to comment #3)
> > As indicated, The test seems to pend on waitForDialog before giving the browser
> > the chance to conduct the test. By running the test you can see that nothing is
> > input to the query box, and no search is performed.
> 
> I seem to recall us having a similar problem with the Firefox searchbox.

In other words, a focus issue.

Comment 6

[Aaron Train [:aaronmt]]

No. The test resolves itself when you simply move the waitForDialog down just right after we click, as Henrik mentioned in comment #1. The test would then need a few waitForPageLoad's and waitForElements and it should be fixed. As well, a removal of the modal dialog assert as that should be handled in waitForDialog. And, that last line   controller.assertNodeNotExist(searchbox); would fail as it has a bad reference, and the direct results page as well has a searchbox.

Comment 7

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

(In reply to comment #2)
> chosen specifically for this purpose.  As you can see, the site will give the
> following warning as long as security.warn_submit_insecure is set:

Looks like I forgot to set this pref. Thought it's enabled by default. But with no UI in Fx4, I haven't seen this. Sorry, so yeah move the line down. My fault.

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Mainly re-arrranging and some basic refactoring. Note, I didn't do a full sweep of necessary refactoring, only some preliminary. The patch works now with the move of waitForDialog to trigger after the click. I removed the assert for the dialog because we have a try/catch in waitForDialog that handles that. Mainly asking for feedback on the last assert for URL change. What was there prior was a check for non-existant element. That wouldn't work because the element becomes stale with the site change anyhow. Is the assert I replaced it with alright?

Comment 9

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

Aaron, please only change those lines, which are necessary for a fix. We don't want to mix-in refactoring work. That only complicates the patches.

Also you should move down the md.start call, so it is as close as possible to the triggering action.

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Patch v1 - (default)

Removed some areas of refactor

Comment 11

[u279076]

Comment on attachment 494391 [details] [diff] [review]
Patch v1 - (default)

> const TIMEOUT_MODAL_DIALOG = 30000;
>+const TEST_SITE = "http://mail.mozilla.org";

Should be https://mail.mozilla.org

>-  // Check that the search field is not shown anymore
>-  controller.assertNodeNotExist(searchbox);
>-
>-  // Test if the modal dialog has been shown
>-  controller.assertJS("subject.isModalWarningShown == true",
>-                      {isModalWarningShown: persisted.modalWarningShown});
>+  // Check for the switch in URL to the search results page
>+  controller.assert(function() {
>+    return locationBar.contains("google.com/custom");
>+  }, "Current URL should not contain: " + TEST_SITE);

Aren't you doing a search on mail.mo? If so the .contains should refer to mail.mo shouldn't it?

Comment 12

[Aaron Train [:aaronmt]]

Well, when you search on mail.mozilla.org, you end up on a custom Google search page. I was thinking of just checking if that current url has a snippet of that address to verify that accepting the warning, takes one to the search page as it should.

Comment 13

[u279076]

(In reply to comment #12)
> Well, when you search on mail.mozilla.org, you end up on a custom Google search
> page. I was thinking of just checking if that current url has a snippet of that
> address to verify that accepting the warning, takes one to the search page as
> it should.

Sure, I see that now. Perhaps for more robustness though we should expand the verification to assert on the following URL parameters:

&domains=mozilla.org (domain where the search was initiated)
&sitesearch=mozilla.org (domain where the search is performed)
&q=mozilla (string to search for)

Comment 14

[u279076]

>+  // Check for the switch in URL to the search results page
>+  controller.assert(function() {
>+    return locationBar.contains("google.com/custom");
>+  }, "Current URL should not contain: " + TEST_SITE);

One thing I forgot to mention, the error message should be positive in nature.

Something like:
"Expected URL to contain " + whatever

Comment 15

[Aaron Train [:aaronmt]]

Attachment: Patch v2 - (default)

Added your suggested conditions. Note that, yes, the code should be re-factored at a later time.

Comment 16

[u279076]

Comment on attachment 494997 [details] [diff] [review]
Patch v2 - (default)

>+const TEST_SITE = "http://mail.mozilla.org";
> 

You might have missed this the first time (comment 11), but the site should be https, not http.

Comment 17

[Aaron Train [:aaronmt]]

Attachment: Patch v2.1 - (default)

(In reply to comment #16)
> Comment on attachment 494997 [details] [diff] [review]
> Patch v2 - (default)
> 
> >+const TEST_SITE = "http://mail.mozilla.org";
> > 
> 
> You might have missed this the first time (comment 11), but the site should be
> https, not http.

Added.

Comment 18

[Aaron Train [:aaronmt]]

Comment on attachment 495893 [details] [diff] [review]
Patch v2.1 - (default)

Let me adjust the error message.

Comment 19

[Aaron Train [:aaronmt]]

Attachment: Patch v2.2 - (default)

s/http/https

As well, I would like feedback on the change I made to the error message. The assert checks for the switch in URL, and the URL value expected is not very clean*, so at the moment, I hardcoded parts of the address we expect the current URL to contain from the search.


* http://www.google.com/custom?cof=LW%3A174%3BLH%3A60%3BL%3Ahttp%3A%2F%2Fwww.mozilla.org%2Fimages%2Fmlogosm.gif%3BGIMP%3A%23cc0000%3BT%3Ablack%3BALC%3A%230000ff%3BGFNT%3Agrey%3BLC%3A%23990000%3BBGC%3Awhite%3BAH%3Acenter%3BVLC%3Apurple%3BGL%3A0%3BGALT%3A%23666633%3BAWFID%3A9262c37cefe23a86%3B&domains=mozilla.org&sitesearch=mozilla.org&q=mozilla

Comment 20

[u279076]

Comment on attachment 496145 [details] [diff] [review]
Patch v2.2 - (default)

>+  controller.assert(function() {
>+    return locationBar.contains("google.com/custom") && 
>+           locationBar.contains("&domains=mozilla.org") &&
>+           locationBar.contains("&sitesearch=mozilla.org") &&
>+           locationBar.contains("&q=mozilla");
>+  }, "URL value contained - got: " + locationBar.value + ", expected " +
>+     "URL value containing " + "google.com/custom" + "&domains=mozilla.org" +
>+     "&sitesearch=mozilla.org" + "&q=mozilla");

So I have two comments about this:

1) These strings should probably be objects within the TEST_PAGE constant
2) I'd prefer the error message to be simpler; something like:
"Expected search results page to be loaded."

Comment 21

[Aaron Train [:aaronmt]]

In regards to #2, the format was specified from Henrik, see https://github.com/whimboo/mozmill-api-refactor/blob/master/modules/assert.js

For #1, the strings have nothing to do with the test_page constant. Henrik?

Comment 22

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

(In reply to comment #21)
> In regards to #2, the format was specified from Henrik, see
> https://github.com/whimboo/mozmill-api-refactor/blob/master/modules/assert.js

Nothing has been specified. It was a proposal, and I have added it to our remaining style guide topics. It's something we really should use to have clear and meaningful data in our hands.

> For #1, the strings have nothing to do with the test_page constant. Henrik?

Why do we have to do such a scary check? Why not simply check the search field for the search term?

Comment 23

[u279076]

(In reply to comment #22)
> (In reply to comment #21)
> > In regards to #2, the format was specified from Henrik, see
> > https://github.com/whimboo/mozmill-api-refactor/blob/master/modules/assert.js

I'd prefer to see "actual" rather than "got".

> Why do we have to do such a scary check? Why not simply check the search field
> for the search term?

Ideally, I'd like to be verifying that search results are actually returned.  Neither checking the URL nor checking the searchbox achieves that.  However, I'm not sure how you can check that reliably, apart from parsing the DOM.

Comment 24

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

(In reply to comment #23)
> > > https://github.com/whimboo/mozmill-api-refactor/blob/master/modules/assert.js
> 
> I'd prefer to see "actual" rather than "got".

This comes from Mochitest and has not been invented by me. And I think that's the way we will have to go with Mozmill itself, once the assert functions have been refactored.

> Ideally, I'd like to be verifying that search results are actually returned. 
> Neither checking the URL nor checking the searchbox achieves that.  However,
> I'm not sure how you can check that reliably, apart from parsing the DOM.

That's not the primary task for this test and also isn't part of the Litmus test. But I wouldn't have anything against it.

Comment 25

[u279076]

> This comes from Mochitest and has not been invented by me. And I think that's
> the way we will have to go with Mozmill itself, once the assert functions have
> been refactored.
> 

That's not really a sound argument, IMO.  However, if that's how we decide to go, then fine.

> That's not the primary task for this test and also isn't part of the Litmus
> test. But I wouldn't have anything against it.

In that case, waitForPageLoad() should be enough.

Comment 26

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

(In reply to comment #25) 
> > That's not the primary task for this test and also isn't part of the Litmus
> > test. But I wouldn't have anything against it.
> 
> In that case, waitForPageLoad() should be enough.

We would have to check at least for the domain or for any kind of element on the target page. By only using waitForPageLoad we don't know if the web page has been changed.

Comment 27

[u279076]

(In reply to comment #26)
> (In reply to comment #25) 
> > > That's not the primary task for this test and also isn't part of the Litmus
> > > test. But I wouldn't have anything against it.
> > 
> > In that case, waitForPageLoad() should be enough.
> 
> We would have to check at least for the domain or for any kind of element on
> the target page. By only using waitForPageLoad we don't know if the web page
> has been changed.

Which leads us back to my original point:
> Ideally, I'd like to be verifying that search results are actually returned.

Comment 28

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Let's just get this fixed. I'm in favour of simplifying the check by checking for an element. Luckily, the results page has a name identifier to differentiate it from the ID identifier of the similar field on the initial mail.mo page. See new patch.

Comment 29

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default) [removed toolbars shared module inclusion]

Removed unnecessary toolbars shared module inclusion from previous patch

Comment 30

[Aaron Train [:aaronmt]]

Landed on default - http://hg.mozilla.org/qa/mozmill-tests/rev/e885ee9e1007

Patch didn't apply to older branches, patch coming tomorrow

Comment 31

[Aaron Train [:aaronmt]]

Landed on 1.9.2 and 1.9.1

http://hg.mozilla.org/qa/mozmill-tests/rev/20d4ab5ce247 - 1.9.2
http://hg.mozilla.org/qa/mozmill-tests/rev/aa9c827c8506 - 1.9.1

Comment 32

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

(In reply to comment #31)
> Landed on 1.9.2 and 1.9.1

Aaron, as our rules say, you have to re-request for review before landing modified patches. Please keep this in mind for the future.