Improve code coverage with targeted tests #253
Conversation
Add comprehensive tests for previously uncovered code paths: - FallbackWrapper: Test all fallback scenarios (get_many, ttl, ttl_many, put_many, delete, delete_many with write_to_fallback) - FernetEncryptionWrapper: Test validation error paths for constructor arguments (fernet/source_material/salt validation) - PassthroughCacheWrapper: Test ttl and ttl_many caching behavior - serialization: Test key_must_be, parse_datetime_str, datetime format handling, and error cases - time_to_live: Test try_parse_datetime_str, prepare_entry_timestamps, and TTL validation edge cases Coverage improvements: - aio package: 78% → 79% - shared package: 85% → 93% - fallback wrapper: 71% → 100% - fernet encryption: 77% → 100% - passthrough cache: 83% → 100% - serialization.py: 77% → 100% - time_to_live.py: 75% → 100%
📝 WalkthroughWalkthroughThis pull request adds comprehensive test coverage across multiple test files in the key-value library. The changes include validation tests for FernetEncryptionWrapper initialization, expansion of the FailingStore test fixture with new async methods (get_many, ttl, ttl_many, put_many, delete, delete_many), TTL-based caching tests for PassthroughCacheWrapper, new serialization utilities (key_must_be and parse_datetime_str) with corresponding tests, and expanded test coverage for TTL handling including datetime string parsing. Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.pykey-value/key-value-aio/tests/stores/wrappers/test_fallback.pykey-value/key-value-aio/tests/stores/wrappers/test_passthrough_cache.pykey-value/key-value-shared/tests/utils/test_serialization.pykey-value/key-value-shared/tests/utils/test_time_to_live.py
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-shared/tests/utils/test_time_to_live.py (2)
key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (5)
prepare_entry_timestamps(84-93)prepare_ttl(45-45)prepare_ttl(49-49)prepare_ttl(52-65)try_parse_datetime_str(34-41)key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
InvalidTTLError(28-35)
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (3)
key-value/key-value-aio/src/key_value/aio/wrappers/prefix_collections/wrapper.py (8)
get_many(40-42)ttl(45-47)ttl_many(50-52)put(55-57)put_many(60-69)delete(72-74)delete_many(77-79)get(35-37)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
keys(86-88)MemoryStore(91-191)key-value/key-value-aio/src/key_value/aio/wrappers/fallback/wrapper.py (1)
FallbackWrapper(10-118)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (2)
key-value/key-value-aio/src/key_value/aio/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(13-89)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/encryption/fernet.py (1)
FernetEncryptionWrapper(16-79)
key-value/key-value-shared/tests/utils/test_serialization.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (5)
key_must_be(20-26)parse_datetime_str(30-35)dump_dict(114-158)load_dict(67-106)dump_json(160-184)
🪛 GitHub Actions: Run Tests
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py
[error] 194-194: No overloads for "init" match the provided arguments. Argument types: (MemoryStore, Fernet, Literal['test'])
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
key-value/key-value-aio/tests/stores/wrappers/test_encryption.py
[failure] 221-221:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Literal['test-source']) (reportCallIssue)
[failure] 215-215:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore) (reportCallIssue)
[failure] 205-205:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Fernet, Literal['test']) (reportCallIssue)
[failure] 194-194:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Fernet, Literal['test']) (reportCallIssue)
🔇 Additional comments (17)
key-value/key-value-aio/tests/stores/wrappers/test_passthrough_cache.py (3)
50-62: Test logic is correct.Validates cache-first behavior effectively. Same optional refinement as above: consider validating the TTL value range.
64-73: LGTM!Clean validation of missing key behavior.
94-107: LGTM!Validates batch cache-first behavior correctly.
key-value/key-value-shared/tests/utils/test_serialization.py (4)
6-8: LGTM: Imports support new test coverage.The new imports for error types and utility functions are necessary for the expanded test suite.
85-143: Excellent coverage of serialization adapter behaviors.These tests comprehensively cover:
- Metadata field inclusion (key/collection)
- Datetime format handling for both serialization and deserialization
- Error paths for incompatible format combinations
- Value parsing from JSON strings
- Validation errors for missing/invalid values
All assertions and error messages correctly match the implementation.
146-160: Complete coverage ofkey_must_beutility.All three code paths are tested:
- Missing key → None
- Type mismatch → TypeError with correct message format
- Valid key and type → value returned
163-172: Complete coverage ofparse_datetime_strutility.Both happy and error paths are tested:
- Valid ISO format parsing
- Invalid format raises DeserializationError
key-value/key-value-aio/tests/stores/wrappers/test_fallback.py (4)
1-1: LGTM!Import addition properly supports the new method signatures.
20-55: LGTM!The new FailingStore methods properly extend the test fixture to simulate primary store failures for bulk operations. Implementation is consistent with existing methods and correctly mirrors the FallbackWrapper API.
113-124: LGTM!Test correctly validates get_many fallback behavior when the primary store is unavailable.
126-137: LGTM!Test correctly validates ttl fallback behavior and verifies both value and TTL are returned.
key-value/key-value-shared/tests/utils/test_time_to_live.py (4)
8-8: LGTM!Import additions are correct and necessary for the new test coverage.
63-66: LGTM!Good edge case coverage for zero TTL validation.
69-72: LGTM!Good edge case coverage for negative TTL validation.
75-90: LGTM!Comprehensive coverage of all code paths in
try_parse_datetime_str: valid datetime strings, invalid strings, and non-string inputs.key-value/key-value-aio/tests/stores/wrappers/test_encryption.py (2)
180-187: LGTM: Valid constructor test for source_material and salt.This test correctly validates the happy path for creating a wrapper with source_material and salt parameters.
227-244: LGTM: Comprehensive validation tests for empty/whitespace inputs.These tests correctly verify that whitespace-only
source_materialandsaltvalues are rejected. The tests match the overload signatures (both parameters provided), so no type checking issues occur. The runtime validation properly uses.strip()to detect empty values.
| def test_fernet_cannot_provide_fernet_with_source_material(memory_store: MemoryStore): | ||
| """Test that providing both fernet and source_material raises ValueError.""" | ||
| fernet = Fernet(key=Fernet.generate_key()) | ||
| with pytest.raises(ValueError, match="Cannot provide fernet together with source_material or salt"): | ||
| FernetEncryptionWrapper( | ||
| key_value=memory_store, | ||
| fernet=fernet, | ||
| source_material="test", | ||
| ) | ||
|
|
||
|
|
||
| def test_fernet_cannot_provide_fernet_with_salt(memory_store: MemoryStore): | ||
| """Test that providing both fernet and salt raises ValueError.""" | ||
| fernet = Fernet(key=Fernet.generate_key()) | ||
| with pytest.raises(ValueError, match="Cannot provide fernet together with source_material or salt"): | ||
| FernetEncryptionWrapper( | ||
| key_value=memory_store, | ||
| fernet=fernet, | ||
| salt="test", | ||
| ) |
There was a problem hiding this comment.
Add type: ignore comments for intentional overload violations.
These tests intentionally pass invalid argument combinations to verify error handling. The type checker correctly flags them as not matching any overload signature. Add type: ignore comments to suppress the warnings.
🔎 Proposed fix
def test_fernet_cannot_provide_fernet_with_source_material(memory_store: MemoryStore):
"""Test that providing both fernet and source_material raises ValueError."""
fernet = Fernet(key=Fernet.generate_key())
with pytest.raises(ValueError, match="Cannot provide fernet together with source_material or salt"):
- FernetEncryptionWrapper(
+ FernetEncryptionWrapper( # type: ignore[call-overload]
key_value=memory_store,
fernet=fernet,
source_material="test",
)
def test_fernet_cannot_provide_fernet_with_salt(memory_store: MemoryStore):
"""Test that providing both fernet and salt raises ValueError."""
fernet = Fernet(key=Fernet.generate_key())
with pytest.raises(ValueError, match="Cannot provide fernet together with source_material or salt"):
- FernetEncryptionWrapper(
+ FernetEncryptionWrapper( # type: ignore[call-overload]
key_value=memory_store,
fernet=fernet,
salt="test",
)🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 194-194: No overloads for "init" match the provided arguments. Argument types: (MemoryStore, Fernet, Literal['test'])
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
[failure] 205-205:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Fernet, Literal['test']) (reportCallIssue)
[failure] 194-194:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Fernet, Literal['test']) (reportCallIssue)
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_encryption.py around lines
190 to 209, the two tests intentionally call FernetEncryptionWrapper with both
fernet and source_material/salt which triggers static type-checker overload
errors; add a trailing "# type: ignore[call-overload]" (or "# type: ignore" if
call-overload isn't available) to each FernetEncryptionWrapper(...) call to
suppress the type-check warnings while keeping the runtime tests unchanged.
| def test_fernet_must_provide_source_material(memory_store: MemoryStore): | ||
| """Test that not providing fernet or source_material raises ValueError.""" | ||
| with pytest.raises(ValueError, match="Must provide either fernet or source_material"): | ||
| FernetEncryptionWrapper(key_value=memory_store) |
There was a problem hiding this comment.
Add type: ignore comment for intentional overload violation.
This test intentionally omits required parameters to verify error handling. Add a type: ignore comment to suppress the type checker warning.
🔎 Proposed fix
def test_fernet_must_provide_source_material(memory_store: MemoryStore):
"""Test that not providing fernet or source_material raises ValueError."""
with pytest.raises(ValueError, match="Must provide either fernet or source_material"):
- FernetEncryptionWrapper(key_value=memory_store)
+ FernetEncryptionWrapper(key_value=memory_store) # type: ignore[call-overload]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_fernet_must_provide_source_material(memory_store: MemoryStore): | |
| """Test that not providing fernet or source_material raises ValueError.""" | |
| with pytest.raises(ValueError, match="Must provide either fernet or source_material"): | |
| FernetEncryptionWrapper(key_value=memory_store) | |
| def test_fernet_must_provide_source_material(memory_store: MemoryStore): | |
| """Test that not providing fernet or source_material raises ValueError.""" | |
| with pytest.raises(ValueError, match="Must provide either fernet or source_material"): | |
| FernetEncryptionWrapper(key_value=memory_store) # type: ignore[call-overload] |
🧰 Tools
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
[failure] 215-215:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore) (reportCallIssue)
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_encryption.py around lines
212 to 215, the test intentionally calls FernetEncryptionWrapper without
required params to assert a ValueError but the type checker raises a
call-argument error; suppress this expected type-checker warning by adding a
type ignore comment to the constructor call (e.g. append "# type:
ignore[call-arg]" or "# type: ignore" to that line) so the test still verifies
runtime behavior while avoiding static-check failures.
| def test_fernet_must_provide_salt_with_source_material(memory_store: MemoryStore): | ||
| """Test that providing source_material without salt raises ValueError.""" | ||
| with pytest.raises(ValueError, match="Must provide a salt"): | ||
| FernetEncryptionWrapper( | ||
| key_value=memory_store, | ||
| source_material="test-source", | ||
| ) |
There was a problem hiding this comment.
Add type: ignore comment for intentional overload violation.
This test intentionally omits the required salt parameter to verify error handling. Add a type: ignore comment to suppress the type checker warning.
🔎 Proposed fix
def test_fernet_must_provide_salt_with_source_material(memory_store: MemoryStore):
"""Test that providing source_material without salt raises ValueError."""
with pytest.raises(ValueError, match="Must provide a salt"):
- FernetEncryptionWrapper(
+ FernetEncryptionWrapper( # type: ignore[call-overload]
key_value=memory_store,
source_material="test-source",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_fernet_must_provide_salt_with_source_material(memory_store: MemoryStore): | |
| """Test that providing source_material without salt raises ValueError.""" | |
| with pytest.raises(ValueError, match="Must provide a salt"): | |
| FernetEncryptionWrapper( | |
| key_value=memory_store, | |
| source_material="test-source", | |
| ) | |
| def test_fernet_must_provide_salt_with_source_material(memory_store: MemoryStore): | |
| """Test that providing source_material without salt raises ValueError.""" | |
| with pytest.raises(ValueError, match="Must provide a salt"): | |
| FernetEncryptionWrapper( # type: ignore[call-overload] | |
| key_value=memory_store, | |
| source_material="test-source", | |
| ) |
🧰 Tools
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
[failure] 221-221:
No overloads for "init" match the provided arguments
Argument types: (MemoryStore, Literal['test-source']) (reportCallIssue)
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_encryption.py around lines
218 to 224, the test intentionally omits the required salt parameter to trigger
a ValueError but triggers a static type-checker warning; add a type ignore
comment to the FernetEncryptionWrapper(...) invocation (e.g. append "# type:
ignore[call-arg]" or "# type: ignore" to that line) so the overload violation is
suppressed while keeping the runtime behavior and assertion unchanged.
| async def test_fallback_ttl_many(self): | ||
| primary_store = FailingStore() | ||
| fallback_store = MemoryStore() | ||
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store) | ||
|
|
||
| # Put data in fallback store | ||
| await fallback_store.put(collection="test", key="k1", value={"v": "1"}, ttl=100) | ||
| await fallback_store.put(collection="test", key="k2", value={"v": "2"}, ttl=200) | ||
|
|
||
| # Should fall back for ttl_many | ||
| results = await wrapper.ttl_many(collection="test", keys=["k1", "k2"]) | ||
| assert results[0][0] == {"v": "1"} | ||
| assert results[1][0] == {"v": "2"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying TTL values.
Test correctly validates ttl_many fallback, but only checks values. Optionally assert that results[0][1] and results[1][1] are not None (or check approximate TTL values) for more thorough validation.
🔎 Optional enhancement
results = await wrapper.ttl_many(collection="test", keys=["k1", "k2"])
assert results[0][0] == {"v": "1"}
assert results[1][0] == {"v": "2"}
+ assert results[0][1] is not None
+ assert results[1][1] is not None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def test_fallback_ttl_many(self): | |
| primary_store = FailingStore() | |
| fallback_store = MemoryStore() | |
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store) | |
| # Put data in fallback store | |
| await fallback_store.put(collection="test", key="k1", value={"v": "1"}, ttl=100) | |
| await fallback_store.put(collection="test", key="k2", value={"v": "2"}, ttl=200) | |
| # Should fall back for ttl_many | |
| results = await wrapper.ttl_many(collection="test", keys=["k1", "k2"]) | |
| assert results[0][0] == {"v": "1"} | |
| assert results[1][0] == {"v": "2"} | |
| async def test_fallback_ttl_many(self): | |
| primary_store = FailingStore() | |
| fallback_store = MemoryStore() | |
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store) | |
| # Put data in fallback store | |
| await fallback_store.put(collection="test", key="k1", value={"v": "1"}, ttl=100) | |
| await fallback_store.put(collection="test", key="k2", value={"v": "2"}, ttl=200) | |
| # Should fall back for ttl_many | |
| results = await wrapper.ttl_many(collection="test", keys=["k1", "k2"]) | |
| assert results[0][0] == {"v": "1"} | |
| assert results[1][0] == {"v": "2"} | |
| assert results[0][1] is not None | |
| assert results[1][1] is not None |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_fallback.py around lines
139 to 151, the test asserts returned values from ttl_many but does not verify
the TTLs; update the test to also assert that results[0][1] and results[1][1]
are not None (or assert they are within expected ranges, e.g. <= 100 and <= 200
and > 0) so the fallback returns both value and a valid TTL; add these
assertions after the existing value checks.
| async def test_fallback_put_many_enabled(self): | ||
| primary_store = FailingStore() | ||
| fallback_store = MemoryStore() | ||
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store, write_to_fallback=True) | ||
|
|
||
| # Should fall back for put_many | ||
| await wrapper.put_many(collection="test", keys=["k1", "k2"], values=[{"v": "1"}, {"v": "2"}]) | ||
|
|
||
| # Verify in fallback | ||
| assert await fallback_store.get(collection="test", key="k1") == {"v": "1"} | ||
| assert await fallback_store.get(collection="test", key="k2") == {"v": "2"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
Test correctly validates put_many falls back to secondary store when write_to_fallback is enabled.
Optionally add a corresponding test for write_to_fallback=False with put_many (should raise ConnectionError, similar to test_write_to_fallback_disabled for put).
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_fallback.py around lines
153-163, add a complementary test to verify behavior when write_to_fallback is
False: create a FailingStore as primary and a MemoryStore as fallback, construct
the FallbackWrapper with write_to_fallback=False, call wrapper.put_many(...) and
assert that it raises a ConnectionError (or the same exception used by
test_write_to_fallback_disabled for put); also assert the fallback store remains
empty/unmodified for the attempted keys. Ensure the test is async and follows
the same naming and assertion style as existing tests.
| async def test_fallback_delete_enabled(self): | ||
| primary_store = FailingStore() | ||
| fallback_store = MemoryStore() | ||
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store, write_to_fallback=True) | ||
|
|
||
| # Put data in fallback | ||
| await fallback_store.put(collection="test", key="test", value={"v": "1"}) | ||
|
|
||
| # Should fall back for delete | ||
| result = await wrapper.delete(collection="test", key="test") | ||
| assert result is True | ||
| assert await fallback_store.get(collection="test", key="test") is None | ||
|
|
||
| async def test_fallback_delete_many_enabled(self): | ||
| primary_store = FailingStore() | ||
| fallback_store = MemoryStore() | ||
| wrapper = FallbackWrapper(primary_key_value=primary_store, fallback_key_value=fallback_store, write_to_fallback=True) | ||
|
|
||
| # Put data in fallback | ||
| await fallback_store.put(collection="test", key="k1", value={"v": "1"}) | ||
| await fallback_store.put(collection="test", key="k2", value={"v": "2"}) | ||
|
|
||
| # Should fall back for delete_many | ||
| result = await wrapper.delete_many(collection="test", keys=["k1", "k2"]) | ||
| assert result == 2 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
Both delete tests correctly validate fallback behavior when write_to_fallback is enabled. The tests verify return values and actual deletion in the fallback store.
Optionally add corresponding tests for write_to_fallback=False with delete/delete_many (should raise ConnectionError).
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_fallback.py around lines
165 to 189, add tests covering the negative case where write_to_fallback=False:
create a FailingStore as primary and a MemoryStore as fallback, construct
FallbackWrapper with write_to_fallback=False, seed the fallback store with
entries, then assert that calling delete(...) raises ConnectionError and that
delete_many(...) raises ConnectionError (and verify that fallback data remains
unchanged). Ensure both tests use async/await and proper pytest.raises for the
ConnectionError.
| async def test_ttl_caches_from_primary(self): | ||
| """Test that ttl retrieves from primary and caches the result.""" | ||
| primary_store = MemoryStore() | ||
| cache_store = MemoryStore() | ||
| wrapper = PassthroughCacheWrapper(primary_key_value=primary_store, cache_key_value=cache_store) | ||
|
|
||
| # Put data in primary with TTL | ||
| await primary_store.put(collection="test", key="test", value={"v": "1"}, ttl=100) | ||
|
|
||
| # Call ttl - should get from primary and cache it | ||
| value, ttl = await wrapper.ttl(collection="test", key="test") | ||
| assert value == {"v": "1"} | ||
| assert ttl is not None | ||
|
|
||
| # Verify it's now in cache | ||
| cached_value = await cache_store.get(collection="test", key="test") | ||
| assert cached_value == {"v": "1"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Validate TTL values more precisely.
The test checks that ttl is not None but doesn't verify the value is close to the expected 100 seconds. Consider asserting ttl > 0 or ttl <= 100 to strengthen validation.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_passthrough_cache.py
around lines 32 to 48, the test currently only asserts `ttl is not None`;
tighten this by asserting the TTL is within expected bounds to catch regressions
— replace or add assertions such as `assert ttl > 0` and `assert ttl <= 100` (or
`assert 0 < ttl <= 100`) so the returned TTL is positive and does not exceed the
original 100-second value, allowing for small elapsed time.
| async def test_ttl_many_caches_from_primary(self): | ||
| """Test that ttl_many retrieves from primary and caches results.""" | ||
| primary_store = MemoryStore() | ||
| cache_store = MemoryStore() | ||
| wrapper = PassthroughCacheWrapper(primary_key_value=primary_store, cache_key_value=cache_store) | ||
|
|
||
| # Put data in primary with TTL | ||
| await primary_store.put(collection="test", key="k1", value={"v": "1"}, ttl=100) | ||
| await primary_store.put(collection="test", key="k2", value={"v": "2"}, ttl=200) | ||
|
|
||
| # Call ttl_many - should get from primary and cache | ||
| results = await wrapper.ttl_many(collection="test", keys=["k1", "k2"]) | ||
| assert results[0][0] == {"v": "1"} | ||
| assert results[1][0] == {"v": "2"} | ||
|
|
||
| # Verify in cache | ||
| assert await cache_store.get(collection="test", key="k1") == {"v": "1"} | ||
| assert await cache_store.get(collection="test", key="k2") == {"v": "2"} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider testing partial cache hits.
Current tests cover full primary fetch and full cache hit scenarios. A test where some keys are cached and others require primary fetch would strengthen coverage of ttl_many behavior.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/wrappers/test_passthrough_cache.py
around lines 75 to 92, add a test for partial cache hits: seed the cache_store
with one key (e.g., "k1") and the primary_store with both keys ("k1" and "k2")
with TTLs, call wrapper.ttl_many(collection="test", keys=["k1","k2"]), assert
returned values for both keys are correct, assert the cached key remains
unchanged and the missing key ("k2") is fetched from primary and written into
cache, and verify cache_store.get for "k2" now returns the expected value; keep
assertions for TTL behavior consistent with existing tests.
| class TestPrepareEntryTimestamps: | ||
| def test_with_ttl(self): | ||
| """Test prepare_entry_timestamps with a TTL.""" | ||
| created_at, ttl, expires_at = prepare_entry_timestamps(ttl=100) | ||
| assert ttl == 100.0 | ||
| assert expires_at is not None | ||
| assert expires_at > created_at | ||
|
|
||
| def test_without_ttl(self): | ||
| """Test prepare_entry_timestamps without a TTL.""" | ||
| created_at, ttl, expires_at = prepare_entry_timestamps(ttl=None) | ||
| assert ttl is None | ||
| assert expires_at is None | ||
| assert created_at is not None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more precise assertion in test_with_ttl.
Tests cover both branches correctly. However, test_with_ttl could verify the exact relationship between expires_at and created_at.
🔎 Proposed refinement
+from datetime import timedelta
+
class TestPrepareEntryTimestamps:
def test_with_ttl(self):
"""Test prepare_entry_timestamps with a TTL."""
created_at, ttl, expires_at = prepare_entry_timestamps(ttl=100)
assert ttl == 100.0
assert expires_at is not None
- assert expires_at > created_at
+ assert expires_at == created_at + timedelta(seconds=100)🤖 Prompt for AI Agents
In key-value/key-value-shared/tests/utils/test_time_to_live.py around lines 93
to 106, the test_with_ttl only asserts that expires_at > created_at; change it
to verify the exact relationship by asserting expires_at equals created_at plus
ttl. If floating-point timing is used, use a tolerant comparison (e.g.,
math.isclose or pytest.approx with a small tolerance) to compare expires_at and
created_at + ttl, and keep the existing ttl assertion.
Test Failure AnalysisSummary: The CI workflow failed due to markdown linting errors in documentation files that are not part of this PR's changes. Root Cause: The Important: This PR only modifies test files:
The linting failures are in documentation files ( Suggested Solution: Fix the markdown table formatting in the affected documentation files by adding spaces around the delimiter content in table separator rows. Specifically, table delimiter rows need to change from: |---------|-------------|To: | --------- | ----------- |Detailed AnalysisThe workflow failure occurs in the Affected Files and Lines:
Error Pattern:
Example from docs/wrappers.md:10: | Wrapper | Description |
|---------|-------------| ← Missing spaces around dashes
| [CompressionWrapper](#compressionwrapper) | Compress values before storing |Should be: | Wrapper | Description |
| --------- | ----------- | ← Spaces added
| [CompressionWrapper](#compressionwrapper) | Compress values before storing |Example from docs/stores.md:398: | Store | Stability | Async | Sync | Description |
|-------|:---------:|:-----:|:----:|:------------| ← Missing spaces
| DynamoDB | Unstable | ✅ | ✖️ | AWS DynamoDB key-value storage |Should be: | Store | Stability | Async | Sync | Description |
| ------- | :-------: | :---: | :--: | :---------- | ← Spaces added
| DynamoDB | Unstable | ✅ | ✖️ | AWS DynamoDB key-value storage |Related FilesFiles requiring fixes (not modified by this PR):
Files modified by this PR (no issues):
Recommendation: These documentation fixes should be applied in a separate PR or commit to unblock this coverage improvement PR. The test changes in this PR are valid and do not cause any failures. |
Add comprehensive tests for previously uncovered code paths:
put_many, delete, delete_many with write_to_fallback)
arguments (fernet/source_material/salt validation)
handling, and error cases
and TTL validation edge cases
Coverage improvements:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.