Last Comment Bug 748686 - Security tests are failing due to favicon UI change
: Security tests are failing due to favicon UI change
Status: RESOLVED FIXED
: regression
Product: Mozilla QA
Classification: Other
Component: Mozmill Tests (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Maniac Vlad Florin (:vladmaniac)
:
Mentors:
http://mozmill-release.blargon7.com/#...
Depends on:
Blocks: 742419
  Show dependency treegraph
 
Reported: 2012-04-25 01:51 PDT by Maniac Vlad Florin (:vladmaniac)
Modified: 2012-05-02 05:23 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
fix patch v1.0 (6.00 KB, patch)
2012-04-25 09:01 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: feedback-
Details | Diff | Splinter Review
[default][mozilla-aurora]patch v2.0 (4.09 KB, patch)
2012-04-26 05:56 PDT, Maniac Vlad Florin (:vladmaniac)
anthony.s.hughes: review+
hskupin: review-
Details | Diff | Splinter Review
Patch v3.0 (4.15 KB, patch)
2012-04-27 01:52 PDT, Maniac Vlad Florin (:vladmaniac)
hskupin: review+
Details | Diff | Splinter Review

Description Maniac Vlad Florin (:vladmaniac) 2012-04-25 01:51:50 PDT
Build: 
Build identifier: Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120424 Firefox/15.0a1

Affected: 
Firefox 14, Firefox 13 (now considering the merge from yesterday)

Errors: There are multiple error type depending on the test, but the cause is the new favicon UI for Firefox
---------------------------------------------------------------------------------------
@resource://mozmill/modules/controller.js:998 @resource://mozmill/modules/frame.js -> file:///tmp/tmp7LQtE5.mozmill-tests/tests/functional/testSecurity/testGreyLarry.js:26 @resource://mozmill/modules/frame.js:557 @resource://mozmill/modules/frame.js:626 @resource://mozmill/modules/frame.js:669 @resource://mozmill/modules/frame.js:506 @resource://mozmill/modules/frame.js:681 @resource://jsbridge/modules/server.js:179 @resource://jsbridge/modules/server.js:183 @resource://jsbridge/modules/server.js:283
----------------------------------------------------------------------------------------
TimeoutError@resource://mozmill/modules/utils.js:429 waitFor@resource://mozmill/modules/utils.js:467 @resource://mozmill/modules/controller.js:648 @resource://mozmill/modules/frame.js -> file:///tmp/tmp7LQtE5.mozmill-tests/tests/functional/testSecurity/testGreenLarry.js:47 @resource://mozmill/modules/frame.js:557 @resource://mozmill/modules/frame.js:626 @resource://mozmill/modules/frame.js:669 @resource://mozmill/modules/frame.js:506 @resource://mozmill/modules/frame.js:681 @resource://jsbridge/modules/server.js:179 @resource://jsbridge/modules/server.js:183 @resource://jsbridge/modules/server.js:283 
-----------------------------------------------------------------------------------------
Controller.assertJSProperty(ID: page-proxy-favicon) : http://www.mozilla.org/media/img/favicon.png ==
------------------------------------------------------------------------------------------
could not validate element ID: identity-icon-label with value mozilla.org
Comment 1 Maniac Vlad Florin (:vladmaniac) 2012-04-25 02:05:24 PDT
Please note that this bug will have a fix patch tomorrow only because of the merge. Need to see runs for Firefox 15 also
Comment 2 Henrik Skupin (:whimboo) 2012-04-25 06:10:29 PDT
Whenever you have a patch please upload. We can ensure that it is working once trunk builds are out - which should happen in the next minutes or so.
Comment 3 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:01:18 PDT
Created attachment 618307 [details] [diff] [review]
fix patch v1.0

Fixes the tests
Comment 4 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:10:10 PDT
Passes in mozmill-crowd 
The patch does not cause any failures 
http://mozmill-crowd.blargon7.com/#/functional/report/2a76d98544f7c11b8b6a1972e90a0841
Comment 5 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-25 09:30:38 PDT
Comment on attachment 618307 [details] [diff] [review]
fix patch v1.0

Patch looks fine. Where does this need to land considering all merges have already occurred? In other words, is the source change in Firefox 15, 14, 13, and 12?
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-25 09:30:58 PDT
Also, did the change make it into esr?
Comment 7 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:32:20 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #5)
> Comment on attachment 618307 [details] [diff] [review]
> fix patch v1.0
> 
> Patch looks fine. Where does this need to land considering all merges have
> already occurred? In other words, is the source change in Firefox 15, 14,
> 13, and 12?

Please land it in Firefox 15 for the time being and we'll check tomorrow's results. than we are landing it for Firefox 14
Comment 8 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:36:28 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #6)
> Also, did the change make it into esr?

The change does not affect esr
Comment 9 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-25 09:36:51 PDT
Comment on attachment 618307 [details] [diff] [review]
fix patch v1.0

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/6ac207bc66d5 (default)
Comment 10 Henrik Skupin (:whimboo) 2012-04-25 09:37:39 PDT
Comment on attachment 618307 [details] [diff] [review]
fix patch v1.0

As said on IRC this behavior has been changed and we can't simply flip over prefs. There is no new ui we would have to test so changing the tests to adapt to the new default behavior is what I would want to see. This is just a plain workaround.

Beside that we could consider to add even more tests as compared to what the litmus test references. Automation is cheap and we have the time. Also Virgil should talk with Anthony first if we need this test at all given the amount of tests for browser chrome.

>+const DISPLAY_SSL_DOMAIN = "browser.identity.ssl_domain_display";

If we want to use it, it should get a PREF_ prefix and name it similar to the pref name.

>-  cert = securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus.serverCert;
>+  cert = securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus.serverCert;  

Nit: unnecessary whitespace change.

>-  controller.assert(function () {
>-    return favicon.getNode().src.indexOf("mozilla.org") !== -1;
>-  }, "Favicon is loaded: got '" + favicon.getNode().src + "'");
>-
>+  expect.equal(favicon.getNode().hidden, false, "Favicon is loaded");
>+  

Please don't change that. All the asserts are taking care of in another bug. With your patch no change is necessary.

>+++ b/tests/functional/testSecurity/testGreenLarry.js
>-  controller.waitFor(function () {
>-    return favicon.getNode().src.indexOf('addons.mozilla') != -1
>-  }, "AMO favicon is loaded.");
>+  expect.equal(favicon.getNode().hidden, false, "Favicon is loaded");

Same here. Also how does it work without changing the pref?

>+++ b/tests/functional/testSecurity/testGreyLarry.js
>-  controller.assertJSProperty(favicon, "src" ,"http://www.mozilla.org/media/img/favicon.png");
>+  var validState = favicon.getNode().getAttribute("pageproxystate");
>+
>+  expect.equal(validState, "valid", "Favicon is visible");

Dito.
Comment 11 Henrik Skupin (:whimboo) 2012-04-25 09:38:23 PDT
This bug is *not* fixed! See my comment from above. I would propose backing out the patch.
Comment 12 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:42:59 PDT
(In reply to Henrik Skupin (:whimboo) from comment #11)
> This bug is *not* fixed! See my comment from above. I would propose backing
> out the patch.

Anthony, due to Henrik's review please backout the patch

Henrik, if the pref is out some checks in the test need to be deleted. There is no other way to test them. Do we want that? 
Please give specific directions here, so I'll know what do to. 

Other nits and problems in your feedback can surely be immediately addressed. 

Thanks
Comment 13 Henrik Skupin (:whimboo) 2012-04-25 09:49:59 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #12)

> Henrik, if the pref is out some checks in the test need to be deleted. There
> is no other way to test them. Do we want that? 
> Please give specific directions here, so I'll know what do to. 

I have already said everything on IRC. Not sure why that hasn't been addressed before the patch has been put up for review. So before you make this change talk to Anthony and Virgil so that the right strategy for this test can be figured out. Here some points (again):

* We should test the default behavior and not a hidden pref setting
* We could consider testing both cases
* Do we need the test(s) at all given the existent browser chrome tests

All this requires feedback from the QA leads of the security component and Anthony to help out and give advices.
Comment 14 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:51:29 PDT
> Same here. Also how does it work without changing the pref?
> 
Henrik, this change has nothing to do with the pref. only the ssl domain name is pref dependent now. That change is because a block of code being removed, its another way to test for the favicon.
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-25 09:54:42 PDT
Patch backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/2c6f7194bd93 (default)
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-25 09:55:46 PDT
Comment on attachment 618307 [details] [diff] [review]
fix patch v1.0

Canceling review because I have nothing further to add from previous r+. I will not be able to comment in terms of code coverage until Virgil has updated the Litmus tests. If we have to wait for that to be done, I propose just disabling the tests until such time.
Comment 17 Maniac Vlad Florin (:vladmaniac) 2012-04-25 09:59:11 PDT
> 
> * We should test the default behavior and not a hidden pref setting


Please keep in mind that this pref enables something extra, meaning the ssl domain. So if we enable the pref, we do an extra-test, which you said its cool to do. 
If we leave the pref to '0', then we need to delete some code in the test, not to perform this check anymore. Otherwise, the behavior is exactly the same. 

The automated tests, as I showed on iRC, have the same logic, 

Here is the code snippet 

function checkResult() {
-  if (gCurrentTest.isHTTPS) {
+  if (gCurrentTest.isHTTPS && Services.prefs.getIntPref("browser.identity.ssl_domain_display") == 1) {
     // Check that the effective host is displayed in the UI
     let label = document.getElementById("identity-icon-label");
     is(label.value, gCurrentTest.effectiveHost, "effective host is displayed in identity icon label for test " + gTestDesc);
   } 

 This is just FYI, I don't mind making any changes you guys need
Comment 18 Henrik Skupin (:whimboo) 2012-04-25 10:39:51 PDT
(In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> Henrik, this change has nothing to do with the pref. only the ssl domain
> name is pref dependent now. That change is because a block of code being
> removed, its another way to test for the favicon.

That's not true Vlad! Please see the generic lock icon which is now presented. There is no favicon of the domain visible anymore.
Comment 19 Maniac Vlad Florin (:vladmaniac) 2012-04-26 00:53:07 PDT
(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #14)
> > Henrik, this change has nothing to do with the pref. only the ssl domain
> > name is pref dependent now. That change is because a block of code being
> > removed, its another way to test for the favicon.
> 
> That's not true Vlad! Please see the generic lock icon which is now
> presented. There is no favicon of the domain visible anymore.

Ok, now lets see how QA gives directions here and we'll just have to decide after that.
Comment 20 Maniac Vlad Florin (:vladmaniac) 2012-04-26 05:56:09 PDT
Created attachment 618633 [details] [diff] [review]
[default][mozilla-aurora]patch v2.0

We don;t support test for ssl domain name anymore, as for bug 748385 

We don't check for the favicon source anymore, since we don't support custom favicons anymore, we just check for default ones to be visible (!hidden) 

Hope this patch complies with the needs 

PS: I've used the old test pattern in the refactoring, to be consistent and let refactoring patches handle the optimizations.
Comment 21 Maniac Vlad Florin (:vladmaniac) 2012-04-26 14:15:20 PDT
Comment on attachment 618633 [details] [diff] [review]
[default][mozilla-aurora]patch v2.0

Moving review to Anthony. 

We will ask for r from Henrik starting Wednesday next week, as he advised. 
Anthony, can you step in please?
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:20:06 PDT
Comment on attachment 618633 [details] [diff] [review]
[default][mozilla-aurora]patch v2.0

Patch looks fine to me -- I'm hesitant to check it in until Henrik gives his review though (don't want to have to back out again).
Comment 23 Henrik Skupin (:whimboo) 2012-04-26 21:40:58 PDT
Comment on attachment 618633 [details] [diff] [review]
[default][mozilla-aurora]patch v2.0

Ok, so just a quick one so that we can fix this failure asap...

>-    return favicon.getNode().src.indexOf("mozilla.org") !== -1;
>-  }, "Favicon is loaded: got '" + favicon.getNode().src + "'");
>+    return favicon.getNode().getAttribute("hidden") == false;
>+  }, "Favicon is loaded");

I would say: "Lock icon is visible in identity box" or so.

>diff --git a/tests/functional/testSecurity/testGreenLarry.js b/tests/functional/testSecurity/testGreenLarry.js

>-    return favicon.getNode().src.indexOf('addons.mozilla') != -1
>+    return favicon.getNode().getAttribute("hidden") == false;
>   }, "AMO favicon is loaded.");

Same as above. It's not the AMO favicon anymore.
Comment 24 Maniac Vlad Florin (:vladmaniac) 2012-04-27 01:52:24 PDT
Created attachment 618954 [details] [diff] [review]
Patch v3.0

You are right. 

Damn one day I'll hopefully learn how to properly make usage of English :)
Comment 25 Henrik Skupin (:whimboo) 2012-04-28 01:28:24 PDT
Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/72f3f3a9f87c

If it works as expected please make sure to land it for aurora too.
Comment 26 Maniac Vlad Florin (:vladmaniac) 2012-05-02 00:34:05 PDT
(In reply to Henrik Skupin (:whimboo) from comment #25)
> Landed on default:
> http://hg.mozilla.org/qa/mozmill-tests/rev/72f3f3a9f87c
> 
> If it works as expected please make sure to land it for aurora too.

Henrik - I have never checked in a patch before (nor me or Alex) - this was left in the backlog a bit with Anthony to make us a demo. We tried once but there were issues with our level 2 access. So I would recommend to have that demo first, then actually check - in patches.
Comment 27 Henrik Skupin (:whimboo) 2012-05-02 05:23:24 PDT
All this is documented very well and you can try this even locally without having to push to a central repository. Not sure what is left here.

Anyway, I have pushed the patch to aurora now:
http://hg.mozilla.org/qa/mozmill-tests/rev/05c59906063b

Note You need to log in before you can comment on or make changes to this bug.