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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

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})

Comment 1

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

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?

Comment 2

[ZhaoGang]

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

Comment 3

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

Working examples for certificate overrides can be found here:

https://dxr.mozilla.org/mozilla-central/rev/a6d29e9432f5f88a941c0ea5284cb082f34bd097/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py#170

So we can indeed remove this specific test module.

Comment 4

[Andreas Tolfsen ❲:ato❳]

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.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1325079 - Delete unused test_certificates.py;

The test was accidentally committed at some earlier
point (hard to research due to hg not tracking file
renamings) and the functionality is sufficiently tested by
testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py:170.

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

Comment 6

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

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.

Comment 7

[Andreas Tolfsen ❲:ato❳]

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 8

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

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 9

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

Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

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

Comment 10

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

Bug 1323770 has been fixed so we can finish this patch up. Thanks.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/101356/diff/1-2/

Comment 12

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

Comment on attachment 8822436 [details]
Bug 1325079 - Delete unused test_certificates.py;

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddb344719745
Delete unused test_certificates.py; r=whimboo

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddb344719745

Comment 15

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

It also needs to be removed from the code base on aurora. So please uplift this test-only change. Thanks.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f79cc643a90