Leverate NUnit Assert.Throws #1416
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors test code to use NUnit's Assert.Throws pattern instead of try-catch blocks with Assert.Fail(). This change improves test reliability by ensuring that tests will fail if an expected exception is not thrown, eliminating the risk of silent passing tests when Assert.Fail() is accidentally omitted.
- Replaces try-catch-Assert.Fail patterns with
Assert.Throws<T>()calls - Applies changes to synchronous test flows only
- Removes redundant closing braces introduced by the refactoring
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/Garnet.test/RespTransactionProcTests.cs | Refactored exception testing for RUNTXP command validation |
| test/Garnet.test/RespTests.cs | Updated SETRANGE command error handling tests |
| test/Garnet.test/RespSortedSetTests.cs | Simplified ZSCAN parameter validation test |
| test/Garnet.test/RespSetTest.cs | Refactored SSCAN and set operation error tests |
| test/Garnet.test/RespModuleTests.cs | Updated module loading error validation tests |
| test/Garnet.test/RespHashTests.cs | Simplified HSCAN parameter validation test |
| test/Garnet.test/RespEtagTests.cs | Updated SETRANGE error handling for etag tests |
| test/Garnet.test/RespCustomCommandTests.cs | Refactored registration limit validation tests |
| test/Garnet.test/RespCommandTests.cs | Updated COMMAND subcommand error test |
| test/Garnet.test/RespAdminCommandsTests.cs | Simplified CONFIG command validation tests |
| test/Garnet.test/GarnetBitmapTests.cs | Updated bitmap operation error validation tests |
| test/Garnet.test.cluster/RedirectTests/ClusterSlotVerificationTests.cs | Refactored cluster redirect and error scenario tests |
| test/Garnet.test.cluster/ClusterConfigTests.cs | Updated cluster forget command error test and removed unused using directive |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
TalZaccai
left a comment
TalZaccai
left a comment
There was a problem hiding this comment.
This is great, thanks for your contribution!
Couple of small comments...
| var ex = Assert.Throws<RedisServerException>(() => server.Execute("cluster", args), | ||
| "Cluster forget call shouldn't have succeeded for an invalid node id."); | ||
|
|
||
| Assert.That(ex.Message == "ERR I don't know about node 1ip23j89123no."); |
There was a problem hiding this comment.
For a better debugging experience, please use Assert.That(ex.Message, Is.EqualTo(<message>));. Otherwise it'll give a generic error, something like "Expected True but was False" rather than "Expected message was "abcd" but got "efgh"".
| } | ||
| catch (Exception ex) | ||
| { | ||
| var ex = Assert.Throws<RedisServerException>(() => context.clusterTestUtils.GetServer(requestNodeIndex).Execute(command.Command, command.GetSingleSlotRequest(), CommandFlags.NoRedirect), |
There was a problem hiding this comment.
Looks like there are still a couple of try/catch blocks in this method?
2 participants
Instead of the pattern of try catch with and asserting on the caught exception leverage Assert.Throws from NUnit. With this approach if an Assert.Fail at the end of the try block is missing a regression won't silently pass. Since this is a style change feel free to close if the team likes the existing pattern.
This PR only changes Sync flows as NUnit does have an Assert.ThrowsAsync but it's return is not a Task and so it performs Sync over Async under the hood. Happy to make that change as well if it is desired.