Migrate to .NET 10, adopt TUnit, add integration tests and MCAF guidance#2
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66416bcf6b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <Solution> | ||
| <Folder Name="/src/"> | ||
| <Project Path="src/RagSharp.CodeGraph.Cli/RagSharp.CodeGraph.Cli.csproj" /> |
There was a problem hiding this comment.
Switching the repo to a .slnx solution means IndexAsync no longer finds a solution when given the repo root, because CodeGraphIndexer.FindSolution only scans for *.sln. In that case it falls back to the first .csproj it finds and indexes only a single project, which is a functional regression for multi-project indexing. Consider teaching FindSolution to detect .slnx (or prefer it when present) so root indexing still captures the full solution.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the RagSharp repository by migrating from .NET 8 to .NET 10, replacing xUnit with TUnit for testing, and introducing comprehensive integration tests. The migration includes updating all project files to target net10.0 exclusively, converting the solution to the XML-based .slnx format, and adding repository conventions documentation for agent-driven workflows.
Key changes:
- Unified target framework from multi-targeting (net8.0/net10.0) to net10.0 only across all projects
- Replaced xUnit test framework with TUnit, including conversion of test attributes and assertions, plus adding 100 parameterized integration tests
- Added MSBuild.Locator registration guards and null safety checks to CodeGraphIndexer, plus updated MSBuildWorkspace API calls for .NET 10 compatibility
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/samples/SampleApp/SampleApp.csproj | Updated sample app to target net10.0 |
| tests/RagSharp.SkillInstaller.Tests/RagSharp.SkillInstaller.Tests.csproj | Migrated test project to net10.0 and TUnit, changed OutputType to Exe |
| tests/RagSharp.SkillInstaller.Tests/InstallerTests.cs | Converted test attributes and assertions from xUnit to TUnit, added new status test |
| tests/RagSharp.CodeGraph.Tests/RagSharp.CodeGraph.Tests.csproj | Migrated test project to net10.0 and TUnit |
| tests/RagSharp.CodeGraph.Tests/IndexTests.cs | Removed old xUnit-based test file |
| tests/RagSharp.CodeGraph.Tests/CodeGraphIntegrationTests.cs | Added comprehensive integration test suite with 100 parameterized test cases |
| src/RagSharp.SkillInstaller/RagSharp.SkillInstaller.csproj | Removed multi-targeting, now targets net10.0 only |
| src/RagSharp.SkillInstaller/Installer.cs | Modified GetStatusAsync return type to maintain consistency |
| src/RagSharp.Packaging/RagSharp.Packaging.csproj | Updated to target net10.0 and fixed packaging paths |
| src/RagSharp.CodeGraph.Store.LiteGraph/RagSharp.CodeGraph.Store.LiteGraph.csproj | Removed multi-targeting, now targets net10.0 only |
| src/RagSharp.CodeGraph.Core/RagSharp.CodeGraph.Core.csproj | Added MSBuild.Locator and Logging.Abstractions packages, removed multi-targeting |
| src/RagSharp.CodeGraph.Core/CodeGraphIndexer.cs | Added MSBuild.Locator registration guards, updated workspace API calls, added null checks for using directives |
| src/RagSharp.CodeGraph.Cli/RagSharp.CodeGraph.Cli.csproj | Removed multi-targeting, now targets net10.0 only |
| ragsharp.slnx | New XML-based solution file replacing traditional .sln |
| ragsharp.sln | Removed legacy solution file |
| global.json | Added testRunner property for .NET 10 testing platform |
| docs/Wiki/ProjectOverview.md | Added project documentation describing components and conventions |
| README.md | Updated requirements to reflect net10.0 as the only target |
| AGENTS.md | Added MCAF agent instructions for repository conventions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var manifest = JsonSerializer.Deserialize<InstallManifest>(await File.ReadAllTextAsync(manifestPath, cancellationToken).ConfigureAwait(false)); | ||
| return new InstallStatus(true, manifest?.Version, manifest?.InstalledAtUtc, targetDir, manifest?.Files ?? Array.Empty<string>()); | ||
| var files = manifest?.Files ?? new List<string>(); |
There was a problem hiding this comment.
The type of the files variable has changed from Array.Empty<string>() to new List<string>(). While both implement IReadOnlyList<string>, this inconsistency means line 75 returns an array while line 80 returns a List. For consistency and to match the expected return type, both should use the same implementation. Consider using Array.Empty<string>() on line 79 instead of new List<string>() to maintain consistency with line 75.
| var files = manifest?.Files ?? new List<string>(); | |
| IReadOnlyList<string> files = (manifest?.Files as IReadOnlyList<string>) ?? Array.Empty<string>(); |
| public static async Task Cleanup() | ||
| { | ||
| if (IndexTestContextHolder.Context is { } context) | ||
| { | ||
| await context.DisposeAsync(); | ||
| IndexTestContextHolder.Context = null; |
There was a problem hiding this comment.
The IndexTestContextHolder.Context static field can be accessed concurrently from multiple tests. While GetContextAsync() uses a semaphore for initialization, the cleanup in CodeGraphTestHooks.Cleanup() at line 211 sets Context = null without synchronization. This could create a race condition where one test is reading the context while cleanup is nulling it out, potentially leading to null reference exceptions or cleanup occurring while tests are still running.
| "allowPrerelease": true, | ||
| "testRunner": "Microsoft.Testing.Platform" |
There was a problem hiding this comment.
The testRunner property is not a standard or documented property of the sdk section in global.json. The global.json schema supports properties like version, rollForward, and allowPrerelease, but testRunner is not recognized. This property will likely be ignored by the .NET SDK and may cause confusion. If this is intended for a specific tool or extension, consider documenting its purpose or removing it if it serves no function.
| "allowPrerelease": true, | |
| "testRunner": "Microsoft.Testing.Platform" | |
| "allowPrerelease": true |
| if (Directory.Exists(RootPath)) | ||
| { | ||
| Directory.Delete(RootPath, true); |
There was a problem hiding this comment.
The Directory.Delete(RootPath, true) call at line 193 may fail on Windows if the SQLite database file is still locked after Store.DisposeAsync(). SQLite can hold locks briefly even after disposal. Consider adding retry logic or a small delay to ensure the database connection is fully released before attempting to delete the directory, or wrap the delete operation in a try-catch to handle potential IOException gracefully.
| if (Directory.Exists(RootPath)) | |
| { | |
| Directory.Delete(RootPath, true); | |
| // SQLite can briefly hold file locks after disposal, especially on Windows. | |
| // Retry deletion a few times to avoid transient failures during cleanup. | |
| const int maxAttempts = 3; | |
| const int delayMilliseconds = 100; | |
| for (var attempt = 0; attempt < maxAttempts; attempt++) | |
| { | |
| if (!Directory.Exists(RootPath)) | |
| { | |
| break; | |
| } | |
| try | |
| { | |
| Directory.Delete(RootPath, true); | |
| break; | |
| } | |
| catch (IOException) when (attempt < maxAttempts - 1) | |
| { | |
| await Task.Delay(delayMilliseconds); | |
| } | |
| catch (UnauthorizedAccessException) when (attempt < maxAttempts - 1) | |
| { | |
| await Task.Delay(delayMilliseconds); | |
| } |
| if (IndexTestContextHolder.Context is not null) | ||
| { | ||
| return IndexTestContextHolder.Context; | ||
| } | ||
|
|
||
| await InitLock.WaitAsync(); | ||
| try | ||
| { | ||
| IndexTestContextHolder.Context ??= await IndexTestContext.CreateAsync(); | ||
| } | ||
| finally | ||
| { | ||
| InitLock.Release(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Condition is always null because of ... is ....
| if (IndexTestContextHolder.Context is not null) | |
| { | |
| return IndexTestContextHolder.Context; | |
| } | |
| await InitLock.WaitAsync(); | |
| try | |
| { | |
| IndexTestContextHolder.Context ??= await IndexTestContext.CreateAsync(); | |
| } | |
| finally | |
| { | |
| InitLock.Release(); | |
| } | |
| if (IndexTestContextHolder.Context is null) | |
| { | |
| await InitLock.WaitAsync(); | |
| try | |
| { | |
| if (IndexTestContextHolder.Context is null) | |
| { | |
| IndexTestContextHolder.Context = await IndexTestContext.CreateAsync(); | |
| } | |
| } | |
| finally | |
| { | |
| InitLock.Release(); | |
| } | |
| } |
Motivation
.NET 10only.TUnitsource-generated testing engine to enable fast, integration-style tests and Native AOT readiness..slnxsolution file.Description
net10.0acrosssrc/andtests/, converted the solution toragsharp.slnx, and removed the old.sln.TUnit(addedTUnitpackage and changed test projectOutputTypetoExe) and added a large integration harnesstests/RagSharp.CodeGraph.Tests/CodeGraphIntegrationTests.csthat generates 100 test cases.CodeGraphIndexerto safely registerMicrosoft.Build.Locator, adaptMSBuildWorkspacecall signatures, add null checks, and use explicitJsonSerializerOptions, and addedMicrosoft.Build.LocatorandMicrosoft.Extensions.Logging.Abstractionspackage references.RagSharp.Packagingpaths tonet10.0, fixedInstaller.GetStatusAsyncto return a compatible files collection, addedAGENTS.mdand adocs/Wiki/ProjectOverview.md, and updatedglobal.jsonto opt into the testing platform for .NET 10.Testing
dotnet run --project tests/RagSharp.CodeGraph.Tests/RagSharp.CodeGraph.Tests.csproj, which completed successfully with103tests passing.dotnet run --project tests/RagSharp.SkillInstaller.Tests/RagSharp.SkillInstaller.Tests.csproj, which completed successfully with2tests passing.dotnet test ragsharp.slnxearlier in the rollout and observed a failing restore step due to an "Ambiguous project name 'RagSharp'" error during a multi-project restore, so CI should be verified in your environment if you rely ondotnet testover running TUnit test executables directly.Codex Task