Skip to content

test: improve test quality — [Collection] isolation, locale-safe assertions, stronger sanitize check #368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laurentiu021 merged 1 commit into from
May 15, 2026
Merged

test: improve test quality — [Collection] isolation, locale-safe assertions, stronger sanitize check #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion SysManager/SysManager.Tests/CleanupCategoryHumanSizeExtendedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,9 @@ public void LargeFileEntry_LastModifiedDisplay_FormatsCorrectly()
LastModified = new DateTime(2024, 3, 1)
};
Assert.Contains("2024", entry.LastModifiedDisplay);
Assert.Contains("Mar", entry.LastModifiedDisplay);
// Use the expected month name from the current culture to avoid locale failures
var expectedMonth = new DateTime(2024, 3, 1).ToString("MMM", System.Globalization.CultureInfo.CurrentCulture);
Assert.Contains(expectedMonth, entry.LastModifiedDisplay);
}

[Fact]
Expand Down
4 changes: 3 additions & 1 deletion SysManager/SysManager.Tests/LogServiceSanitizeEdgeCaseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ public void SanitizePath_NoUserPath_ReturnsUnchanged()
}

[Fact]
public void SanitizePath_MultipleUsersInPath_ReplacesFirst()
public void SanitizePath_MultipleUsersInPath_ReplacesAll()
{
var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt");
Assert.Contains("[user]", result);
Assert.DoesNotContain("alice", result);
Assert.DoesNotContain("bob", result);
Comment on lines +42 to +47

@coderabbitai coderabbitai Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Tighten this test to verify the exact sanitized output.

At Line 45–Line 47, the current checks can still pass if usernames are removed instead of both being replaced with [user]. Since the test is ReplacesAll, assert the full expected string.

✅ Suggested change 
     var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt");
-    Assert.Contains("[user]", result);
-    Assert.DoesNotContain("alice", result);
-    Assert.DoesNotContain("bob", result);
+    Assert.Equal(@"C:\Users\[user]\backup\C:\Users\[user]\file.txt", result);
📝 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.

Suggested change
public void SanitizePath_MultipleUsersInPath_ReplacesAll()
{
var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt");
Assert.Contains("[user]", result);
Assert.DoesNotContain("alice", result);
Assert.DoesNotContain("bob", result);
public void SanitizePath_MultipleUsersInPath_ReplacesAll()
{
var result = LogService.SanitizePath(@"C:\Users\alice\backup\C:\Users\bob\file.txt");
Assert.Equal(@"C:\Users\[user]\backup\C:\Users\[user]\file.txt", result);
}
🤖 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 `@SysManager/SysManager.Tests/LogServiceSanitizeEdgeCaseTests.cs` around lines
42 - 47, Replace the loose assertions in
SanitizePath_MultipleUsersInPath_ReplacesAll so the test checks the exact
sanitized output from LogService.SanitizePath instead of just
contains/does-not-contain; assert that result equals the expected string
"C:\Users\[user]\backup\C:\Users\[user]\file.txt" (use the same
verbatim/backslash format as the input) by replacing
Assert.Contains/DoesNotContain calls with a single Assert.Equal(expected,
result) against that exact expected value.

}

[Fact]
Expand Down
23 changes: 16 additions & 7 deletions SysManager/SysManager.Tests/OperationLockServiceEdgeCaseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

namespace SysManager.Tests;

[Collection("OperationLock")]
public class OperationLockServiceEdgeCaseTests
{
private static OperationLockService Service => OperationLockService.Instance;
Expand Down Expand Up @@ -117,14 +118,22 @@ public void ActiveOperations_ReturnsSnapshot()
public void PropertyChanged_FiredOnAcquire()
{
var changed = new List<string>();
Service.PropertyChanged += (_, e) => changed.Add(e.PropertyName!);
void handler(object? _, System.ComponentModel.PropertyChangedEventArgs e) => changed.Add(e.PropertyName!);
Service.PropertyChanged += handler;

var handle = Service.TryAcquire(OperationCategory.Disk, "PropChanged");
Assert.NotNull(handle);
handle.Dispose();

Assert.Contains("ActiveOperations", changed);
Assert.Contains("HasActiveOperations", changed);
try
{
var handle = Service.TryAcquire(OperationCategory.Disk, "PropChanged");
Assert.NotNull(handle);
handle.Dispose();

Assert.Contains("ActiveOperations", changed);
Assert.Contains("HasActiveOperations", changed);
}
finally
{
Service.PropertyChanged -= handler;
}
}

[Fact]
Expand Down
Loading