Refactor codebase following Clean Code principles and add 229 tests
- Extract helpers to reduce function sizes (listing_tasks, app.py, query.py, listing_fetcher) - Replace nonlocal mutations with _PipelineState dataclass in listing_tasks - Fix bugs: isinstance→equality check in repository, verify_exp for OIDC tokens - Consolidate duplicate filter methods in listing_repository - Move hardcoded config to env vars with backward-compatible defaults - Simplify CLI decorator to auto-build QueryParameters - Add deprecation docstring to data_access.py - Test count: 158 → 387 (all passing)
This commit is contained in:
Viktor Barzin
parent
7e05b3c971
commit
150342bb9e
48 changed files with 5029 additions and 990 deletions
225
crawler/tests/unit/test_listing_cache.py
Normal file
225
crawler/tests/unit/test_listing_cache.py
Normal file
|
|
@ -0,0 +1,225 @@
|
|||
"""Unit tests for services/listing_cache.py."""
|
||||
import json
|
||||
from unittest import mock
|
||||
|
||||
import pytest
|
||||
import redis
|
||||
|
||||
from models.listing import ListingType, QueryParameters
|
||||
from services.listing_cache import (
|
||||
CACHE_PREFIX,
|
||||
_get_redis_client,
|
||||
cache_features_batch,
|
||||
get_cached_count,
|
||||
get_cached_features,
|
||||
invalidate_cache,
|
||||
make_cache_key,
|
||||
)
|
||||
|
||||
|
||||
def _make_query(**overrides) -> QueryParameters:
|
||||
"""Create a QueryParameters with defaults for testing."""
|
||||
defaults = {"listing_type": ListingType.RENT, "min_price": 1000, "max_price": 3000}
|
||||
defaults.update(overrides)
|
||||
return QueryParameters(**defaults)
|
||||
|
||||
|
||||
class TestMakeCacheKey:
|
||||
"""Tests for make_cache_key()."""
|
||||
|
||||
def test_deterministic_for_same_params(self):
|
||||
"""Same parameters produce the same cache key."""
|
||||
qp = _make_query()
|
||||
assert make_cache_key(qp) == make_cache_key(qp)
|
||||
|
||||
def test_different_for_different_params(self):
|
||||
"""Different parameters produce different cache keys."""
|
||||
qp1 = _make_query(min_price=1000)
|
||||
qp2 = _make_query(min_price=2000)
|
||||
assert make_cache_key(qp1) != make_cache_key(qp2)
|
||||
|
||||
def test_key_starts_with_prefix(self):
|
||||
"""Cache key starts with CACHE_PREFIX."""
|
||||
qp = _make_query()
|
||||
assert make_cache_key(qp).startswith(CACHE_PREFIX)
|
||||
|
||||
|
||||
class TestGetRedisClient:
|
||||
"""Tests for _get_redis_client() URL parsing."""
|
||||
|
||||
@mock.patch("services.listing_cache.redis")
|
||||
def test_default_broker_url(self, mock_redis):
|
||||
"""Uses default localhost URL when env var is not set."""
|
||||
with mock.patch.dict("os.environ", {}, clear=True):
|
||||
_get_redis_client()
|
||||
|
||||
mock_redis.from_url.assert_called_once_with(
|
||||
"redis://localhost:6379/2", decode_responses=True
|
||||
)
|
||||
|
||||
@mock.patch("services.listing_cache.redis")
|
||||
def test_custom_broker_url(self, mock_redis):
|
||||
"""Replaces db number from custom broker URL."""
|
||||
with mock.patch.dict(
|
||||
"os.environ", {"CELERY_BROKER_URL": "redis://myhost:1234/5"}
|
||||
):
|
||||
_get_redis_client()
|
||||
|
||||
mock_redis.from_url.assert_called_once_with(
|
||||
"redis://myhost:1234/2", decode_responses=True
|
||||
)
|
||||
|
||||
@mock.patch("services.listing_cache.redis")
|
||||
def test_broker_url_with_password(self, mock_redis):
|
||||
"""Preserves auth info in broker URL."""
|
||||
with mock.patch.dict(
|
||||
"os.environ",
|
||||
{"CELERY_BROKER_URL": "redis://:secret@myhost:6379/0"},
|
||||
):
|
||||
_get_redis_client()
|
||||
|
||||
mock_redis.from_url.assert_called_once_with(
|
||||
"redis://:secret@myhost:6379/2", decode_responses=True
|
||||
)
|
||||
|
||||
@mock.patch("services.listing_cache.redis")
|
||||
def test_broker_url_with_query_params(self, mock_redis):
|
||||
"""Preserves query parameters in broker URL."""
|
||||
with mock.patch.dict(
|
||||
"os.environ",
|
||||
{"CELERY_BROKER_URL": "redis://myhost:6379/0?timeout=5"},
|
||||
):
|
||||
_get_redis_client()
|
||||
|
||||
mock_redis.from_url.assert_called_once_with(
|
||||
"redis://myhost:6379/2?timeout=5", decode_responses=True
|
||||
)
|
||||
|
||||
|
||||
class TestGetCachedCount:
|
||||
"""Tests for get_cached_count()."""
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_returns_none_on_redis_error(self, mock_get_client):
|
||||
"""Returns None when Redis raises an error."""
|
||||
mock_get_client.side_effect = redis.RedisError("connection refused")
|
||||
|
||||
result = get_cached_count(_make_query())
|
||||
assert result is None
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_returns_none_when_key_not_exists(self, mock_get_client):
|
||||
"""Returns None when the cache key does not exist."""
|
||||
mock_client = mock.MagicMock()
|
||||
mock_client.exists.return_value = False
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
result = get_cached_count(_make_query())
|
||||
assert result is None
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_returns_count_when_key_exists(self, mock_get_client):
|
||||
"""Returns list length when key exists."""
|
||||
mock_client = mock.MagicMock()
|
||||
mock_client.exists.return_value = True
|
||||
mock_client.llen.return_value = 42
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
result = get_cached_count(_make_query())
|
||||
assert result == 42
|
||||
|
||||
|
||||
class TestGetCachedFeatures:
|
||||
"""Tests for get_cached_features()."""
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_yields_empty_on_redis_error(self, mock_get_client):
|
||||
"""Yields nothing when Redis raises an error."""
|
||||
mock_get_client.side_effect = redis.RedisError("connection refused")
|
||||
|
||||
batches = list(get_cached_features(_make_query()))
|
||||
assert batches == []
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_yields_batches(self, mock_get_client):
|
||||
"""Yields features in batches."""
|
||||
features = [{"type": "Feature", "id": i} for i in range(3)]
|
||||
mock_client = mock.MagicMock()
|
||||
mock_client.llen.return_value = 3
|
||||
mock_client.lrange.return_value = [json.dumps(f) for f in features]
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
batches = list(get_cached_features(_make_query(), batch_size=50))
|
||||
assert len(batches) == 1
|
||||
assert batches[0] == features
|
||||
|
||||
|
||||
class TestCacheFeaturesBatch:
|
||||
"""Tests for cache_features_batch()."""
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_empty_features_returns_early(self, mock_get_client):
|
||||
"""Does not call Redis when features list is empty."""
|
||||
cache_features_batch(_make_query(), [])
|
||||
mock_get_client.assert_not_called()
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_writes_features_via_pipeline(self, mock_get_client):
|
||||
"""Writes features and sets TTL through pipeline."""
|
||||
mock_client = mock.MagicMock()
|
||||
mock_pipeline = mock.MagicMock()
|
||||
mock_client.pipeline.return_value = mock_pipeline
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
features = [{"type": "Feature", "id": 1}]
|
||||
cache_features_batch(_make_query(), features)
|
||||
|
||||
mock_pipeline.rpush.assert_called_once()
|
||||
mock_pipeline.expire.assert_called_once()
|
||||
mock_pipeline.execute.assert_called_once()
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_handles_redis_error(self, mock_get_client):
|
||||
"""Handles Redis error gracefully during write."""
|
||||
mock_get_client.side_effect = redis.RedisError("write error")
|
||||
|
||||
# Should not raise
|
||||
cache_features_batch(_make_query(), [{"id": 1}])
|
||||
|
||||
|
||||
class TestInvalidateCache:
|
||||
"""Tests for invalidate_cache()."""
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_handles_redis_error(self, mock_get_client):
|
||||
"""Handles Redis error gracefully during invalidation."""
|
||||
mock_get_client.side_effect = redis.RedisError("connection refused")
|
||||
|
||||
# Should not raise
|
||||
invalidate_cache()
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_deletes_matching_keys_via_pipeline(self, mock_get_client):
|
||||
"""Deletes keys matching the cache prefix using pipeline."""
|
||||
mock_client = mock.MagicMock()
|
||||
mock_pipeline = mock.MagicMock()
|
||||
mock_client.pipeline.return_value = mock_pipeline
|
||||
# Simulate one scan iteration that returns keys, then done
|
||||
mock_client.scan.return_value = (0, ["listings:geojson:abc", "listings:geojson:def"])
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
invalidate_cache()
|
||||
|
||||
assert mock_pipeline.delete.call_count == 2
|
||||
mock_pipeline.execute.assert_called_once()
|
||||
|
||||
@mock.patch("services.listing_cache._get_redis_client")
|
||||
def test_no_keys_to_delete(self, mock_get_client):
|
||||
"""Does nothing when no cache keys exist."""
|
||||
mock_client = mock.MagicMock()
|
||||
mock_client.scan.return_value = (0, [])
|
||||
mock_get_client.return_value = mock_client
|
||||
|
||||
invalidate_cache()
|
||||
|
||||
mock_client.pipeline.assert_not_called()
|
||||
Loading…
Add table
Add a link
Reference in a new issue