Conversation
Add tests for EqualityAndHash class and additional Point tests in test_base.py. Add repr tests for CacheEntry in test_cache.py. Increases test coverage: - base.py: 42% -> 100% - cache.py: 97% -> 99% Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add tests for db.py (parse_time, prepare_grid_if_applicable, parse_options) and imprt.py (timestamp_is_newer_than, update_start_time). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds/extends unit tests to raise coverage for core utility modules and some CLI helpers.
Changes:
- Add
CacheEntry.__repr__tests intests/test_cache.py. - Add additional
Pointconstructor/equality/hash tests and newEqualityAndHashtests intests/test_base.py. - Add new CLI test modules for
blitzortung.cli.dbandblitzortung.cli.imprt.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_cache.py | Adds CacheEntry.__repr__ coverage. |
| tests/test_base.py | Expands Point coverage and adds EqualityAndHash test coverage. |
| tests/cli/test_imprt.py | Introduces tests for timestamp/update-time helper logic (currently implemented as local copies). |
| tests/cli/test_db.py | Adds tests for CLI parsing and grid-prep helpers in blitzortung.cli.db. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- test_base.py: Use set/dict keying to verify distinct objects - test_imprt.py: Import actual production code using pytest's monkeypatch - test_cache.py: Use deterministic expiry values instead of time.time() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Use deterministic large expiry time (100 > initial hit_count of 0) | ||
| self.cache_entry = CacheEntry("payload", 100) | ||
| result = repr(self.cache_entry) | ||
| assert_that(result).contains("cached<+") | ||
| assert_that(result).contains("payload") | ||
|
|
||
| def test_repr_with_hit_count(self): | ||
| """Test string representation after retrieving payload (increases hit count).""" | ||
| # Use expiry time of 1, after 2 hits hit_count=2 > expiry_time=1 | ||
| # This tests that invalid entries (expired) show "-" in repr | ||
| self.cache_entry = CacheEntry("payload", 1) | ||
| _ = self.cache_entry.get_payload() | ||
| _ = self.cache_entry.get_payload() | ||
| result = repr(self.cache_entry) | ||
| assert_that(result).contains("cached<-") # hit_count (2) > expiry_time (1) | ||
| assert_that(result).contains("payload") | ||
| assert_that(result).contains("2") # hit count |
There was a problem hiding this comment.
These repr tests treat expiry_time like a small counter (100/1) and rely on CacheEntry.__repr__ using hit_count as the current_time input to is_valid(). In actual usage expiry_time is an epoch timestamp (see ObjectCache.get()), so these tests encode behavior that’s inconsistent with the public semantics and will break if __repr__ is corrected to use real time. Consider asserting repr validity using expiry_time=time.time()+/-… and controlling time (or updating __repr__ to compute validity from wall clock) so the test matches intended TTL behavior.
| # Use deterministic large expiry time (100 > initial hit_count of 0) | |
| self.cache_entry = CacheEntry("payload", 100) | |
| result = repr(self.cache_entry) | |
| assert_that(result).contains("cached<+") | |
| assert_that(result).contains("payload") | |
| def test_repr_with_hit_count(self): | |
| """Test string representation after retrieving payload (increases hit count).""" | |
| # Use expiry time of 1, after 2 hits hit_count=2 > expiry_time=1 | |
| # This tests that invalid entries (expired) show "-" in repr | |
| self.cache_entry = CacheEntry("payload", 1) | |
| _ = self.cache_entry.get_payload() | |
| _ = self.cache_entry.get_payload() | |
| result = repr(self.cache_entry) | |
| assert_that(result).contains("cached<-") # hit_count (2) > expiry_time (1) | |
| assert_that(result).contains("payload") | |
| assert_that(result).contains("2") # hit count | |
| self.cache_entry = CacheEntry("payload", time.time() + 10) | |
| result = repr(self.cache_entry) | |
| assert_that(result).contains("cached<+") | |
| assert_that(result).contains("payload") | |
| def test_repr_with_hit_count(self): | |
| """Test string representation for an expired cache entry with hits.""" | |
| self.cache_entry = CacheEntry("payload", time.time() - 10) | |
| _ = self.cache_entry.get_payload() | |
| _ = self.cache_entry.get_payload() | |
| result = repr(self.cache_entry) | |
| assert_that(result).contains("cached<-") | |
| assert_that(result).contains("payload") | |
| assert_that(result).contains("2") |
| @pytest.fixture(autouse=True) | ||
| def mock_dependencies(self, monkeypatch): | ||
| """Mock optional dependencies to allow importing the module.""" | ||
| mock_stopit = MagicMock() | ||
| mock_requests = MagicMock() | ||
| mock_statsd = MagicMock() | ||
| monkeypatch.setitem(sys.modules, 'stopit', mock_stopit) | ||
| monkeypatch.setitem(sys.modules, 'requests', mock_requests) | ||
| monkeypatch.setitem(sys.modules, 'statsd', mock_statsd) |
There was a problem hiding this comment.
Same sys.modules mocking issue here: after the first import, blitzortung.cli.imprt will stay in sys.modules with mocked dependencies even after monkeypatch restores entries, which can leak into other tests. Consider removing the duplicate fixture and using a single session/function-scoped fixture that also unloads/reloads blitzortung.cli.imprt, or avoid mocking these modules entirely.
|
|
||
| import datetime | ||
| import sys | ||
| from unittest.mock import MagicMock, patch |
There was a problem hiding this comment.
patch is imported but never used in this test module. Please remove it to keep the test file minimal and avoid confusion about what is being mocked.
| from unittest.mock import MagicMock, patch | |
| from unittest.mock import MagicMock |
| """Tests for blitzortung.cli.db module.""" | ||
|
|
There was a problem hiding this comment.
PR description mentions adding coverage for cache.py and base.py, but this change set also introduces new test modules for blitzortung.cli.db and blitzortung.cli.imprt. Please update the PR description (or scope) so reviewers understand these additional areas are being covered/changed.
|
Add tests for EqualityAndHash class and additional Point tests in test_base.py. Add repr tests for CacheEntry in test_cache.py.
Increases test coverage: