Initial unit test coverage of shared tlsserver module.#491
Initial unit test coverage of shared tlsserver module.#491jonasbardino wants to merge 3 commits intonextfrom
Conversation
jonasbardino
added
the
test-only
…us key and self-signed certificate for most tests. Implement variations to test the effect of toggling function hardening argument values. Move the original tests using Mock into their own class and disbale the tests that won't work in the current form. Main coverage is the built-in `ssl` module now that our PyOpenSSL dependency is likely to dwindle.
jonasbardino
force-pushed
the
test/add-tlsserver-unit-tests
branch
from
830f32f to
2b9ef68
Compare
|
I'm not sure if we should simply drop the Mock-based tests as they perhaps add little now that we have proper Also not sure if it would be better to move the inline dummy test host key and certificate to some fixture / state file or generate them on demand instead. |
| """Prepare isolated test config""" | ||
| return 'testconfig' | ||
|
|
||
| def _prepare_key_cert(self, key_path, cert_path): |
There was a problem hiding this comment.
If I read correctly this is used by every test here - as such, I think it might help to just make this a before_each function. If doing so one can than tighten the TestCase name to something like ...Tlsserver_with_existing_certs, perhaps include "hardened" if that is actually what these all check.
|
|
||
| def test_hardened_ssl_context_basic(self): | ||
| """Test basic SSL context creation with default parameters""" | ||
| with patch('mig.shared.tlsserver.ssl') as mock_ssl: |
There was a problem hiding this comment.
These patch blocks are very jarring relative to what we do in most other tests. I'll note its also quit a sudden use of patch and MagicMock, which there may be no issue with but does stand out.
Our typical approach across the suite is to have an explicitly named function that prepares the stubbed and/or mocked thing and returns it wholesale. There are a couple of reasons for this - only mocking some parts of something might cause problems later as the code evolves, and a single "prepare" style function serves not just to keep us honest but also a central place to ensure sane default behaviours (for example when checking network requests, if one has not explicitly specified a response it will generate an invalid one in a way that can be tracked down).
Hopefully that explains the purpose. Thus, let me offer some suggestions to make this consistent with the rest of the test suite:
- define a single
prepare_mock_sslfunction that returns, in its entirety, a substitute 'mig.shared.tlsserver.ssl' object
def prepare_mock_ssl():
mock_ssl = {}
mock_ssl.PROTOCOL_SSLv23 = 1
mock_ssl.SSLContext = MagicMock()
mock_ssl.SSLContext.return_value = MagicMock()
return mock_ssl
- use patch as a function decorator on the test case rather than as a with, and if my reading of the documentation is correct the
new_callable=keyboard arg can specified as the defined prepare function.
| config = self.configuration | ||
| config.logger = self.logger | ||
|
|
||
| with self.assertLogs(level="INFO") as log_capture: |
There was a problem hiding this comment.
I'm a bit confused as to why this is needed? We don't appear to do anything with what is captured here.
|
|
||
| # PyOpenSSL is required for hardened_openssl_context tests | ||
| try: | ||
| import OpenSSL |
There was a problem hiding this comment.
If this whole test cannot run without the presence of this library I think the entire test case should be skipped if it is not present. It should be sufficient to call skipTest() within before_each when encournering this (see https://docs.python.org/3/library/unittest.html#unittest.TestCase.skipTest).
albu-diku
left a comment
albu-diku
left a comment
There was a problem hiding this comment.
Have left some comments. The test exercises some good things but I think it needs some work to make it much more consistent with the rest of the test suite.
2 participants
Generated simple tests to cover some of the basic functionality before we start tightening to python3 and perhaps use TLS 1.3 as minimum version.