Update to .NET 10, bump NuGet packages#63
Conversation
All package bumps from running `dotnet package update`.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdate solution target to net10.0, bump centralized NuGet package versions (including EF Core → 10.0.7), add/update linq2db packages and logging import, convert multiple test lifecycle methods from Task to ValueTask, and make tests cancellation-aware. Changes.NET 10 Infrastructure and Test Adjustments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@CodeRabbit - Running the tests with |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the exact versions in use and look up compatibility information.
Root CauseA This is a well-known class of errors in the The latest stable release of What to CheckLook at what version of If the project has been bumped to a prerelease version of Suggested FixesOption 1 (Recommended): Pin Version 10.3.0 is the latest stable <PackageVersion Include="linq2db" Version="6.2.1" />
<PackageVersion Include="linq2db.EntityFrameworkCore" Version="10.3.0" />Option 2: Upgrade If you specifically need a newer version of Then update Option 3: Diagnose the exact mismatch Run the following to see exactly which version of This will confirm whether there is a transitive version conflict overriding the pinned SummaryThe most likely cause is that 🏁 Script executed: Length of output: 211 🏁 Script executed: Length of output: 90 🏁 Script executed: Length of output: 45 Let me look up the exact NuGet dependencies for
Root Cause: Deprecated
|
|
@CodeRabbit Next question. Once I get past the linq2db errors, I get the following: "SIL.Harmony.Tests.DbContextTests.VerifyModel (124ms): Error Message: System.MethodAccessException : Attempt by method 'VerifyXunit.Verifier.Verify(VerifyTests.VerifySettings, System.String, System.Func |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 202
Root Cause: Mismatched Core
|
| Package | Version | Core Verify it was compiled against |
|---|---|---|
Verify.Xunit |
31.12.5 | Verify >= 31.12.5 |
Verify.EntityFramework |
15.1.0 | Verify ~15.x (much older) |
Verify.DiffPlex |
3.1.2 | Verify ~3.x (very old) |
NuGet resolves to the highest minimum, so core Verify 31.x gets loaded at runtime. However, Verify.EntityFramework 15.1.0 and Verify.DiffPlex 3.1.2 were compiled against much older core Verify assemblies, where the internal API surface (including the [InternalsVisibleTo] declarations and Guards class structure) was different. This causes the MethodAccessException when Verify.Xunit 31.x tries to call into internals it expects to be accessible in Verify 31.x, but isn't (because the actual loaded assembly's internal contracts differ from what was compiled in).
There is an additional important note: Verify.Xunit 31.12.5 itself is marked as deprecated on NuGet — "legacy and is no longer maintained." This means simply updating Verify.Xunit to its latest version won't give you a stable long-term solution.
Fix
dotnet package update updates each package greedily and independently, without understanding cross-package compatibility constraints within ecosystems like VerifyTests. All Verify.* packages must be pinned to compatible versions — ones that all depend on the same range of the core Verify package.
Step 1: Find aligned versions
Run this to check what version of core Verify each extension package transitively pulls in:
dotnet list package --include-transitive | grep -i "^.*Verify"Step 2: Align all Verify packages to compatible versions
You need to update Verify.EntityFramework and Verify.DiffPlex to versions that also depend on Verify >= 31.x. For example (check NuGet for the exact latest compatible versions):
<!-- In Directory.Packages.props - align all Verify.* to compatible versions -->
<PackageVersion Include="Verify.DiffPlex" Version="X.Y.Z" /> <!-- bump to version depending on Verify >= 31.x -->
<PackageVersion Include="Verify.EntityFramework" Version="X.Y.Z" /> <!-- bump to version depending on Verify >= 31.x -->
<PackageVersion Include="Verify.Xunit" Version="31.12.5" />To find the right versions, check each package on NuGet and look at its Dependencies tab to confirm it lists Verify (>= 31.x).
Step 3 (optional but recommended): Migrate away from deprecated Verify.Xunit
Since Verify.Xunit 31.12.5 is deprecated, you should check the VerifyTests GitHub (https://github.com/VerifyTests/Verify) for the recommended migration path — the project may have moved to a new package name or integration model for xunit v3 support.
Summary
| Issue | Cause | Fix |
|---|---|---|
MethodAccessException on Guards.AgainstBadSourceFile |
Verify.EntityFramework 15.1.0 / Verify.DiffPlex 3.1.2 are compiled against an old Verify core, while Verify.Xunit 31.12.5 requires Verify >= 31.12.5 |
Update all Verify.* packages to versions that consistently depend on Verify >= 31.x |
The bottom line: in the VerifyTests ecosystem, all Verify.* extension packages must be at versions that declare a compatible range for the same core Verify version. dotnet package update doesn't enforce this constraint.
|
@CodeRabbit Suggest changes to the Harmony source code to port to Xunit V3. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather information about the current test setup and xUnit v3 migration requirements. 🏁 Script executed: Length of output: 1870 🏁 Script executed: Length of output: 3486 🌐 Web query: 💡 Result: Official xUnit.net v2 → v3 migration guide (Apr 12, 2025) is here: xunit.net docs “Migrating Unit Tests from v2 to v3”. It recommends this order: (1) understand architectural changes, (2) update NuGet packages, (3) convert project to an executable, (4) update target frameworks. [1] Key breaking/required changes when migrating v2 → v31) Update NuGet package references (package renames)Most packages moved from 2) Convert the test project from library to executablev3 test projects are stand-alone executables. Update your 3) Update target frameworks (new minimums)xUnit.net v3 minimum runtime requirements include .NET Framework 4.7.2+ and .NET 8+; update your project TFMs accordingly. [1] 4) Fix common code issues
Helpful companion docs
Sources: [1] (xunit.net) [2] (xunit.net) [3] (xunit.net) Citations:
Let me look at the actual test files and check the Verify v3 package situation. 🏁 Script executed: Length of output: 15696 🌐 Web query: 💡 Result: For xUnit.net v3, the replacement for Install: dotnet add package Verify.XunitV3
(Separately, xUnit v3 itself uses the Sources: [1] [2] [3]
xUnit v3 Migration1.
|
| File | Change |
|---|---|
Directory.Packages.props |
xunit → xunit.v3; Verify.Xunit → Verify.XunitV3 |
SIL.Harmony.Tests.csproj |
Add <OutputType>Exe</OutputType>; swap package refs |
DataModelTestBase.cs |
IAsyncLifetime methods: Task → ValueTask |
MultiThreadingTests.cs |
Remove using Xunit.Abstractions; |
DataModelPerformanceTests.cs |
Remove using Xunit.Abstractions; |
GlobalUsings.cs |
No change needed |
|
@CodeRabbit - |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2337
Option 1 (Recommended):
|
|
@CodeRabbit - Next question. When running |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the package version and compatibility with xUnit v3. 🏁 Script executed: Length of output: 2242 🌐 Web query: 💡 Result: What “SelfRegisteredExtensions” means (and why it breaks)In the Microsoft.Testing.Platform (MTP) world, extensions/reporters are typically auto-registered by generated entry-point code. If that generated code can’t find the auto-registration type (often surfaced as a compile error mentioning GitHubActionsTestLogger + xUnit.net v3: what’s compatible
The big gotcha:
|
|
Only performance tests are failing, and they pass for me when I run them locally. I wonder if we should even run performance tests in GHA; it's a very artificial environment. Perhaps performance tests should be confined to running on actual computers rather than resource-constrained VMs. At any rate, I think we should go ahead and merge even though the SimpleAddChangePerformanceTest is failing in GHA, because it does pass for me. @myieye, if it passes on your computer as well, want to approve this? |
|
@CodeRabbit fullreview |
|
✅ Actions performedFull review triggered. |
|
@CodeRabbit reviews remaining? |
|
Review rate limit: 0/1 reviews remaining, refill in 10 minutes and 37 seconds. |
|
@CodeRabbit rate limit |
|
Review rate limit: 1/1 review remaining, available now. |
|
@CodeRabbit fullreview |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/SIL.Harmony.Tests/MultiThreadingTests.cs (1)
19-19: ⚡ Quick winPrefer
GetAwaiter().GetResult()overAsTask().Wait()for ValueTask.While
.AsTask().Wait()works, using.GetAwaiter().GetResult()is more efficient (avoids Task allocation) and provides better exception propagation forValueTask.⚡ Proposed improvement
- fixture.InitializeAsync().AsTask().Wait(); + fixture.InitializeAsync().GetAwaiter().GetResult();🤖 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 `@src/SIL.Harmony.Tests/MultiThreadingTests.cs` at line 19, Replace the current synchronous wait on the ValueTask returned by fixture.InitializeAsync() to avoid Task allocation and improve exception handling: locate the call expression fixture.InitializeAsync().AsTask().Wait() in MultiThreadingTests.cs and change it to use the ValueTask waiter (call GetAwaiter().GetResult() on the ValueTask returned by InitializeAsync) so the method blocks without creating a Task.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Directory.Packages.props`:
- Line 17: Update the PackageVersion entry for Microsoft.NET.Test.Sdk to a valid
released version: locate the XML element with PackageVersion
Include="Microsoft.NET.Test.Sdk" Version="18.5.1" and change the Version
attribute to a valid release (e.g., "18.4.0") so the project uses an existing
NuGet package version.
---
Nitpick comments:
In `@src/SIL.Harmony.Tests/MultiThreadingTests.cs`:
- Line 19: Replace the current synchronous wait on the ValueTask returned by
fixture.InitializeAsync() to avoid Task allocation and improve exception
handling: locate the call expression fixture.InitializeAsync().AsTask().Wait()
in MultiThreadingTests.cs and change it to use the ValueTask waiter (call
GetAwaiter().GetResult() on the ValueTask returned by InitializeAsync) so the
method blocks without creating a Task.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78785239-7cab-4d95-9cb0-70631425315f
📒 Files selected for processing (13)
Directory.Packages.propssrc/Directory.Build.propssrc/SIL.Harmony.Linq2db/Linq2dbKernel.cssrc/SIL.Harmony.Linq2db/SIL.Harmony.Linq2db.csprojsrc/SIL.Harmony.Tests/DataModelPerformanceTests.cssrc/SIL.Harmony.Tests/DataModelReferenceTests.cssrc/SIL.Harmony.Tests/DataModelTestBase.cssrc/SIL.Harmony.Tests/DataQueryTests.cssrc/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txtsrc/SIL.Harmony.Tests/MultiThreadingTests.cssrc/SIL.Harmony.Tests/RepositoryTests.cssrc/SIL.Harmony.Tests/SIL.Harmony.Tests.csprojsrc/SIL.Harmony.Tests/SyncTests.cs
💤 Files with no reviewable changes (1)
- src/SIL.Harmony.Tests/DataModelPerformanceTests.cs
ValueTask.GetAwaiter().GetResult() doesn't allocate a Task.
|
FINALLY got the performance tests to pass by just rerunning the failed build over and over until it landed on a GHA runner that was fast enough. That's not a great experience, and I still would prefer to configure the tests so GHA doesn't run benchmark tests, as its performance today was just too unpredictable. But hey, once @myieye reviews this, we can merge it with no qualms now. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/SIL.Harmony.Tests/ModelSnapshotTests.cs (1)
84-87: ⚡ Quick winMake
ClearNonRootSnapshotscancellation-aware for consistency.
ClearNonRootSnapshotsstill uses parameterlessExecuteDeleteAsync(), while the rest of this file now passesTestContext.Current.CancellationToken. Passing the token here keeps teardown/rebuild paths consistently cancelable.Suggested diff
private Task ClearNonRootSnapshots() { - return DbContext.Snapshots.Where(s => !s.IsRoot).ExecuteDeleteAsync(); + return DbContext.Snapshots + .Where(s => !s.IsRoot) + .ExecuteDeleteAsync(TestContext.Current.CancellationToken); }🤖 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 `@src/SIL.Harmony.Tests/ModelSnapshotTests.cs` around lines 84 - 87, The ClearNonRootSnapshots method currently calls ExecuteDeleteAsync() without a token; update it to accept/call ExecuteDeleteAsync(TestContext.Current.CancellationToken) so the delete is cancellation-aware like other methods in this file—locate the ClearNonRootSnapshots method and modify the ExecuteDeleteAsync invocation on DbContext.Snapshots to pass TestContext.Current.CancellationToken.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/SIL.Harmony.Tests/ModelSnapshotTests.cs`:
- Around line 84-87: The ClearNonRootSnapshots method currently calls
ExecuteDeleteAsync() without a token; update it to accept/call
ExecuteDeleteAsync(TestContext.Current.CancellationToken) so the delete is
cancellation-aware like other methods in this file—locate the
ClearNonRootSnapshots method and modify the ExecuteDeleteAsync invocation on
DbContext.Snapshots to pass TestContext.Current.CancellationToken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7322f0-b833-44a4-96c8-9b3cfeb42271
📒 Files selected for processing (15)
src/SIL.Harmony.Tests/Adapter/CustomObjectAdapterTests.cssrc/SIL.Harmony.Tests/DataModelReferenceTests.cssrc/SIL.Harmony.Tests/DataModelSimpleChanges.cssrc/SIL.Harmony.Tests/DataQueryTests.cssrc/SIL.Harmony.Tests/DbContextTests.cssrc/SIL.Harmony.Tests/DefinitionTests.cssrc/SIL.Harmony.Tests/DeleteAndCreateTests.cssrc/SIL.Harmony.Tests/ModelSnapshotTests.cssrc/SIL.Harmony.Tests/MultiThreadingTests.cssrc/SIL.Harmony.Tests/PersistExtraDataTests.cssrc/SIL.Harmony.Tests/RepositoryTests.cssrc/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cssrc/SIL.Harmony.Tests/ResourceTests/WordResourceTests.cssrc/SIL.Harmony.Tests/SnapshotTests.cssrc/SIL.Harmony.Tests/SyncTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/SIL.Harmony.Tests/SyncTests.cs
Fixes #62.
All package bumps come from running
dotnet package updateon each .csproj file.Summary by CodeRabbit
Chores
Tests