Failing unit tests in test_certificate.py (test_block_insecure_sites, test_accept_all_insecure, test_accept_some_insecure)

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: whimboo, Assigned: ato)

Tracking

Version 3
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment)

While working on bug 1323770 to correct skip decorators in Marionette I noticed that the test test_certificate.py was not included in the unit-tests.ini file and as such has never been run in automation.

The following tests are broken:

TEST-UNEXPECTED-ERROR | test_certificates.py TestCertificates.test_accept_all_insecure | InsecureCertificateException
Traceback (most recent call last):
  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 166, in run
    testMethod()
  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/tests/unit/test_certificates.py", line 24, in test_accept_all_insecure
    self.marionette.navigate(self.fixtures.where_is("test.html", on="https"))
  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 1637, in navigate
    self._send_message("get", {"url": url})

TEST-UNEXPECTED-ERROR | test_certificates.py TestCertificates.test_block_insecure_sites | InsecureCertificateException
Traceback (most recent call last):
  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 166, in run
    testMethod()
  File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/tests/unit/test_certificates.py", line 19, in test_block_insecure_sites
    self.marionette.navigate(self.fixtures.where_is("test.html", on="https"))
  File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 1637, in navigate
    self._send_message("get", {"url": url})
Flags: needinfo?(ato)
Summary: Failing unit tests in test_certificate.py (test_block_insecure_sites, test_accept_all_insecure) → Failing unit tests in test_certificate.py (test_block_insecure_sites, test_accept_all_insecure, test_accept_some_insecure)
I think the tests are broken in different ways:

* They are using acceptSslCerts instead of acceptInsecureCerts
* They are using a desiredCapability dict under capabilities which will not result in the correct behavior in creating the session
* Some other smaller issues

I wonder if this test file has been created in an early state of the TLS work and you missed to update it? Maybe we can even get rid of it completely due to the coverage in test_navigation.py and test_capabilities.py?
(In reply to Henrik Skupin (:whimboo) from comment #1)
> I think the tests are broken in different ways:
> 
> * They are using acceptSslCerts instead of acceptInsecureCerts
> * They are using a desiredCapability dict under capabilities which will not
> result in the correct behavior in creating the session
> * Some other smaller issues

Cloud you point out What is the correct capability?
whimboo’s research is correct.  I’m not quite sure how I ended up including this in an earlier patch (it wasn’t in bug 1103196 apparently), but it is correct that this was part of some early-stage TLS certificate work and should never have been committed.  Thankfully it wasn’t listed in the test manifest and consequently not run on try.

All the necessary tests for TLS certificates are in test_navigation.py:170.

I’m going to use this bug to remove test_certificates.py.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

https://reviewboard.mozilla.org/r/101356/#review101840

You will have to remove it from the unit-tests.ini file as it was added via my patch on bug 1323770. Given that this patch is on autoland only, you have to wait for a rebase once the merge to m-c happened.
Attachment #8822436 - Flags: review?(hskupin) → review-
Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

https://reviewboard.mozilla.org/r/101356/#review101840

Will do that when rebasing after bug 1323770 lands.  However, is this reason enough to r- the patch?
Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

https://reviewboard.mozilla.org/r/101356/#review101840

I will just see the full patch. If removing the r? flag makes more sense to you, I can also do that.
Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

https://reviewboard.mozilla.org/r/101356/#review101882
Attachment #8822436 - Flags: review-
Depends on: 1323770
Bug 1323770 has been fixed so we can finish this patch up. Thanks.
Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

https://reviewboard.mozilla.org/r/101356/#review102234
Attachment #8822436 - Flags: review?(hskupin) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ddb344719745
Delete unused test_certificates.py; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/ddb344719745
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
It also needs to be removed from the code base on aurora. So please uplift this test-only change. Thanks.
Whiteboard: [checkin-needed-aurora]
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f79cc643a90
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.