Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

missing pytest.mark.skipif on TestSSLConnectionURLParsing #26

Closed
kjaymiller opened this issue Jun 27, 2024 · 1 comment · Fixed by #36
Closed

missing pytest.mark.skipif on TestSSLConnectionURLParsing #26

kjaymiller opened this issue Jun 27, 2024 · 1 comment · Fixed by #36

Comments

@kjaymiller
Copy link
Contributor

Version: 5.1.0b7

Platform: Python 3.12 / MacOS Sonoma in M1 Arm Device

Description:

There is an inconsistent skip on testing if SSL isn't installed between the connection pool in the sync and async tests.

These tests are otherwise identical (pointing to my suggested issue #25)

sync:
https://github.com/valkey-io/valkey-py/blob/d116aa6efa03877a0c37598139a556554ad47c13/tests/test_connection_pool.py#L456C1-L494C1

async:

class TestSSLConnectionURLParsing:
def test_host(self):
pool = valkey.ConnectionPool.from_url("valkeys://my.host")
assert pool.connection_class == valkey.SSLConnection
assert pool.connection_kwargs == {"host": "my.host"}
def test_cert_reqs_options(self):
import ssl
class DummyConnectionPool(valkey.ConnectionPool):
def get_connection(self, *args, **kwargs):
return self.make_connection()
pool = DummyConnectionPool.from_url("valkeys://?ssl_cert_reqs=none")
assert pool.get_connection("_").cert_reqs == ssl.CERT_NONE
pool = DummyConnectionPool.from_url("valkeys://?ssl_cert_reqs=optional")
assert pool.get_connection("_").cert_reqs == ssl.CERT_OPTIONAL
pool = DummyConnectionPool.from_url("valkeys://?ssl_cert_reqs=required")
assert pool.get_connection("_").cert_reqs == ssl.CERT_REQUIRED
pool = DummyConnectionPool.from_url("valkeys://?ssl_check_hostname=False")
assert pool.get_connection("_").check_hostname is False
pool = DummyConnectionPool.from_url("valkeys://?ssl_check_hostname=True")
assert pool.get_connection("_").check_hostname is True

Unless there is a reason the async client should not skip the tests (or a particular reason the synchronous one should) is it not better to make the tests test the same thing the same way (or implement #25 and condense that logic into a module that both versions use which would eliminate the need for testing in both areas.

@aiven-sal
Copy link
Member

Yes, the skipif should be added to the async test as well, but I'd keep the 2 tests separate. The tests should not know/care if valkey and valkey.asyncio share some implementation detail or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants