test: fix async test failures in cache embeddings and buffer strategy tests #1237
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1237 +/- ##
========================================
Coverage 68.65% 68.65%
========================================
Files 161 161
Lines 15978 15978
========================================
Hits 10969 10969
Misses 5009 5009
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes async test failures in cache embeddings and buffer strategy tests by updating method signatures, patching strategies, and call count assertions.
- Converted get_embeddings to an async function and updated corresponding tests.
- Updated patching to use mocks for proper async behavior and adjusted assertions on call counts.
- Changed an async test to a synchronous one for the buffer chunk strategy.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_cache_embeddings.py | Converted get_embeddings to async, added pytest.mark.asyncio decorator, and updated caching-related patching and assertions. |
| tests/test_buffer_strategy.py | Changed the test_generate_chunk_str function from async to sync and set up the buffer_strategy.last_index appropriately. |
| ] | ||
| mock_cache.get.assert_not_called() | ||
| mock_cache.set.assert_not_called() | ||
| with patch.object(MyClass, "cache_config", new_callable=lambda: mock_config): |
There was a problem hiding this comment.
[nitpick] Since cache_config is a property, consider using PropertyMock (e.g., new_callable=PropertyMock) for clarity and to ensure consistent behavior when patching properties.
Copilot uses AI. Check for mistakes.
| @@ -68,10 +68,13 @@ async def async_enumerate(aiterable, start=0): | |||
| idx += 1 | |||
|
|
|||
|
|
|||
| async def test_generate_chunk_str(): | |||
| def test_generate_chunk_str(): | |||
There was a problem hiding this comment.
[nitpick] Ensure that changing the test from async to synchronous is intentional, and if so, consider updating the test name or documentation to reflect the synchronous behavior.
| def test_generate_chunk_str(): | |
| def test_generate_chunk_str_synchronous(): |
Copilot uses AI. Check for mistakes.
fixes failure in latest dependencies test in CI:
✅ https://github.com/NVIDIA/NeMo-Guardrails/actions/runs/15851925033
example of failure:
❌ https://github.com/NVIDIA/NeMo-Guardrails/actions/runs/15836110756