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

Status: VERIFIED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

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.

Comment 1

[Aaron Train [:aaronmt]]

This has blown up across all platforms

http://mozmill-release.brasstacks.mozilla.com/#/general/failure?branch=4.0&from=2011-01-31&to=2011-02-07&test=firefox%2FtestSecurity%2FtestGreenLarry.js&func=testLarryGreen

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 3

[u279076]

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.

Comment 4

[Aaron Train [:aaronmt]]

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?

Comment 5

[u279076]

Attachment: Patch v1 [checked-in]

Refactor the AMO Favicon assertion.

Comment 6

[u279076]

(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.

Comment 7

[u279076]

(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 8

[Aaron Train [:aaronmt]]

Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Looks fine, but I still question the need for the check.

Comment 9

[u279076]

Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Geo, can you quickly spotcheck this patch before I check it in? Thanks.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Looks fine. r+

Comment 11

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

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 12

[u279076]

Comment on attachment 510333 [details] [diff] [review]
Patch v1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/9301bd8cadde [default]

Comment 13

[u279076]

Attachment: Patch v1 (1.9.2) [checked-in]

Backport changes to mozilla1.9.2 and mozilla1.9.1

Comment 14

[u279076]

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]

Comment 15

[u279076]

Attachment: Patch v1 (1.9.1) [checked-in]

Backport for mozilla1.9.1

Comment 16

[u279076]

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]

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(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.

Comment 18

[u279076]

Error free today...marking verified.