Closed Bug 1270040 Opened 4 years ago Closed 4 years ago

Various test failures for security tests due to changes in Firefox

Categories

(Testing :: Firefox UI Tests, defect)

48 Branch
defect
Not set
normal

Tracking

(firefox47 unaffected, firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(3 files)

I see lots of test failures for our certificate tests under the security folder:

identity-popup-content-host
---------------------------
TEST-UNEXPECTED-ERROR | test_dv_certificate.py TestDVCertificate.test_dv_cert | NoSuchElementException: NoSuchElementException: Unable to locate element: identity-popup-content-host
TEST-UNEXPECTED-ERROR | test_ev_certificate.py TestEVCertificate.test_ev_certificate | NoSuchElementException: NoSuchElementException: Unable to locate element: identity-popup-content-host

technicalContentText
--------------------
TEST-UNEXPECTED-ERROR | test_security_notification.py TestSecurityNotification.test_invalid_cert | NoSuchElementException: NoSuchElementException: Unable to locate element: technicalContentText 

cert_domain_link
----------------
TEST-UNEXPECTED-ERROR | test_unknown_issuer.py TestUnknownIssuer.test_unknown_issuer | NoSuchElementException:
NoSuchElementException: Unable to locate element: cert_domain_link

errorTitleText
--------------
TEST-UNEXPECTED-ERROR | test_ssl_disabled_error_page.py TestSSLDisabledErrorPage.test_ssl_disabled_error_page | NoSuchElementException:
NoSuchElementException: Unable to locate element: errorTitleText

All seem to be happening due to the merge of certerror and neterror pages as done on bug 1240594. I have to do more investigation to find out what's expected and what not.
As I mentioned in Bug 1240594 comment 42, it seems more likely that this is a consequence of Bug 1261936.
Some of these look like assumptions (e.g. #errorTitleText being a thing) that the merge broke, but as Cykesiopka said, others just look like expected changes in behaviour based on network changes.
Blocks: 1261936
(In reply to :Cykesiopka from comment #1)
> As I mentioned in Bug 1240594 comment 42, it seems more likely that this is
> a consequence of Bug 1261936.

Thanks! Looks like I missed that one bug. So one question...

> This looks like Bug 1261936. In particular, note that
> https://ssl-unknownissuer.mozqa.com uses a cert that only has a Common Name
> and no Subject Alternative Name entries. For certs like these, this is
> exactly the behaviour that is expected.

Is there something which we should change for this certificate for broader testing or is it fine as it is? If we keep it as it is, I would say we only check for the error code.
Flags: needinfo?(cykesiopka.bmo)
Summary: Various test failures due to merge of cert error page → Various test failures for security tests due to changes in Firefox
Ok, I fixed nearly all failures. The two I still have to do are the ones for the missing 'identity-popup-content-host' element. I checked Treeherder and figured out that it started with the landing of bug 1207619.

I wish that someone else would have had a look at our test results on Treeherder while I was away for a month. Now that I'm back I see a lot of bustage. So excuse the single bug for three different regressions.
Assignee: nobody → hskupin
Blocks: 1207619
Status: NEW → ASSIGNED
Comment on attachment 8748594 [details]
MozReview Request: Bug 1270040 - Fix host usage in control center for fx ui tests. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50409/diff/1-2/
Comment on attachment 8748593 [details]
MozReview Request: Bug 1270040 - Fix fx ui tests regressions for certerror page merge. r=gijs

https://reviewboard.mozilla.org/r/50407/#review47207

::: testing/firefox-ui/tests/functional/security/test_unknown_issuer.py
(Diff revision 1)
> -            # Check for the correct error code
> -            error = self.marionette.find_element(By.ID, 'errorCode')
> -            self.assertEquals(error.get_attribute('textContent'),
> -                              'SEC_ERROR_UNKNOWN_ISSUER')

Why remove this?
Attachment #8748593 - Flags: review?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/50407/#review47207

> Why remove this?

We have the same check for technicalContentText some lines below. I don't see why we have to check it twice for our kind of test. I'm sure that the errorcode is covered by some unit test, or not?
Comment on attachment 8748594 [details]
MozReview Request: Bug 1270040 - Fix host usage in control center for fx ui tests. r=gijs

https://reviewboard.mozilla.org/r/50409/#review47209

rs=me . I would fully expect this to break again when we next change DOM structures though - what's the timeline to convert all of these tests to stable marionette tests that run as part of regular tier-1 automation?

::: testing/firefox-ui/tests/functional/security/test_ev_certificate.py:57
(Diff revision 2)
>  
>          # Check the idenity popup doorhanger
>          self.assertEqual(self.identity_popup.element.get_attribute('connection'), 'secure-ev')
>  
>          # For EV certificates no hostname but the organization name is shown
> -        self.assertEqual(self.identity_popup.host.get_attribute('value'),
> +        self.assertEqual(self.identity_popup.view.main.host.get_attribute('textContent'),

Who thought it was a good idea to name an API that fetches properties instead of attributes "get_attribute" ? :-\
Attachment #8748594 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8748593 [details]
MozReview Request: Bug 1270040 - Fix fx ui tests regressions for certerror page merge. r=gijs

https://reviewboard.mozilla.org/r/50407/#review47215
Attachment #8748593 - Flags: review+
(In reply to Henrik Skupin (:whimboo) from comment #10)
> https://reviewboard.mozilla.org/r/50407/#review47207
> 
> > Why remove this?
> 
> We have the same check for technicalContentText some lines below. I don't
> see why we have to check it twice for our kind of test. I'm sure that the
> errorcode is covered by some unit test, or not?

Then maybe we should just remove the entire test, if we already have mochitest coverage?
https://reviewboard.mozilla.org/r/50409/#review47209

Those are already Marionette tests and we report to Treeherder. But we are tier-3 at the moment. I work towards to get at least tier-2. For current results see:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Firefox%20UI&filter-tier=2&filter-tier=3

> Who thought it was a good idea to name an API that fetches properties instead of attributes "get_attribute" ? :-\

I'm also not a fan of that, but that is how Marionette works at the moment.
(In reply to :Gijs Kruitbosch from comment #13)
> Then maybe we should just remove the entire test, if we already have
> mochitest coverage?

All of our security tests are actually running against real websites with real certificates. That's not something what we have with Mochitest.
(In reply to Henrik Skupin (:whimboo) from comment #15)
> (In reply to :Gijs Kruitbosch from comment #13)
> > Then maybe we should just remove the entire test, if we already have
> > mochitest coverage?
> 
> All of our security tests are actually running against real websites with
> real certificates. That's not something what we have with Mochitest.

Real websites in what sense? They're still on a local server, right? Mochitest does the same thing...
(In reply to :Gijs Kruitbosch from comment #16)
> Real websites in what sense? They're still on a local server, right?
> Mochitest does the same thing...

No, various subdomains for mozqa.com. See: http://mxr.mozilla.org/mozilla-central/search?string=mozqa.com&find=testing%2Ffirefox-ui%2Ftests%2Ffunctional%2Fsecurity&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
Comment on attachment 8748592 [details]
MozReview Request: Bug 1270040 - Fix test_unknown_issuer.py for cert domain changes. r=Cykesiopka

https://reviewboard.mozilla.org/r/50405/#review47305

r+, although it appears these changes get removed in Part 2 anyways.
Attachment #8748592 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/50407/#review47207

> We have the same check for technicalContentText some lines below. I don't see why we have to check it twice for our kind of test. I'm sure that the errorcode is covered by some unit test, or not?

Not explicitly it appears, but this is close: https://hg.mozilla.org/mozilla-central/annotate/0af3c129a366/browser/base/content/test/general/browser_aboutCertError.js#l356
(In reply to Henrik Skupin (:whimboo) from comment #3)
> Is there something which we should change for this certificate for broader
> testing or is it fine as it is? If we keep it as it is, I would say we only
> check for the error code.

The patch for Bug 1261936 already included test changes to make sure the message being shown is broadly correct, so a check for just the error code is sufficient, assuming you keep it.
Flags: needinfo?(cykesiopka.bmo)
Ok, so I will make it explicit in test_unknown_issuer.py and really check for the error code via the appropriate element. Updated patch upcoming.
Comment on attachment 8748592 [details]
MozReview Request: Bug 1270040 - Fix test_unknown_issuer.py for cert domain changes. r=Cykesiopka

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50405/diff/1-2/
Comment on attachment 8748593 [details]
MozReview Request: Bug 1270040 - Fix fx ui tests regressions for certerror page merge. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50407/diff/1-2/
Comment on attachment 8748594 [details]
MozReview Request: Bug 1270040 - Fix host usage in control center for fx ui tests. r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50409/diff/2-3/
Tests are all fixed. Lets get them backported to mozilla-aurora.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.