[Backend.Tests] Clean up test style#4162
Conversation
📝 WalkthroughWalkthroughThe PR refactors the test suite to improve null-safety by replacing null-forgiving operators with explicit assertions, converts async patterns to synchronous waits via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
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
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
github-actions
Bot
added
backend
test
goal: MergeDup
component: TreeView
import/export
audio
project
login/signUp
goal
dependencies
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Backend.Tests/Services/StatisticsServiceTests.cs (1)
180-193:⚠️ Potential issue | 🟡 MinorFix non-adjacent nullable dereferences after null-assertion.
NUnit3001 suppresses CS8602 warnings only when the null-check assertion is in the immediately preceding statement. The documented pattern is
Assert.That(x, Is.Not.Null)followed directly byx.Lengthor similar—not arbitrary subsequent accesses.Lines 192–193 and 232–233 (adjacent pairs) are correctly covered by NUnit3001. However,
user.Idis dereferenced at non-adjacent sites:
- Line 186: inside the
foreachloop body- Line 191: inside the
Findlambda (10 lines after assertion)These will still emit CS8602 warnings. Either hoist
user.Idinto a local variable immediately after the assertion, or use a null-forgiving operator on those specific dereferences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend.Tests/Services/StatisticsServiceTests.cs` around lines 180 - 193, After asserting the created user is not null (Assert.That(user, Is.Not.Null)), hoist its Id into a non-null local immediately (e.g., var userId = user.Id) and replace all later non-adjacent dereferences of user.Id (inside the foreach that sets SemanticDomains[0].UserId and inside the Find lambda passed to GetSemanticDomainUserCounts result) to use userId instead; this removes CS8602 warnings without changing behavior (alternatively you may apply the null-forgiving operator on the specific dereferences, but prefer the hoisted userId approach).
🧹 Nitpick comments (3)
Backend.Tests/Services/WordServiceTests.cs (1)
44-44:.Wait()wraps failures inAggregateException— minor diagnostics note.
Task.Wait()re-throws asAggregateException, whereas.Resultunwraps to the original inner exception. Both block synchronously, but a failure on these setup calls will surface with a less informative exception type. Not a correctness issue here since these results are intentionally discarded, but worth knowing if a future mock throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend.Tests/Services/WordServiceTests.cs` at line 44, The test calls _wordService.Create(UserId, new() { ProjectId = ProjId }...) and uses .Wait(), which wraps exceptions in AggregateException; change the call to unwrap the original exception by either awaiting the Task (make the test method async and use await _wordService.Create(...)) or read the Task's Result (e.g., _wordService.Create(...).Result) so failures surface as the original exception instead of AggregateException; update the test method signature if you choose async/await and adjust any using of _wordService.Create accordingly.Backend.Tests/Controllers/UserRoleControllerTests.cs (1)
78-79: Direct cast toOkObjectResultproducesInvalidCastExceptioninstead ofAssertionExceptionon mismatchThe pattern
(OkObjectResult)await controller.Method()(used ~7 times in this file) gives an opaqueInvalidCastExceptionon failure. Replacing with a prior type assertion gives a descriptive NUnit failure message.♻️ Optional: assert type before casting
- var result = (OkObjectResult)await _userRoleController.HasPermission(ProjId, Permission.WordEntry); - Assert.That(result.Value, Is.False); + var resultAction = await _userRoleController.HasPermission(ProjId, Permission.WordEntry); + Assert.That(resultAction, Is.InstanceOf<OkObjectResult>()); + Assert.That(((OkObjectResult)resultAction).Value, Is.False);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend.Tests/Controllers/UserRoleControllerTests.cs` around lines 78 - 79, The tests directly cast the controller response using (OkObjectResult)await _userRoleController.HasPermission(...), which throws InvalidCastException on mismatch; change each occurrence (including the HasPermission call) to first assert the response type with Assert.IsInstanceOf<OkObjectResult>(response) or Assert.That(response, Is.TypeOf<OkObjectResult>()) and then cast (or use response as OkObjectResult and assert not null) before inspecting Value so failures produce descriptive NUnit assertion messages.Backend.Tests/Controllers/SpeakerControllerTests.cs (1)
315-323: Inconsistent cast: useOkObjectResultinstead ofObjectResultinTestUploadConsentAddAudioConsentBoth upload consent tests assert
Is.InstanceOf<OkObjectResult>()on the result, but lines 317–318 cast toObjectResultwhile the equivalent image test (lines 333–334) correctly casts toOkObjectResult. This is inconsistent with the PR's own intent.♻️ Proposed fix
-Assert.That(((ObjectResult)result).Value, Is.InstanceOf<Speaker>()); -var speaker = (Speaker)((ObjectResult)result).Value; +Assert.That(((OkObjectResult)result).Value, Is.InstanceOf<Speaker>()); +var speaker = (Speaker)((OkObjectResult)result).Value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Backend.Tests/Controllers/SpeakerControllerTests.cs` around lines 315 - 323, In TestUploadConsentAddAudioConsent, the test casts the controller result to ObjectResult while earlier asserting it's an OkObjectResult; update the casts to OkObjectResult to match the assert and the image consent test. Locate the usage of _speakerController.UploadConsent(ProjId, _speaker.Id, _file) and change the cast of the returned result and the extraction of Value to use OkObjectResult (and the test-local variable currently named speaker) so the assertions use the OkObjectResult.Value typed as Speaker and remain consistent with the equivalent image consent test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Backend.Tests/Controllers/LiftControllerTests.cs`:
- Around line 259-261: Add an explicit null check for the result of the "as"
cast before asserting the count: after verifying result is an OkObjectResult in
the test in LiftControllerTests, assert that the variable writingSystems (the
cast of ((OkObjectResult)result).Value as List<WritingSystem>) is not null
(e.g., Assert.That(writingSystems, Is.Not.Null)) before the existing
Assert.That(writingSystems, Has.Count.Not.Zero) so a failed cast yields a clear
assertion rather than a PropertyNotFoundException.
In `@Backend.Tests/Controllers/UserControllerTests.cs`:
- Around line 349-357: The test currently casts the controller response to
List<UserProjectInfo> with "var projects = result.Value as
List<UserProjectInfo>;" and then calls projects.Exists(...) without asserting
non-null; add an explicit null-guard assertion before any use of projects (e.g.,
Assert.That(projects, Is.Not.Null)) in the UserControllerTests test that calls
_userController.GetUserProjects(user.Id) so the test fails cleanly if the
controller returned an unexpected type rather than throwing a
NullReferenceException.
In `@Backend.Tests/Controllers/UserRoleControllerTests.cs`:
- Around line 279-282: The test incorrectly relies on input mutation by
discarding the return value from _userRepo.Create(); change the pattern to
capture and use the returned object (e.g., assign the result of
_userRepo.Create(user) to a variable and use its Id when setting userId) so that
the test uses the created/returned User instance rather than the original input
User; update the references to user.Id/userId to use the returned object from
_userRepo.Create().
In `@Backend.Tests/Services/MergeServiceTests.cs`:
- Around line 38-42: TearDown currently calls _cache.Dispose() unconditionally
which can throw if SetUp failed and _cache is null; update the TearDown method
(the TearDown attribute method) to null-guard _cache before disposing (e.g.,
check _cache != null or use null-conditional dispose) and optionally set _cache
to null after disposing to avoid repeated disposal.
---
Outside diff comments:
In `@Backend.Tests/Services/StatisticsServiceTests.cs`:
- Around line 180-193: After asserting the created user is not null
(Assert.That(user, Is.Not.Null)), hoist its Id into a non-null local immediately
(e.g., var userId = user.Id) and replace all later non-adjacent dereferences of
user.Id (inside the foreach that sets SemanticDomains[0].UserId and inside the
Find lambda passed to GetSemanticDomainUserCounts result) to use userId instead;
this removes CS8602 warnings without changing behavior (alternatively you may
apply the null-forgiving operator on the specific dereferences, but prefer the
hoisted userId approach).
---
Duplicate comments:
In `@Backend.Tests/Services/StatisticsServiceTests.cs`:
- Around line 199-233: The test reads user.Id in non-adjacent places which
defeats the null-safety assertion — immediately after creating and asserting the
user from _userRepo.Create(GetUserWithProjId()).Result, capture user.Id into a
local non-nullable variable (e.g., var userId = user.Id) and then use that
userId when initializing olderDomain.UserId/newerDomain.UserId and in the Find
lambda (result.Find(uc => uc.Id == userId)); keep the existing assertions
(Assert.That(user, Is.Not.Null) and Assert.That(userCount, Is.Not.Null))
adjacent to the creation and use sites to preserve null-check semantics for the
compiler.
In `@Backend.Tests/Services/WordServiceTests.cs`:
- Around line 173-175: The test is synchronously blocking on async operations
using .Wait(), which can cause AggregateException; change the test to be async
by making the test method return Task and replace .Wait() calls on
_wordRepo.Create(oldWordSameProj) and _wordRepo.Create(oldWordDiffProj) with
await (or use await Task.WhenAll(...)) so the async creates are awaited properly
within WordServiceTests.
---
Nitpick comments:
In `@Backend.Tests/Controllers/SpeakerControllerTests.cs`:
- Around line 315-323: In TestUploadConsentAddAudioConsent, the test casts the
controller result to ObjectResult while earlier asserting it's an
OkObjectResult; update the casts to OkObjectResult to match the assert and the
image consent test. Locate the usage of _speakerController.UploadConsent(ProjId,
_speaker.Id, _file) and change the cast of the returned result and the
extraction of Value to use OkObjectResult (and the test-local variable currently
named speaker) so the assertions use the OkObjectResult.Value typed as Speaker
and remain consistent with the equivalent image consent test.
In `@Backend.Tests/Controllers/UserRoleControllerTests.cs`:
- Around line 78-79: The tests directly cast the controller response using
(OkObjectResult)await _userRoleController.HasPermission(...), which throws
InvalidCastException on mismatch; change each occurrence (including the
HasPermission call) to first assert the response type with
Assert.IsInstanceOf<OkObjectResult>(response) or Assert.That(response,
Is.TypeOf<OkObjectResult>()) and then cast (or use response as OkObjectResult
and assert not null) before inspecting Value so failures produce descriptive
NUnit assertion messages.
In `@Backend.Tests/Services/WordServiceTests.cs`:
- Line 44: The test calls _wordService.Create(UserId, new() { ProjectId = ProjId
}...) and uses .Wait(), which wraps exceptions in AggregateException; change the
call to unwrap the original exception by either awaiting the Task (make the test
method async and use await _wordService.Create(...)) or read the Task's Result
(e.g., _wordService.Create(...).Result) so failures surface as the original
exception instead of AggregateException; update the test method signature if you
choose async/await and adjust any using of _wordService.Create accordingly.
| Assert.That(result, Is.TypeOf<OkObjectResult>()); | ||
| var writingSystems = (result as OkObjectResult)!.Value as List<WritingSystem>; | ||
| var writingSystems = ((OkObjectResult)result).Value as List<WritingSystem>; | ||
| Assert.That(writingSystems, Has.Count.Not.Zero); |
There was a problem hiding this comment.
Missing null check for writingSystems after as cast
The as cast on line 260 returns null if Value isn't a List<WritingSystem>. NUnit's Has.Count constraint accesses the Count property via reflection and throws a PropertyNotFoundException on null rather than producing a clean assertion failure. An explicit null assertion is consistent with how the same issue was handled elsewhere in this PR (e.g., lines 534, 602).
🛡️ Proposed fix
var writingSystems = ((OkObjectResult)result).Value as List<WritingSystem>;
+Assert.That(writingSystems, Is.Not.Null);
Assert.That(writingSystems, Has.Count.Not.Zero);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Assert.That(result, Is.TypeOf<OkObjectResult>()); | |
| var writingSystems = (result as OkObjectResult)!.Value as List<WritingSystem>; | |
| var writingSystems = ((OkObjectResult)result).Value as List<WritingSystem>; | |
| Assert.That(writingSystems, Has.Count.Not.Zero); | |
| Assert.That(result, Is.TypeOf<OkObjectResult>()); | |
| var writingSystems = ((OkObjectResult)result).Value as List<WritingSystem>; | |
| Assert.That(writingSystems, Is.Not.Null); | |
| Assert.That(writingSystems, Has.Count.Not.Zero); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend.Tests/Controllers/LiftControllerTests.cs` around lines 259 - 261, Add
an explicit null check for the result of the "as" cast before asserting the
count: after verifying result is an OkObjectResult in the test in
LiftControllerTests, assert that the variable writingSystems (the cast of
((OkObjectResult)result).Value as List<WritingSystem>) is not null (e.g.,
Assert.That(writingSystems, Is.Not.Null)) before the existing
Assert.That(writingSystems, Has.Count.Not.Zero) so a failed cast yields a clear
assertion rather than a PropertyNotFoundException.
| var result = (ObjectResult)_userController.GetUserProjects(user.Id).Result; | ||
| var projects = result.Value as List<UserProjectInfo>; | ||
|
|
||
| // Verify both projects are returned with correct roles | ||
| Assert.That(projects, Has.Count.EqualTo(2)); | ||
| Assert.That(projects!.Exists(p => p.ProjectId == project1.Id && p.ProjectIsActive == project1.IsActive | ||
| Assert.That(projects.Exists(p => p.ProjectId == project1.Id && p.ProjectIsActive == project1.IsActive | ||
| && p.ProjectName == project1.Name && p.Role == userRole1.Role)); | ||
| Assert.That(projects.Exists(p => p.ProjectId == project2.Id && p.ProjectIsActive == project2.IsActive | ||
| && p.ProjectName == project2.Name && p.Role == userRole2.Role)); |
There was a problem hiding this comment.
Missing null assertion for projects before projects.Exists(...)
projects is obtained via an as cast on line 350 and can be null if the controller returns an unexpected type. The PR removes the null-forgiving ! on line 354 but doesn't add an Assert.That(projects, Is.Not.Null) guard, leaving a potential NullReferenceException instead of a clean test failure.
🛡️ Proposed fix
var result = (ObjectResult)_userController.GetUserProjects(user.Id).Result;
var projects = result.Value as List<UserProjectInfo>;
+
+Assert.That(projects, Is.Not.Null);
// Verify both projects are returned with correct roles
Assert.That(projects, Has.Count.EqualTo(2));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = (ObjectResult)_userController.GetUserProjects(user.Id).Result; | |
| var projects = result.Value as List<UserProjectInfo>; | |
| // Verify both projects are returned with correct roles | |
| Assert.That(projects, Has.Count.EqualTo(2)); | |
| Assert.That(projects!.Exists(p => p.ProjectId == project1.Id && p.ProjectIsActive == project1.IsActive | |
| Assert.That(projects.Exists(p => p.ProjectId == project1.Id && p.ProjectIsActive == project1.IsActive | |
| && p.ProjectName == project1.Name && p.Role == userRole1.Role)); | |
| Assert.That(projects.Exists(p => p.ProjectId == project2.Id && p.ProjectIsActive == project2.IsActive | |
| && p.ProjectName == project2.Name && p.Role == userRole2.Role)); | |
| var result = (ObjectResult)_userController.GetUserProjects(user.Id).Result; | |
| var projects = result.Value as List<UserProjectInfo>; | |
| Assert.That(projects, Is.Not.Null); | |
| // Verify both projects are returned with correct roles | |
| Assert.That(projects, Has.Count.EqualTo(2)); | |
| Assert.That(projects.Exists(p => p.ProjectId == project1.Id && p.ProjectIsActive == project1.IsActive | |
| && p.ProjectName == project1.Name && p.Role == userRole1.Role)); | |
| Assert.That(projects.Exists(p => p.ProjectId == project2.Id && p.ProjectIsActive == project2.IsActive | |
| && p.ProjectName == project2.Name && p.Role == userRole2.Role)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend.Tests/Controllers/UserControllerTests.cs` around lines 349 - 357, The
test currently casts the controller response to List<UserProjectInfo> with "var
projects = result.Value as List<UserProjectInfo>;" and then calls
projects.Exists(...) without asserting non-null; add an explicit null-guard
assertion before any use of projects (e.g., Assert.That(projects, Is.Not.Null))
in the UserControllerTests test that calls
_userController.GetUserProjects(user.Id) so the test fails cleanly if the
controller returned an unexpected type rather than throwing a
NullReferenceException.
| var user = new User { ProjectRoles = { [ProjId] = userRole.Id } }; | ||
| var userId = (await _userRepo.Create(user))!.Id; | ||
| await _userRepo.Create(user); | ||
| Assert.That(user, Is.Not.Null); | ||
| var userId = user.Id; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*UserRepositoryMock*" -type fRepository: sillsdev/TheCombine
Length of output: 106
🏁 Script executed:
find . -name "*Mock*.cs" -type f | head -20Repository: sillsdev/TheCombine
Length of output: 1013
🏁 Script executed:
fd -t f "UserRepository" | grep -i mockRepository: sillsdev/TheCombine
Length of output: 104
🏁 Script executed:
cat -n ./Backend.Tests/Mocks/UserRepositoryMock.csRepository: sillsdev/TheCombine
Length of output: 4608
🏁 Script executed:
cd Backend.Tests/Controllers && wc -l UserRoleControllerTests.csRepository: sillsdev/TheCombine
Length of output: 93
🏁 Script executed:
sed -n '270,290p' ./Backend.Tests/Controllers/UserRoleControllerTests.csRepository: sillsdev/TheCombine
Length of output: 1065
🏁 Script executed:
sed -n '220,250p' ./Backend.Tests/Controllers/UserRoleControllerTests.csRepository: sillsdev/TheCombine
Length of output: 1403
🏁 Script executed:
rg -n "await _userRepo.Create" ./Backend.Tests/Controllers/UserRoleControllerTests.cs -A 2 -B 1Repository: sillsdev/TheCombine
Length of output: 5883
Use the established pattern: capture the return value from _userRepo.Create()
Lines 279–282 discard the return value of _userRepo.Create() and rely on input mutation to set user.Id. Every other test in this file (20+ instances across lines 86–457) captures the return value instead. This inconsistency creates unnecessary fragility; while the mock does mutate the input, the returned object is what the mock stores internally. Align with the pattern used throughout the file.
Fix inconsistency with other tests
- var user = new User { ProjectRoles = { [ProjId] = userRole.Id } };
- await _userRepo.Create(user);
- Assert.That(user, Is.Not.Null);
- var userId = user.Id;
+ var user = await _userRepo.Create(new User { ProjectRoles = { [ProjId] = userRole.Id } });
+ Assert.That(user, Is.Not.Null);
+ var userId = user!.Id;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var user = new User { ProjectRoles = { [ProjId] = userRole.Id } }; | |
| var userId = (await _userRepo.Create(user))!.Id; | |
| await _userRepo.Create(user); | |
| Assert.That(user, Is.Not.Null); | |
| var userId = user.Id; | |
| var user = await _userRepo.Create(new User { ProjectRoles = { [ProjId] = userRole.Id } }); | |
| Assert.That(user, Is.Not.Null); | |
| var userId = user!.Id; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend.Tests/Controllers/UserRoleControllerTests.cs` around lines 279 - 282,
The test incorrectly relies on input mutation by discarding the return value
from _userRepo.Create(); change the pattern to capture and use the returned
object (e.g., assign the result of _userRepo.Create(user) to a variable and use
its Id when setting userId) so that the test uses the created/returned User
instance rather than the original input User; update the references to
user.Id/userId to use the returned object from _userRepo.Create().
| [TearDown] | ||
| public void TearDown() | ||
| { | ||
| _cache.Dispose(); | ||
| } |
There was a problem hiding this comment.
TearDown should null-guard _cache before calling Dispose().
NUnit executes [TearDown] even when [SetUp] throws. If the SetUp initialization at lines 29–30 ever fails before _cache is assigned, _cache retains its null! initial value and the unconditional _cache.Dispose() will throw a NullReferenceException in teardown, obscuring the original failure.
🛡️ Proposed fix
- [TearDown]
- public void TearDown()
- {
- _cache.Dispose();
- }
+ [TearDown]
+ public void TearDown()
+ {
+ _cache?.Dispose();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [TearDown] | |
| public void TearDown() | |
| { | |
| _cache.Dispose(); | |
| } | |
| [TearDown] | |
| public void TearDown() | |
| { | |
| _cache?.Dispose(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Backend.Tests/Services/MergeServiceTests.cs` around lines 38 - 42, TearDown
currently calls _cache.Dispose() unconditionally which can throw if SetUp failed
and _cache is null; update the TearDown method (the TearDown attribute method)
to null-guard _cache before disposing (e.g., check _cache != null or use
null-conditional dispose) and optionally set _cache to null after disposing to
avoid repeated disposal.
imnasnainaec
changed the title
|
Partially replaced by #4177 Anything else can be redone later. |
1 participant
Todo in this pr: Update the C# style guide per #4165
Partially replaced by #4177
This change is
Summary by CodeRabbit
Tests
Chores