Expands test coverage to include public and internal initialization #4
Conversation
Expands test coverage to include public and internal initialization logic, Redis connection handling with different config scenarios, queue length and dequeue edge cases, Lua script reloading, and utility functions for payload deserialization and type conversion. Enhances reliability by verifying error handling and compatibility with legacy payload formats, as well as safe resource cleanup.
Introduces comprehensive async unit tests to exercise rare and error-prone execution paths, improving code robustness and coverage for cluster initialization, queue clearing, deep status checks, and input validation. Enables detection of regressions in edge scenarios and aids future refactoring.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds extensive async unit and functional tests for Redis/Lua edge cases, tightens config loading, and introduces an early Redis connectivity validation during queue initialization; also removes the top-level docker-compose version field. Changes
Sequence Diagram(s)sequenceDiagram
participant Init as Initializer
participant Conf as Config Reader
participant Redis as Redis Server
participant Lua as Lua Script Loader
Init->>Conf: load config file
Conf-->>Init: config (redis, fq) or error
Init->>Redis: open connection
Redis-->>Init: connection object
Init->>Redis: PING (via _validate_redis_connection)
alt ping OK
Redis-->>Init: PONG
Init->>Lua: load/refresh scripts
Lua-->>Init: scripts loaded
else ping FAIL
Redis-->>Init: error/false
Init-->>Init: raise FQException (abort initialization)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c466fee55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tests/test_func.py
Outdated
| async def test_asyncTearDown(self): | ||
| await self.queue._r.flushdb() | ||
| await self.queue.close() |
There was a problem hiding this comment.
Renaming the asyncTearDown hook to test_asyncTearDown means unittest will treat it as a normal test and never run teardown after each test case. That leaves Redis state and connections open between tests, which can cause order-dependent failures and resource leaks when the suite grows or runs in parallel. This regression is visible in the new method name and breaks the expected unittest lifecycle.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_func.py`:
- Around line 1817-1841: The test test_initialize_unix_socket_connection is
asserting success against a non-existent Unix socket; update the test to either
mock the Redis client or skip when the socket is absent: replace the real
connect attempt in FQ._initialize by patching the Redis constructor (or the
method used inside FQ._initialize) to return a fake/non-blocking client so fq._r
is set, or add a pre-check for the unix_socket_path and call self.skipTest(...)
when the socket file does not exist; reference
test_initialize_unix_socket_connection, class FQ, and method _initialize to
locate and apply the change.
🧹 Nitpick comments (3)
tests/test_func.py (2)
1886-1929: Tests defined as async but don't use await.The following tests are defined as
asyncbut don't perform any async operations:
test_deserialize_payload_old_formattest_deserialize_payload_new_formattest_convert_to_str_with_mixed_typesWhile this works fine with
IsolatedAsyncioTestCase, regulardefwould be more appropriate for synchronous tests.♻️ Suggested change
- async def test_deserialize_payload_old_format(self): + def test_deserialize_payload_old_format(self): """Test deserialize_payload with old quote-wrapped format - tests utils.py line 63.""" ... - async def test_deserialize_payload_new_format(self): + def test_deserialize_payload_new_format(self): """Test deserialize_payload handles new unwrapped format - tests utils.py line 63.""" ... - async def test_convert_to_str_with_mixed_types(self): + def test_convert_to_str_with_mixed_types(self): """Test convert_to_str handles both bytes and strings.""" ...
1904-1915: Test duplication detected.
test_deep_status_redis_availability(lines 1911-1915) is essentially a duplicate oftest_deep_status(lines 1729-1732). Both tests verify the same behavior - thatdeep_status()returns a non-None result. Consider removing one of them.tests/test_edge_cases.py (1)
149-155: Redundant assertion on line 155.Line 154 already asserts
fq._ris None. Line 155 (self.assertTrue(fq._r is None)) is redundant and can be removed.♻️ Suggested fix
async def test_close_fallback_paths(self): """Covers close() fallback paths (queue.py lines 528-549).""" fq = FQ(self.config_path) fq._r = FakeRedisForClose() await fq.close() self.assertIsNone(fq._r) - self.assertTrue(fq._r is None)
There was a problem hiding this comment.
Pull request overview
Expands the test suite for the FQ Redis-backed queue to cover additional initialization paths, edge cases, and utility behavior.
Changes:
- Added a new edge-case-focused test module with mock Redis/cluster implementations.
- Added multiple new functional tests covering initialization, client lifecycle, queue operations, and payload/utility handling.
- Updated
uv.lockmetadata for the editable local package entry.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| uv.lock | Adjusts the lock entry for the local editable flowdacity-queue package. |
| tests/test_func.py | Adds functional tests for initialization, client management, queue length, dequeue empty behavior, payload deserialization, and utility conversions. |
| tests/test_edge_cases.py | Introduces a new edge-case test module using fakes/mocks to exercise failure/branch paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_func.py
Outdated
| self.assertIn("key2", result) | ||
| self.assertIn("key3", result) | ||
|
|
||
| async def test_asyncTearDown(self): |
There was a problem hiding this comment.
unittest.IsolatedAsyncioTestCase uses asyncTearDown() for per-test cleanup, but this file defines test_asyncTearDown() instead. That means the Redis connection is no longer closed after each test (and this method will run as a normal test instead), which can lead to unclosed-connection ResourceWarnings and test interference over time. Rename this method back to asyncTearDown (and keep the flush/close logic there).
| async def test_asyncTearDown(self): | |
| async def asyncTearDown(self): |
tests/test_edge_cases.py
Outdated
| fq._r = FakeRedisForClose() | ||
| await fq.close() | ||
| self.assertIsNone(fq._r) | ||
| self.assertTrue(fq._r is None) |
There was a problem hiding this comment.
assertTrue(a is b) cannot provide an informative message. Using assertIs(a, b) instead will give more informative messages.
| self.assertTrue(fq._r is None) | |
| self.assertIsNone(fq._r) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: ochui <[email protected]>
Fix test teardown method and remove redundant assertion
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_edge_cases.py`:
- Around line 18-21: The unused parameters in register_script's inner async
function _runner and in the class/method __call__ should be explicitly prefixed
with underscores to satisfy Ruff ARG001/ARG002; update _runner's signature to
use _args and _kwargs (or prefix each unused param with an underscore) and
likewise rename any unused parameters in __call__ to start with an underscore
(e.g., _args, _kwargs or _param -> _param) so the linter recognizes they are
intentionally unused.
♻️ Duplicate comments (1)
tests/test_func.py (1)
1817-1841: Mock or skip unix-socket init to avoid a hard failure.This test points to
/tmp/redis_nonexistent.sockand expects_initialize()to succeed. On most systems the socket won’t exist, so the test will fail or be flaky. Consider mocking the Redis client (or skipping when the socket is absent).✅ Safer approach: mock Redis for the unix socket path
async def test_initialize_unix_socket_connection(self): """Test initialization with unix socket connection - tests line 59.""" # Create a temporary config with unix_sock with tempfile.NamedTemporaryFile(mode='w', suffix='.conf', delete=False) as f: f.write("""[fq] @@ """) config_path = f.name try: + from unittest.mock import AsyncMock, patch fq = FQ(config_path) - # This tests the unix_sock path (line 59) - await fq._initialize() - self.assertIsNotNone(fq._r) - await fq.close() + # Mock Redis to avoid requiring a real unix socket + with patch("fq.queue.Redis") as mock_redis: + mock_client = AsyncMock() + mock_redis.return_value = mock_client + await fq._initialize() + call_kwargs = mock_redis.call_args[1] + self.assertEqual( + call_kwargs.get("unix_socket_path"), + "/tmp/redis_nonexistent.sock", + ) + await fq.close() finally: os.unlink(config_path)
| def register_script(self, _): | ||
| async def _runner(*args, **kwargs): | ||
| return [] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Ruff configuration files
find . -type f \( -name "pyproject.toml" -o -name "setup.cfg" -o -name "ruff.toml" -o -name ".ruff.toml" \) | head -20Repository: flowdacity/flowdacity-queue
Length of output: 99
🏁 Script executed:
# Check the test file structure and content at specified lines
wc -l tests/test_edge_cases.pyRepository: flowdacity/flowdacity-queue
Length of output: 99
🏁 Script executed:
# View the code at lines 18-21
sed -n '15,25p' tests/test_edge_cases.pyRepository: flowdacity/flowdacity-queue
Length of output: 316
🏁 Script executed:
# View the code at lines 55-56 (and context)
sed -n '50,60p' tests/test_edge_cases.pyRepository: flowdacity/flowdacity-queue
Length of output: 285
🏁 Script executed:
# Check Ruff configuration for ARG rules
cat pyproject.tomlRepository: flowdacity/flowdacity-queue
Length of output: 788
🏁 Script executed:
# Also check the dedicated ruff.toml
cat ruff.tomlRepository: flowdacity/flowdacity-queue
Length of output: 977
🏁 Script executed:
# Run Ruff linting on the test file to confirm the issues
ruff check tests/test_edge_cases.py --select ARGRepository: flowdacity/flowdacity-queue
Length of output: 1739
Prefix unused parameters with underscores to silence Ruff ARG001/ARG002.
The project enforces Ruff linting with ARG rules enabled. The parameters in _runner and __call__ are intentionally unused, so prefix them with underscores to make this explicit and pass linting.
🔧 Proposed fix
class FakeCluster:
def register_script(self, _):
- async def _runner(*args, **kwargs):
+ async def _runner(*_args, **_kwargs):
return []
class FakeLuaDequeue:
- async def __call__(self, keys=None, args=None):
+ async def __call__(self, _keys=None, _args=None):
self.called = True
return [b"q1", b"j1", None, b"0"]🧰 Tools
🪛 Ruff (0.14.13)
19-19: Unused function argument: args
(ARG001)
19-19: Unused function argument: kwargs
(ARG001)
🤖 Prompt for AI Agents
In `@tests/test_edge_cases.py` around lines 18 - 21, The unused parameters in
register_script's inner async function _runner and in the class/method __call__
should be explicitly prefixed with underscores to satisfy Ruff ARG001/ARG002;
update _runner's signature to use _args and _kwargs (or prefix each unused param
with an underscore) and likewise rename any unused parameters in __call__ to
start with an underscore (e.g., _args, _kwargs or _param -> _param) so the
linter recognizes they are intentionally unused.
Adds early validation for missing or unreadable config files and ensures Redis connection errors are surfaced during initialization. Updates tests to cover these failure cases, improving robustness and error transparency.
Prevents actual Redis connections during testing by using AsyncMock for the Redis client and its ping method. Increases test isolation and reliability by removing dependency on an external Redis server.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_func.py
Outdated
| import unittest | ||
| import msgpack | ||
| import tempfile | ||
| from unittest.mock import patch, AsyncMock |
There was a problem hiding this comment.
patch is imported but never used in this test module. If you’re running linting (or plan to), this will be flagged; please remove the unused import or use it in the new tests.
| from unittest.mock import patch, AsyncMock | |
| from unittest.mock import AsyncMock |
tests/test_func.py
Outdated
| fq = FQ(config_path) | ||
| # Mock the Redis ping to avoid actual connection attempt | ||
| mock_redis = AsyncMock() | ||
| mock_redis.ping = AsyncMock(return_value=True) | ||
| fq._r = mock_redis | ||
| # This tests the unix_sock path was configured (line 59) | ||
| self.assertIsNotNone(fq._r) |
There was a problem hiding this comment.
test_initialize_unix_socket_connection doesn’t actually exercise the unix-socket initialization path: it never calls fq._initialize() / fq.initialize(), and it manually assigns fq._r, so it can pass even if the conn_type == "unix_sock" branch is broken. To make this test meaningful, patch fq.queue.Redis to a fake that records init kwargs and provides an async ping(), then call await fq._initialize() and assert unix_socket_path was used.
| class TestEdgeCases(unittest.IsolatedAsyncioTestCase): | ||
| async def asyncSetUp(self): | ||
| cwd = os.path.dirname(os.path.realpath(__file__)) | ||
| self.config_path = os.path.join(cwd, "test.conf") | ||
|
|
There was a problem hiding this comment.
This new test module runs against the shared Redis service (several tests call await fq._initialize() and execute commands) but doesn’t consistently clean up Redis state between tests. Other suites flush Redis in setup/teardown (e.g., tests/test_func.py:29-32, tests/test_queue.py:60-66). Consider adding an asyncTearDown (or per-test helper) that flushes the configured DB and closes the client when initialized, to avoid order-dependent failures when running subsets or adding new tests later.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: ochui <[email protected]>
Co-authored-by: ochui <[email protected]>
Co-authored-by: ochui <[email protected]>
Fix test coverage gaps: unused imports, mock unix socket init, and cleanup
This pull request significantly expands and strengthens the test suite for edge cases and functional coverage of the
FQqueueing system. It introduces a new dedicated edge case test module with extensive mocking and adds numerous functional tests to ensure correct behavior across a wide range of scenarios, including configuration handling, connection types, queue operations, payload deserialization, and Redis client management.The most important changes are:
New edge case test coverage:
tests/test_edge_cases.pythat uses mock classes to simulate various Redis behaviors and covers many edge conditions and fallback paths inqueue.pyandutils.py, such as cluster initialization, dequeue withNonepayload, queue clearing, client connection handling, and configuration reloads.Expanded functional tests for robustness:
tests/test_func.pyto cover:initialize()method, Redis client getter, and connection close logic.deep_status()method for Redis availability.reload_lua_scripts()method and its effect on subsequent operations.get_queue_length(), including invalid parameters and empty queues.dequeue()method on empty queues.deserialize_payload()utility with both old (quote-wrapped) and new formats.convert_to_str()utility with mixed bytes/string sets.Test infrastructure and imports:
tests/test_func.pyto support new tests and error checks.These changes greatly improve the reliability and maintainability of the codebase by ensuring edge cases and error conditions are thoroughly tested.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.