Closed Bug 631511 Opened 10 years ago Closed 10 years ago

Test failure "Failed for 'subject.faviconFromAMO == true'" in testGreenLarry.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: whimboo, Assigned: u279076)

References

()

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(3 files)

Module:    testSecurity/testGreenLarry.js
Test:      testLarryGreen    
Failure:   controller.assertJS: Failed for 'subject.faviconFromAMO == true'
Branches:  default (older branches not tested)
Platforms: Linux

Seems like every build on Linux is affected by this bug. Identified during the all locales test-run for beta 11 builds on qa-horus.
It looks like the test is expecting the favicon to be hosted at addons.mozilla.org but it's being served from https://static.addons.mozilla.net/media/img/favicon.ico

It could be a simple case of updating the test, but would like to see confirmation that this new behavior is expected.
We have regressions like this every time AMO pushes a new release.  We should really find a more reliable way to do this than having to refactor the tests every time AMO does a new release.  This is in my area so I'll assign it to myself.  However, any ideas here for how we can avoid this constant refactoring are much appreciated.
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Well, do we really need to check the favicon? If yes, why aren't we doing this on every other test that uses online content.  In the test we check the certificate, the site-identify box,  other areas and elements related to the security test itself, why the favicon?
Refactor the AMO Favicon assertion.
Attachment #510333 - Flags: review?(aaron.train)
(In reply to comment #4)
> Well, do we really need to check the favicon? If yes, why aren't we doing this
> on every other test that uses online content.  In the test we check the
> certificate, the site-identify box,  other areas and elements related to the
> security test itself, why the favicon?

It's a check for good measure, ie. making sure that a favicon displays on a EVCert secured site.
(In reply to comment #5)
> Created attachment 510333 [details] [diff] [review]
> Patch v1
> 
> Refactor the AMO Favicon assertion.

Additional note, we now check for "addons.mozilla" in the favicon URL.  This should make it more robust for future URL changes.
Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Looks fine, but I still question the need for the check.
Attachment #510333 - Flags: review?(aaron.train) → review+
Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Geo, can you quickly spotcheck this patch before I check it in? Thanks.
Attachment #510333 - Flags: review?(gmealer)
Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Looks fine. r+
Attachment #510333 - Flags: review?(gmealer) → review+
Re: Aaron's concern, this is sort of why I prefer having the improvements made to the manual version, then reflected in the automation. 

It's probably good to do the check, but there's no documentation anywhere as to why we're doing it and what the check is. Instead you have to read a bunch of code and analyze it to understand the test.

Which is to say, I wouldn't refuse a code patch because it's doing extra checks, but do consider that if we go too far down that road it can dilute one of our big advantages over mochitest for QC: understanding precisely what our coverage is and how it matches product requirements.
Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9301bd8cadde [default]
Attachment #510333 - Attachment description: Patch v1 → Patch v1 [checked-in]
Backport changes to mozilla1.9.2 and mozilla1.9.1
Attachment #510415 - Flags: review?(aaron.train)
Attachment #510415 - Flags: review?(aaron.train) → review+
Comment on attachment 510415 [details] [diff] [review]
Patch v1 (1.9.2) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7fbc38af19e0 [mozilla1.9.2]
Attachment #510415 - Attachment description: Patch v1 (backport) → Patch v1 (1.9.2) [checked-in]
Backport for mozilla1.9.1
Attachment #510494 - Flags: review?(aaron.train)
Attachment #510494 - Flags: review?(aaron.train) → review+
Comment on attachment 510494 [details] [diff] [review]
Patch v1 (1.9.1) [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/74e4ee6684ef [mozilla1.9.1]
Attachment #510494 - Attachment description: Patch v1 (1.9.1) → Patch v1 (1.9.1) [checked-in]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #3)
> We have regressions like this every time AMO pushes a new release.  We should

That's not really true. AMO pushes new releases each week and things like that happen not that often. Also take into account that some failures were real AMO issues, which we have found with Mozmill and not our Selenium tests.

But to answer that kinda question again, once we have the shadow server in-place, we can finally move to httpd and don't have to rely on AMO.
Error free today...marking verified.
Status: RESOLVED → VERIFIED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.