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.

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

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.

Review commit: https://reviewboard.mozilla.org/r/50405/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50405/

Review commit: https://reviewboard.mozilla.org/r/50407/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50407/

Review commit: https://reviewboard.mozilla.org/r/50409/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50409/

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?

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" ? :-\

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

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

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.

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/

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a089b9813a https://hg.mozilla.org/integration/mozilla-inbound/rev/974d89198e8c https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba069d19cf1

https://hg.mozilla.org/mozilla-central/rev/e7a089b9813a https://hg.mozilla.org/mozilla-central/rev/974d89198e8c https://hg.mozilla.org/mozilla-central/rev/8ba069d19cf1

Tests are all fixed. Lets get them backported to mozilla-aurora.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef57089630e6 https://hg.mozilla.org/releases/mozilla-aurora/rev/11be3fa64aa0 https://hg.mozilla.org/releases/mozilla-aurora/rev/0e7f650568bb