Closed Bug 748686 Opened 12 years ago Closed 12 years ago

Security tests are failing due to favicon UI change

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(firefox14 fixed, firefox15 fixed)

RESOLVED FIXED
Tracking Status
firefox14 --- fixed
firefox15 --- fixed

People

(Reporter: vladmaniac, Assigned: vladmaniac)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Please note that this bug will have a fix patch tomorrow only because of the merge. Need to see runs for Firefox 15 also
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.
Attached patch fix patch v1.0 (obsolete) — Splinter Review
Fixes the tests
Attachment #618307 - Flags: review?(anthony.s.hughes)
Depends on: 742419
Passes in mozmill-crowd 
The patch does not cause any failures 
http://mozmill-crowd.blargon7.com/#/functional/report/2a76d98544f7c11b8b6a1972e90a0841
Blocks: 742419
No longer depends on: 742419
Keywords: regression
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?
Attachment #618307 - Flags: review?(anthony.s.hughes) → review+
Also, did the change make it into esr?
(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
(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 on attachment 618307 [details] [diff] [review]
fix patch v1.0

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/6ac207bc66d5 (default)
Attachment #618307 - Attachment description: fix patch v1.0 → fix v1.0 [checked-in:default]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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.
Attachment #618307 - Attachment description: fix v1.0 [checked-in:default] → fix patch v1.0
Attachment #618307 - Flags: review?(anthony.s.hughes)
Attachment #618307 - Flags: review+
Attachment #618307 - Flags: feedback-
This bug is *not* fixed! See my comment from above. I would propose backing out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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
(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.
Status: REOPENED → ASSIGNED
> 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.
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
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.
Attachment #618307 - Flags: review?(anthony.s.hughes)
> 
> * 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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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.
(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.
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.
Attachment #618307 - Attachment is obsolete: true
Attachment #618633 - Flags: review?(hskupin)
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?
Attachment #618633 - Flags: review?(hskupin) → review?(anthony.s.hughes)
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).
Attachment #618633 - Flags: review?(anthony.s.hughes) → review+
Keywords: checkin-needed
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.
Attachment #618633 - Flags: review-
Attached patch Patch v3.0Splinter Review
You are right. 

Damn one day I'll hopefully learn how to properly make usage of English :)
Attachment #618633 - Attachment is obsolete: true
Attachment #618954 - Flags: review?(hskupin)
Attachment #618954 - Attachment description: [default][mozilla-aurora]patch v3.0 → Patch v3.0
Attachment #618954 - Flags: review?(hskupin) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(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.
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
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: