Add .NET 9.0 to app and tests; set roll-forward; update Rider config - #20
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as SerializationTests
participant S as Serializer
participant FS as FileSystem
rect #f8f9fa
Note over T,S: Tests set a fixed relative directory
T->>S: SetSerializationDirectory("./test-serialization")
end
T->>S: Serialize(object)
S->>FS: Write ./test-serialization/<file>
FS-->>S: OK
S-->>T: Serialized result
T->>S: Deserialize(...)
S->>FS: Read ./test-serialization/<file>
FS-->>S: file contents
S-->>T: Deserialized object
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
yTools.Tests/SerializationTests.cs (1)
71-84: Consider updating this test to use the same directory pattern.For consistency,
CheckSerializationDirCreatedcould also use./test-serializationas the base directory instead ofLocalApplicationData/yTools. This would:
- Make all tests use the same directory strategy
- Simplify the cleanup logic to target a single location
- Avoid platform-specific path issues with
LocalApplicationDataIf you prefer to keep the current approach, the cleanup logic as modified in the previous comment will handle both cases correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
yTools.Tests/SerializationTests.cs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
yTools.Tests/SerializationTests.cs (3)
yTools/Serialization/BinarySerializer.cs (1)
SetSerializationDirectory(34-40)yTools/Serialization/XmlSerializer.cs (1)
SetSerializationDirectory(32-38)yTools/Serialization/JsonSerializer.cs (1)
SetSerializationDirectory(32-38)
🪛 GitHub Actions: .NET
yTools.Tests/SerializationTests.cs
[error] 17-17: TestCleanup method yTools.Tests.SerializationTests.Cleanup threw exception. System.IO.DirectoryNotFoundException: Could not find a part of the path '/home/runner/.local/share/yTools/json'.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
yTools.Tests/SerializationTests.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
yTools.Tests/SerializationTests.cs (3)
yTools/Serialization/BinarySerializer.cs (1)
SetSerializationDirectory(34-40)yTools/Serialization/JsonSerializer.cs (1)
SetSerializationDirectory(32-38)yTools/Serialization/XmlSerializer.cs (1)
SetSerializationDirectory(32-38)
🔇 Additional comments (2)
yTools.Tests/SerializationTests.cs (2)
22-22: Good change to use relative test directory.Using a fixed relative path
./test-serializationinstead ofLocalApplicationDataimproves test isolation, reproducibility, and portability.
47-47: Consistent test directory usage.The change mirrors the JSON test approach (line 22), maintaining consistency across serialization tests.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
yTools.Tests/SerializationTests.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
yTools.Tests/SerializationTests.cs (4)
yTools/General.cs (1)
General(6-25)yTools/Serialization/XmlSerializer.cs (1)
SetSerializationDirectory(32-38)yTools/Serialization/JsonSerializer.cs (1)
SetSerializationDirectory(32-38)yTools/Serialization/BinarySerializer.cs (1)
SetSerializationDirectory(34-40)
🪛 GitHub Actions: .NET
yTools.Tests/SerializationTests.cs
[error] 16-16: TestCleanup method yTools.Tests.SerializationTests.Cleanup threw exception. System.IO.IOException: Directory not empty : '/home/runner/.local/share/yTools/json'.
[error] 82-82: Assert.IsTrue failed in CheckSerializationDirCreated().
🔇 Additional comments (2)
yTools.Tests/SerializationTests.cs (2)
29-29: LGTM!The change to use a relative test directory (
./test-serialization) aligns with the PR objectives and is properly handled by the cleanup method.
54-54: LGTM!Consistent with the JSON test changes; the relative test directory is properly managed by the cleanup method.
| [TestCleanup] | ||
| public void Cleanup() | ||
| { | ||
| const string testDirectory = "yTools"; | ||
|
|
||
| var fullDir = $"{Environment.GetFolderPath( | ||
| Environment.SpecialFolder.LocalApplicationData | ||
| )}{_sep}{testDirectory}"; | ||
| if (Directory.Exists(fullDir)) | ||
| Directory.Delete(fullDir, true); | ||
|
|
||
| if (Directory.Exists("./test-serialization")) | ||
| Directory.Delete("./test-serialization", true); | ||
| } |
There was a problem hiding this comment.
Address cleanup failures caused by locked file handles.
The cleanup logic now correctly handles both directories (as requested in past reviews). However, the pipeline failure at line 16 indicates Directory.Delete is failing with "Directory not empty" on the /yTools/json subdirectory, likely because file handles remain open after tests complete.
The CheckSerializationDirCreated test (lines 73-86) creates serializer instances without explicit disposal. If these serializers implement IDisposable and don't close file handles automatically, cleanup will fail in CI environments.
Consider adding defensive cleanup with retry logic:
[TestCleanup]
public void Cleanup()
{
const string testDirectory = "yTools";
var fullDir = $"{Environment.GetFolderPath(
Environment.SpecialFolder.LocalApplicationData
)}{_sep}{testDirectory}";
- if (Directory.Exists(fullDir))
- Directory.Delete(fullDir, true);
+
+ if (Directory.Exists(fullDir))
+ {
+ try
+ {
+ Directory.Delete(fullDir, true);
+ }
+ catch (IOException)
+ {
+ // Retry after brief delay to handle locked file handles
+ Thread.Sleep(100);
+ if (Directory.Exists(fullDir))
+ Directory.Delete(fullDir, true);
+ }
+ }
if (Directory.Exists("./test-serialization"))
- Directory.Delete("./test-serialization", true);
+ {
+ try
+ {
+ Directory.Delete("./test-serialization", true);
+ }
+ catch (IOException)
+ {
+ Thread.Sleep(100);
+ if (Directory.Exists("./test-serialization"))
+ Directory.Delete("./test-serialization", true);
+ }
+ }
}Alternatively, ensure serializers in CheckSerializationDirCreated are properly disposed by using using statements if they implement IDisposable.
📝 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.
| [TestCleanup] | |
| public void Cleanup() | |
| { | |
| const string testDirectory = "yTools"; | |
| var fullDir = $"{Environment.GetFolderPath( | |
| Environment.SpecialFolder.LocalApplicationData | |
| )}{_sep}{testDirectory}"; | |
| if (Directory.Exists(fullDir)) | |
| Directory.Delete(fullDir, true); | |
| if (Directory.Exists("./test-serialization")) | |
| Directory.Delete("./test-serialization", true); | |
| } | |
| [TestCleanup] | |
| public void Cleanup() | |
| { | |
| const string testDirectory = "yTools"; | |
| var fullDir = $"{Environment.GetFolderPath( | |
| Environment.SpecialFolder.LocalApplicationData | |
| )}{_sep}{testDirectory}"; | |
| if (Directory.Exists(fullDir)) | |
| { | |
| try | |
| { | |
| Directory.Delete(fullDir, true); | |
| } | |
| catch (IOException) | |
| { | |
| // Retry after brief delay to handle locked file handles | |
| Thread.Sleep(100); | |
| if (Directory.Exists(fullDir)) | |
| Directory.Delete(fullDir, true); | |
| } | |
| } | |
| if (Directory.Exists("./test-serialization")) | |
| { | |
| try | |
| { | |
| Directory.Delete("./test-serialization", true); | |
| } | |
| catch (IOException) | |
| { | |
| Thread.Sleep(100); | |
| if (Directory.Exists("./test-serialization")) | |
| Directory.Delete("./test-serialization", true); | |
| } | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: .NET
[error] 16-16: TestCleanup method yTools.Tests.SerializationTests.Cleanup threw exception. System.IO.IOException: Directory not empty : '/home/runner/.local/share/yTools/json'.
🤖 Prompt for AI Agents
In yTools.Tests/SerializationTests.cs around lines 8 to 20, the cleanup
Directory.Delete is failing because file handles remain open; update the cleanup
to defensively retry deletes with short backoff and catch
IOException/UnauthorizedAccessException (e.g., attempt up to 5 times with
Thread.Sleep between attempts) before giving up, and also modify the
CheckSerializationDirCreated test to ensure any serializer instances are
properly disposed (wrap them in using blocks or call Dispose) so file handles
are closed promptly.
| jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out _, out _); | ||
| Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}json")); |
There was a problem hiding this comment.
Check serialization results to diagnose the pipeline failure.
Lines 81 and 84 discard the serialization return value and exception with out _, so if serialization fails, the test won't detect it until the assertion at line 82—which is currently failing in CI. Other tests in this file (e.g., CheckObjectsJsonSerialized) properly check both the return value and exception.
Apply this diff to detect serialization failures:
- jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out _, out _);
+ var jsonSuccess = jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out var jsonEx, out _);
+ if (!jsonSuccess && jsonEx != null) throw jsonEx;
Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}json"));
- xmlSerializer.Serialize(testObj, "xml.xml", $@"{baseDir}{_sep}xml", out _, out _);
+ var xmlSuccess = xmlSerializer.Serialize(testObj, "xml.xml", $@"{baseDir}{_sep}xml", out var xmlEx, out _);
+ if (!xmlSuccess && xmlEx != null) throw xmlEx;
Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}xml"));This change will surface the root cause of the failed assertion and help resolve the pipeline failure.
📝 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.
| jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out _, out _); | |
| Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}json")); | |
| var jsonSuccess = jsonSerializer.Serialize( | |
| testObj, | |
| "json.json", | |
| $@"{baseDir}{_sep}json", | |
| out var jsonEx, | |
| out _ | |
| ); | |
| if (!jsonSuccess && jsonEx != null) throw jsonEx; | |
| Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}json")); | |
| var xmlSuccess = xmlSerializer.Serialize( | |
| testObj, | |
| "xml.xml", | |
| $@"{baseDir}{_sep}xml", | |
| out var xmlEx, | |
| out _ | |
| ); | |
| if (!xmlSuccess && xmlEx != null) throw xmlEx; | |
| Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}xml")); |
🧰 Tools
🪛 GitHub Actions: .NET
[error] 82-82: Assert.IsTrue failed in CheckSerializationDirCreated().
🤖 Prompt for AI Agents
In yTools.Tests/SerializationTests.cs around lines 81-82, the call to
jsonSerializer.Serialize discards its return value and exception (uses out _),
so serialization failures are hidden until the Directory.Exists assertion;
capture the method outputs instead and assert they indicate success: store the
bool return and the exception out parameter into variables,
Assert.IsTrue(returnValue) and Assert.IsNull(exception) (then keep the directory
existence check). This will surface serialization errors immediately and help
diagnose the CI failure.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
yTools.Tests/SerializationTests.cs (1)
88-89: Check serialization results to prevent hidden failures.The serialization calls discard their return values and exceptions using
out _, so failures are hidden until the directory existence assertion. This was flagged as a critical issue in past reviews and may be causing the pipeline failures.Other tests in this file (e.g.,
CheckObjectsJsonSerializedat lines 38-41) properly check both the return value and exception. Apply the same pattern here:- jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out _, out _); + var jsonSuccess = jsonSerializer.Serialize(testObj, "json.json", $@"{baseDir}{_sep}json", out var jsonEx, out _); + if (!jsonSuccess && jsonEx != null) throw jsonEx; Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}json")); - xmlSerializer.Serialize(testObj, "xml.xml", $@"{baseDir}{_sep}xml", out _, out _); + var xmlSuccess = xmlSerializer.Serialize(testObj, "xml.xml", $@"{baseDir}{_sep}xml", out var xmlEx, out _); + if (!xmlSuccess && xmlEx != null) throw xmlEx; Assert.IsTrue(Directory.Exists($"{baseDir}{_sep}xml"));This will surface serialization errors immediately and help diagnose CI failures.
Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
yTools.Tests/SerializationTests.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
yTools.Tests/SerializationTests.cs (4)
yTools/General.cs (1)
General(6-25)yTools/Serialization/BinarySerializer.cs (1)
SetSerializationDirectory(34-40)yTools/Serialization/JsonSerializer.cs (1)
SetSerializationDirectory(32-38)yTools/Serialization/XmlSerializer.cs (1)
SetSerializationDirectory(32-38)
🔇 Additional comments (1)
yTools.Tests/SerializationTests.cs (1)
36-36: LGTM!The transition to using a fixed relative directory
./test-serializationsimplifies test setup and aligns with the updated cleanup logic.
| try | ||
| { | ||
| const string testDirectory = "yTools"; | ||
| var fullDir = $"{Environment.GetFolderPath( | ||
| Environment.SpecialFolder.LocalApplicationData | ||
| )}{_sep}{testDirectory}"; | ||
| if (Directory.Exists(fullDir)) | ||
| Directory.Delete(fullDir, true); | ||
|
|
||
| if (Directory.Exists("./test-serialization")) | ||
| Directory.Delete("./test-serialization", true); | ||
| } | ||
| catch | ||
| { | ||
| Console.WriteLine("Something went wrong when cleaning up."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Implement retry logic for locked file handles.
The try-catch wrapper silences cleanup failures but doesn't address the locked file handle issue flagged in past reviews. Pipeline failures indicate Directory.Delete fails with "Directory not empty" errors, likely due to file handles remaining open after tests complete.
Consider implementing retry logic with backoff as suggested in previous reviews:
[TestCleanup]
public void Cleanup()
{
- try
- {
- const string testDirectory = "yTools";
- var fullDir = $"{Environment.GetFolderPath(
- Environment.SpecialFolder.LocalApplicationData
- )}{_sep}{testDirectory}";
- if (Directory.Exists(fullDir))
- Directory.Delete(fullDir, true);
+ const string testDirectory = "yTools";
+ var fullDir = $"{Environment.GetFolderPath(
+ Environment.SpecialFolder.LocalApplicationData
+ )}{_sep}{testDirectory}";
+
+ TryDeleteDirectory(fullDir);
+ TryDeleteDirectory("./test-serialization");
+}
+
+private static void TryDeleteDirectory(string path, int maxAttempts = 3)
+{
+ if (!Directory.Exists(path)) return;
+
+ for (int attempt = 0; attempt < maxAttempts; attempt++)
+ {
+ try
+ {
+ Directory.Delete(path, true);
+ return;
+ }
+ catch (IOException) when (attempt < maxAttempts - 1)
+ {
+ Thread.Sleep(100);
+ }
+ catch (UnauthorizedAccessException) when (attempt < maxAttempts - 1)
+ {
+ Thread.Sleep(100);
+ }
+ }
+
+ // Log failure after all retries
+ Console.WriteLine($"Failed to delete directory: {path}");
+}Alternatively, ensure serializers in CheckSerializationDirCreated are properly disposed using using statements if they implement IDisposable.
Based on past review comments.
🏁 Script executed:
#!/bin/bash
# Locate the test file
TEST_FILE=$(find . -type f -name 'SerializationTests.cs')
echo "Found file: $TEST_FILE"
# Show lines around the TestCleanup method
rg -nC3 '\[TestCleanup\]' "$TEST_FILE"
# Show the full Cleanup method
sed -n '1,200p' "$TEST_FILE"Length of output: 6160
🏁 Script executed:
#!/bin/bash
# Find JsonSerializer definition
rg -nP 'class\s+JsonSerializer' -C3 .
# Find XmlSerializer definition
rg -nP 'class\s+XmlSerializer' -C3 .Length of output: 799
Replace broad try-catch in Cleanup with retry logic for Directory.Delete
Implement a helper like TryDeleteDirectory(path, maxAttempts) that retries on IOException/UnauthorizedAccessException with a short backoff, and call it for both cleanup paths instead of silencing failures:
private static void TryDeleteDirectory(string path, int maxAttempts = 3)
{
if (!Directory.Exists(path)) return;
for (int i = 1; i <= maxAttempts; i++)
{
try { Directory.Delete(path, true); return; }
catch (IOException) when (i < maxAttempts) { Thread.Sleep(100); }
catch (UnauthorizedAccessException) when (i < maxAttempts) { Thread.Sleep(100); }
}
Console.WriteLine($"Failed to delete directory after retries: {path}");
}Call TryDeleteDirectory(fullDir) and TryDeleteDirectory("./test-serialization") in Cleanup.
🤖 Prompt for AI Agents
In yTools.Tests/SerializationTests.cs around lines 11 to 26, replace the broad
try-catch that swallows all exceptions during directory cleanup with a retrying
helper: add a private static TryDeleteDirectory(string path, int maxAttempts =
3) that returns early if the path doesn't exist and attempts
Directory.Delete(path, true) up to maxAttempts, catching IOException and
UnauthorizedAccessException and retrying after a short Thread.Sleep backoff when
i < maxAttempts, and logging a failure message if all attempts fail; then call
TryDeleteDirectory(fullDir) and TryDeleteDirectory("./test-serialization") from
the Cleanup block instead of the existing try-catch.
1 participant
Summary by CodeRabbit
New Features
Chores
Tests