505611 - [mozmill] Improve level of testing for testSecurityNotification.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Proposed changes:
1. Check the actual link reported in the error page, not just that it exists
2. Check the error code is being displayed
3. CSS background image URLs should be url("blah.png") not url(blah.png)

Comment 1

[u279076]

Attachment: Initial patch

Comment 2

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

Comment on attachment 389839 [details] [diff] [review]
Initial patch

>- * The Original Code is Mozmill Test Code.
>- *

Please revert this change.

>  * Contributor(s):
>  *   Aakash Desai <[e-mail]>
>+ * Anthony Hughes <[e-mail]>

Please fix the indentation.

>+var teardownModule = function(module) {
>+  UtilsAPI.closeAllTabs(controller);
>+}

This function is not needed.

>+ * Tests the identify/favicon button and Bad Cert error page

Nit: "Test the..."

>-  if (cssSecButton.getPropertyValue('list-style-image') != 'url(chrome://browser/skin/Secure-statusbar.png)') {
>+  if (cssSecButton.getPropertyValue('list-style-image') != 'url("chrome://browser/skin/Secure-statusbar.png")') {

Why it needs quotes around the URL? DOMi doesn't show those for all computes styles.

>+  var link = new elementslib.ID(controller.tabs.activeTab, "cert_domain_link");
>+  controller.waitForElement(link);
>+  controller.assertNode(link);
>+  if (link.getNode().textContent != "www.verisign.com") {  
>+    throw "Site link is invalid!";
>+  }

No need for assertNode. And can you please use assertProperty?

>+  // Verify the error code is correct
>+  var text = new elementslib.ID(controller.tabs.activeTab, "technicalContentText");
>+  controller.waitForElement(text);
>+  controller.assertNode(text);

You can remove assertNode here too.

Comment 3

[u279076]

Attachment: Rev1

I've made all requested revisions save for this one:

>> -  if (cssSecButton.getPropertyValue('list-style-image') != 'url(chrome://browser/skin/Secure-statusbar.png)') {
>> +  if (cssSecButton.getPropertyValue('list-style-image') != 'url("chrome://browser/skin/Secure-statusbar.png")') {
>
> Why it needs quotes around the URL? DOMi doesn't show those for all computes
> styles.

In the case of "list-style-image" quotes are used in the attribute value.  Excluding quotes causes the test to fail 80% of the time I run it.  Personally, I think this it is safer to assume quotes.  You are right that DOMi doesn't show it for ALL computed styles, but it does for THIS computed style.

A couple of the revisions I implemented but they confuse me:

>> +var teardownModule = function(module) {
>> +  UtilsAPI.closeAllTabs(controller);
>> +}
>
> This function is not needed.
It was my understanding that it was a good idea to use this whenever we loaded a page during the test.  IMO, all tests should clean up after themselves.  Before we never used it, then we did, now we don't again? We've used this recently, why no longer?

>>   * Contributor(s):
>>   *   Aakash Desai <[e-mail]>
>> + * Anthony Hughes <[e-mail]>
>
> Please fix the indentation.
Indenting so that names are right-aligned and emails are right-aligned is more readable IMO.

Comment 4

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

(In reply to comment #3)
> > Why it needs quotes around the URL? DOMi doesn't show those for all computes
> > styles.
> 
> In the case of "list-style-image" quotes are used in the attribute value. 
> Excluding quotes causes the test to fail 80% of the time I run it.  

Interesting. I saw failures lately too. Lets see if this fixes it.

> >> +var teardownModule = function(module) {
> >> +  UtilsAPI.closeAllTabs(controller);
> >> +}
> >
> > This function is not needed.
> It was my understanding that it was a good idea to use this whenever we loaded
> a page during the test.  IMO, all tests should clean up after themselves. 
> Before we never used it, then we did, now we don't again? We've used this
> recently, why no longer?

You don't open any additional tabs in your test. So there is nothing you have to restore afterward.

> >>   * Contributor(s):
> >>   *   Aakash Desai <[e-mail]>
> >> + * Anthony Hughes <[e-mail]>
> >
> > Please fix the indentation.
> Indenting so that names are right-aligned and emails are right-aligned is more
> readable IMO.

This is the convention which is used everywhere else. We shouldn't change it. Please also consider really long names which could make it look ugly.

Comment 5

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

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

>-  if (cssSecButton.getPropertyValue('list-style-image') != 'url(chrome://browser/skin/Secure-statusbar.png)') {
>+  if (cssSecButton.getPropertyValue('list-style-image') != 'url("chrome://browser/skin/Secure-statusbar.png")') {

Now I know why it failed. I forgot the remove the patch from the last review. As what I can see we really cannot go this way. The url cannot have quotes around the uri. It always fails for me.

Further and the most annoying problem is, that the whole test doesn't work on Windows. The list-style-image is different on OS X, Windows, and probably Linux too. So whether we compare another uri for each OS or we simply trust and use "x != ''". I would tend to do the latter way for now. Probably this also affects the other comparisons.

Comment 6

[u279076]

(In reply to comment #5)
> (From update of attachment 391374 [details] [diff] [review])
> >-  if (cssSecButton.getPropertyValue('list-style-image') != 'url(chrome://browser/skin/Secure-statusbar.png)') {
> >+  if (cssSecButton.getPropertyValue('list-style-image') != 'url("chrome://browser/skin/Secure-statusbar.png")') {
> 
> Now I know why it failed. I forgot the remove the patch from the last review.
> As what I can see we really cannot go this way. The url cannot have quotes
> around the uri. It always fails for me.
> 
It always fails for me without quotes.  Using DOMi, list-style-image has quotes.  Please advise.

Comment 7

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

Please don't run the tests in Minefield until we decided to branch for 3.5. As what I can see the quotes are only existent on trunk. For now we stay on 3.5. Please make a comment so we know that we have to change it once we have branched.

Comment 8

[u279076]

We'll be branching soon, no?  Why don't I just wait until then.  This will save us all the hassle.

Comment 9

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

There is no ETA yet. Since at least I run daily Mozmill tests against Shiretoko that would be counter-productive. We would have to change it in any way for 3.5 after the branching. So just let us use it without quotes for now.

Comment 10

[u279076]

When you say Shiretoko, 3.5.2 should be ok to use?

Comment 11

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

Sure, any version from the 1.9.1 branch.

Comment 12

[u279076]

Attachment: Rev2

Suggested revisions made...please review.

Comment 13

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

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

Please see my second paragraph in comment 5. The test doesn't work on Windows.

Please test at least on Windows too before requesting a review.

Comment 14

[u279076]

(In reply to comment #13)
> (From update of attachment 392615 [details] [diff] [review])
> Please see my second paragraph in comment 5. The test doesn't work on Windows.
> 
> Please test at least on Windows too before requesting a review.

I test on Mac, Windows and Linux before submitting for review.  It works for me on all platforms.

Comment 15

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

Anthony, I don't blame you. This failure went in with the test from Aakash. But as you can read in comment 5 it cannot work. The check for the background url of the security-button will fail an Windows and Linux because on OS X it is a different one.

http://mxr.mozilla.org/mozilla1.9.1/source/browser/themes/winstripe/browser/browser.css#1294
http://mxr.mozilla.org/mozilla1.9.1/source/browser/themes/pinstripe/browser/browser.css#1400
http://mxr.mozilla.org/mozilla1.9.1/source/browser/themes/gnomestripe/browser/browser.css#1122

Further is bug 503480 not an issue for you anymore?

So can you please make the change?

Comment 16

[u279076]

> Further and the most annoying problem is, that the whole test doesn't work on
> Windows. The list-style-image is different on OS X, Windows, and probably Linux
> too. So whether we compare another uri for each OS or we simply trust and use
> "x != ''". I would tend to do the latter way for now. Probably this also
> affects the other comparisons.

Upon further investigation, I found the list-style-image attribute is set to "none" when the icon is not visible.  Checking for x != 'none' is far better than x != '' IMO.  In fact, it's probably an approach we want to take in the future for all images which are "hidden".

Patch to follow...

Comment 17

[u279076]

Attachment: Revised test

Here is the full test.  HG is currently down...
Please review and we can create a patch later.

Comment 18

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

Anthony, I wonder a bit about the needs for this Mozmill test at all when seeing your 3 tests for each larry state (green, blue, gray). I feel we should move everything from this test into the comprehensive tests on bug 511208, bug 511314, and bug 511318. What do you think?

Comment 19

[u279076]

I'll review and get back to you on this.

Comment 20

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

We should go with this test. Even when some parts are duplicated. Security is a major area so we can have a duplicate. I will check this test today.

Comment 21

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

Comment on attachment 395487 [details]
Revised test

We have to do some updates to get it working on all platforms. Given the fact it is a smoketest we need it definitely.

Comment 22

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

Attachment: Patch for all platforms

This test works for Firefox 3.5.3 and the latest Namoroka nightly build. Anthony can you please check if everything is ok? Thanks.

Comment 23

[u279076]

Comment on attachment 396866 [details] [diff] [review]
Patch for all platforms

Code looks fine.  Test passes.  R+

Comment 24

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

Thanks Anthony. Updates have been landed on both branches:
http://hg.mozilla.org/qa/mozmill-tests/rev/20906d98da60
http://hg.mozilla.org/qa/mozmill-tests/rev/51cb4f07587d

Comment 25

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

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.