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)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox14 fixed, firefox15 fixed)
RESOLVED
FIXED
People
(Reporter: vladmaniac, Assigned: vladmaniac)
References
()
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
4.15 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → vlad.mozbugs
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
Fixes the tests
Attachment #618307 -
Flags: review?(anthony.s.hughes)
Assignee | ||
Comment 4•12 years ago
|
||
Passes in mozmill-crowd The patch does not cause any failures http://mozmill-crowd.blargon7.com/#/functional/report/2a76d98544f7c11b8b6a1972e90a0841
Updated•12 years ago
|
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+
Assignee | ||
Comment 7•12 years ago
|
||
(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
Assignee | ||
Comment 8•12 years ago
|
||
(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 10•12 years ago
|
||
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-
Comment 11•12 years ago
|
||
This bug is *not* fixed! See my comment from above. I would propose backing out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•12 years ago
|
||
(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•12 years ago
|
||
(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
Assignee | ||
Comment 14•12 years ago
|
||
> 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 15•12 years ago
|
||
Patch backed out: http://hg.mozilla.org/qa/mozmill-tests/rev/2c6f7194bd93 (default)
Comment 16•12 years ago
|
||
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)
Assignee | ||
Comment 17•12 years ago
|
||
>
> * 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
Assignee | ||
Updated•12 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
(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.
Assignee | ||
Comment 20•12 years ago
|
||
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)
Assignee | ||
Comment 21•12 years ago
|
||
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 22•12 years ago
|
||
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 23•12 years ago
|
||
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-
Assignee | ||
Comment 24•12 years ago
|
||
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)
Updated•12 years ago
|
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Updated•12 years ago
|
Attachment #618954 -
Attachment description: [default][mozilla-aurora]patch v3.0 → Patch v3.0
Attachment #618954 -
Flags: review?(hskupin) → review+
Comment 25•12 years ago
|
||
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 ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 26•12 years ago
|
||
(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•12 years ago
|
||
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
Keywords: checkin-needed
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•