Add integration tests for all CommandFlags overloads - #393
Conversation
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
There was a problem hiding this comment.
Pull request overview
Adds missing integration coverage for StackExchange.Redis-compatible CommandFlags overloads to ensure unsupported flags consistently throw NotImplementedException, and aligns a couple of server scripting methods with that behavior. Also extends the Taskfile to support a simple test filter variable.
Changes:
- Added many new
CommandFlagsintegration tests across DB/server/subscriber surfaces. - Updated
ValkeyServer.ScriptExistsAsync/ScriptLoadAsyncto checkCommandFlagsbefore other argument validation. - Added
filtersupport totask test:unit/task test:integration(and updated docs).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs | Adds extensive integration tests asserting unsupported CommandFlags throw NotImplementedException. |
| sources/Valkey.Glide/Abstract/ValkeyServer.cs | Reorders validation so unsupported flags fail fast for script APIs. |
| Taskfile.yml | Adds filter variable support for dotnet test filtering; refactors build target variable. |
| DEVELOPER.md | Documents how to use the new Taskfile filter variable for tests. |
| AGENTS.md | Updates agent-facing test filtering instructions (currently inconsistent with Taskfile). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds broad CommandFlags integration tests, reorders flag validation in ValkeyServer script methods to run before argument checks, and updates Taskfile and docs to enable running filtered tests via ChangesTest Infrastructure & Documentation Updates
CommandFlags Validation & Integration Test Coverage
Sequence DiagramsequenceDiagram
participant CommandFlagsTests
participant API_Method
participant GuardClauses
CommandFlagsTests->>API_Method: invoke with UnsupportedFlag
API_Method->>GuardClauses: ThrowIfCommandFlags(flags)
GuardClauses-->>API_Method: throw NotImplementedException
API_Method-->>CommandFlagsTests: NotImplementedException raised
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs (2)
441-447:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate test assertions - likely missing DateTime overload coverage.
Lines 444 and 446 call the same method with identical parameters. Based on the pattern in the
SingleFieldtests (lines 433-437), this should test both theTimeSpan-based andDateTime-based overloads ofHashFieldSetAndSetExpiryAsyncfor multi-field operations.🔄 Suggested fix to test both overloads
[Theory(DisableDiscoveryEnumeration = true)] [MemberData(nameof(TestConfiguration.TestDatabases), MemberType = typeof(TestConfiguration))] public async Task HashFieldSetAndSetExpiryAsync_MultiField_ThrowsOnCommandFlags(IDatabaseAsync db) { _ = await Assert.ThrowsAsync<NotImplementedException>( - () => db.HashFieldSetAndSetExpiryAsync("key", [], flags: UnsupportedFlag)); + () => db.HashFieldSetAndSetExpiryAsync("key", new[] { new HashFieldValue("field", "value", TimeSpan.Zero) }, flags: UnsupportedFlag)); _ = await Assert.ThrowsAsync<NotImplementedException>( - () => db.HashFieldSetAndSetExpiryAsync("key", [], flags: UnsupportedFlag)); + () => db.HashFieldSetAndSetExpiryAsync("key", new[] { new HashFieldValue("field", "value", DateTime.MinValue) }, flags: UnsupportedFlag)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs` around lines 441 - 447, The two assertions in HashFieldSetAndSetExpiryAsync_MultiField_ThrowsOnCommandFlags currently call the same TimeSpan overload twice; update the second Assert.ThrowsAsync to call the DateTime overload of HashFieldSetAndSetExpiryAsync so the test covers both overloads (keep using the same key, empty field list and UnsupportedFlag), referencing the IDatabaseAsync.HashFieldSetAndSetExpiryAsync(DateTime) signature to ensure the DateTime-based overload is invoked.
421-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate test assertions - likely missing DateTime overload coverage.
Lines 424 and 426 call the same method with identical parameters. Based on the pattern in the
SingleFieldtests (lines 411-417), this should test both theTimeSpan-based andDateTime-based overloads ofHashFieldGetAndSetExpiryAsyncfor multi-field operations.🔄 Suggested fix to test both overloads
[Theory(DisableDiscoveryEnumeration = true)] [MemberData(nameof(TestConfiguration.TestDatabases), MemberType = typeof(TestConfiguration))] public async Task HashFieldGetAndSetExpiryAsync_MultiField_ThrowsOnCommandFlags(IDatabaseAsync db) { _ = await Assert.ThrowsAsync<NotImplementedException>( - () => db.HashFieldGetAndSetExpiryAsync("key", [], flags: UnsupportedFlag)); + () => db.HashFieldGetAndSetExpiryAsync("key", new[] { new HashFieldExpiry("field", TimeSpan.Zero) }, flags: UnsupportedFlag)); _ = await Assert.ThrowsAsync<NotImplementedException>( - () => db.HashFieldGetAndSetExpiryAsync("key", [], flags: UnsupportedFlag)); + () => db.HashFieldGetAndSetExpiryAsync("key", new[] { new HashFieldExpiry("field", DateTime.MinValue) }, flags: UnsupportedFlag)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs` around lines 421 - 427, The two assertions call the same overload of HashFieldGetAndSetExpiryAsync; change the second assertion to exercise the DateTime-based overload for multi-field operations so both overloads are covered: keep the first assertion testing the TimeSpan-based overload (existing call with UnsupportedFlag) and modify the second call to pass the DateTime expiry overload variant for HashFieldGetAndSetExpiryAsync while still asserting NotImplementedException and using the same test key, multi-field input, and UnsupportedFlag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs`:
- Around line 441-447: The two assertions in
HashFieldSetAndSetExpiryAsync_MultiField_ThrowsOnCommandFlags currently call the
same TimeSpan overload twice; update the second Assert.ThrowsAsync to call the
DateTime overload of HashFieldSetAndSetExpiryAsync so the test covers both
overloads (keep using the same key, empty field list and UnsupportedFlag),
referencing the IDatabaseAsync.HashFieldSetAndSetExpiryAsync(DateTime) signature
to ensure the DateTime-based overload is invoked.
- Around line 421-427: The two assertions call the same overload of
HashFieldGetAndSetExpiryAsync; change the second assertion to exercise the
DateTime-based overload for multi-field operations so both overloads are
covered: keep the first assertion testing the TimeSpan-based overload (existing
call with UnsupportedFlag) and modify the second call to pass the DateTime
expiry overload variant for HashFieldGetAndSetExpiryAsync while still asserting
NotImplementedException and using the same test key, multi-field input, and
UnsupportedFlag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9237eb40-f7fc-42cd-a09c-f458d0ccfdaa
📒 Files selected for processing (5)
AGENTS.mdDEVELOPER.mdTaskfile.ymlsources/Valkey.Glide/Abstract/ValkeyServer.cstests/Valkey.Glide.IntegrationTests/StackExchange/CommandFlagsTests.cs
…ssertions Without a prefix, FT.CREATE indexes scan all keys in the database. When tests run in parallel, stray keys from other tests can be indexed, causing attr.Size to be non-zero unexpectedly. Adding a unique prefix (based on the index GUID) isolates each test's index from other data in the database. Signed-off-by: currantw <taylor.curran@improving.com>
…hash expiry HashFieldGetAndSetExpiryAsync_MultiField and HashFieldSetAndSetExpiryAsync_MultiField were calling the same TimeSpan-based overload twice. The second assertion now calls the DateTime-based overload to cover both signatures. Signed-off-by: currantw <taylor.curran@improving.com>
… into currantw/254_add_missing_command_flags_tests Signed-off-by: currantw <taylor.curran@improving.com>
|
Cross referencing the list inf Issue #254, it looks like you may be missing some overloads |
Signed-off-by: currantw <taylor.curran@improving.com>
Added test for |
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Signed-off-by: currantw <taylor.curran@improving.com>
Summary
Add integration tests for all missing
CommandFlagsoverloads across the StackExchange.Redis compatibility surface (previously 194/253 were covered).Issue Link
Closes #254
Features, Behaviour Changes, and Implementation
CommandFlagsoverloads now have integration tests verifying they throwNotImplementedExceptionwhen a flag other thanCommandFlags.Noneis passed.ValkeyServer.ScriptExistsAsyncandScriptLoadAsyncnow checkCommandFlagsbefore validating other arguments, consistent with the rest of the codebase.filtervariable for filtering tests by class or method name (e.g.task test:integration filter=CommandFlagsTests).Limitations
⚪ None
Testing
task test:integration filter=CommandFlagsTests.Related Issues
⚪ None
Checklist
CHANGELOG.md,README.md,DEVELOPER.md, and other documentation files are updated.mainor releasemain, squash otherwise.Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores