Conversation
|
Coverage report: |
# Conflicts: # test/test_mixins/test_acl_commands.py
# Conflicts: # test/test_mixins/test_bitmap_commands.py
# Conflicts: # fakeredis/stack/_json_mixin.py # uv.lock
# Conflicts: # test/test_mixins/test_scripting.py
# Conflicts: # .github/workflows/test-dragonfly.yml
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Dragonfly server support to the test suite, including test exclusions, Dragonfly-specific test files, and implementation fixes for Dragonfly-specific behavior differences.
- Adds Dragonfly-specific bitmap command tests in a new dedicated test file
- Updates test markers to exclude Dragonfly from tests that aren't compatible and enables previously skipped tests now passing on Dragonfly
- Implements Dragonfly-specific error handling and behavior differences in the fakeredis implementation
- Modifies CI workflow to run Dragonfly tests on PRs while temporarily disabling standard tests on PRs
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/test.yml |
Disables pull_request trigger for main test workflow |
.github/workflows/test-dragonfly.yml |
Enables pull_request trigger and adds hypothesis tests to Dragonfly CI |
test/test_mixins/test_string_commands.py |
Reduces setrange offset test value from 501970112 to 50197112 |
test/test_mixins/test_sortedset_commands.py |
Removes unsupported_server_types markers for tests now passing on Dragonfly, updates TODO comments |
test/test_mixins/test_scripting.py |
Consolidates Redis/Valkey Lua bool argument tests into single test with multi-server error message handling |
test/test_mixins/test_geo_commands.py |
Reformats multi-line tuples to single-line for consistency |
test/test_mixins/test_bitmap_commands_dragonfly.py |
Adds comprehensive Dragonfly-specific bitmap command tests (new file) |
test/test_mixins/test_bitmap_commands.py |
Adds dragonfly exclusion marker to existing bitmap tests |
test/test_mixins/test_acl_commands.py |
Adds dragonfly exclusion marker to ACL tests |
test/test_json/test_json_arr_commands.py |
Reformats multi-line dictionary literals to single-line |
test/test_json/test_json.py |
Reformats multi-line dictionary literals to single-line |
test/test_hypothesis/test_server.py |
Adds dragonfly command strategy, renames redis-only strategy |
test/test_hypothesis/test_hash.py |
Adds dragonfly-specific command strategy for hash commands |
test/test_hypothesis/base.py |
Adds dragonfly command strategy integration to base test class |
fakeredis/stack/_json_mixin.py |
Adds Dragonfly-specific error message for missing keys in JSON.STRLEN |
fakeredis/commands_mixins/bitmap_mixin.py |
Adds Dragonfly-specific behavior for empty bitfield commands |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # pull_request: | ||
| # branches: | ||
| # - master |
There was a problem hiding this comment.
Commenting out the pull_request trigger for the main test workflow effectively disables it for pull requests. This means that the main unit tests (which cover redis and valkey) will no longer run on pull requests, only on pushes to master. This is likely unintentional and will reduce test coverage for PRs. If this was intentional to focus on dragonfly testing during this development phase, it should be temporary and clearly documented.
| for m in (msgs.LUA_COMMAND_ARG_MSG6, msgs.LUA_COMMAND_ARG_MSG, msgs.VALKEY_LUA_COMMAND_ARG_MSG): | ||
| count += m[4:] in str(exc_info.value) |
There was a problem hiding this comment.
The logic here uses string slicing m[4:] to skip the "ERR " prefix and then checks if that substring appears in the error message. However, this approach has issues:
- The VALKEY_LUA_COMMAND_ARG_MSG does not start with "ERR ", so slicing
[4:]removes the first 4 characters ("Comm") instead of the expected prefix, making the match check incorrect. - Using substring matching with
incan lead to false positives if part of the message appears elsewhere in the error.
A more robust approach would be to check if the full error message matches any of the expected messages, possibly after normalizing both by removing common prefixes.
| | commands(st.just("save")) | ||
| ) | ||
| command_strategy_redis_only = commands(st.sampled_from(["flushdb", "flushall"]), st.sampled_from([[], "async"])) | ||
| command_strategy_dragonfly = commands(st.sampled_from(["flushdb", "flushall"])) |
There was a problem hiding this comment.
The variable is named command_strategy_dragonfly but it's actually used for commands that work on dragonfly. For consistency with the naming pattern used for command_strategy_redis7 and command_strategy_redis_only, consider renaming to command_strategy_dragonfly_only if these commands are dragonfly-exclusive, or providing a clearer name that indicates what distinguishes these commands.
| ) | ||
|
|
||
|
|
||
| # TODO fix |
There was a problem hiding this comment.
The TODO comment "fix" is too vague. It should explain what needs to be fixed about this test. Based on the context, this appears to be a working test for Dragonfly, so it's unclear what the TODO refers to. Either clarify the TODO or remove it if the test is working as expected.
| if len(args) == 0 and self._server.server_type == "dragonfly": | ||
| raise SimpleError(msgs.WRONG_ARGS_MSG6.format("bitfield")) |
There was a problem hiding this comment.
This check for empty args only applies to Dragonfly and raises an error before processing begins. However, line 274-275 handles the case where results is empty at the end, also only for Dragonfly. This creates inconsistent behavior: if args is empty, we raise an error (line 238-239), but if args contains only "OVERFLOW" commands with no actual operations (which would also result in empty results), we return None (line 274-275). Consider consolidating this logic or clarifying when an error should be raised vs when None should be returned.
No description provided.