Skip to content

Update to .NET 10, bump NuGet packages#63

Merged
rmunn merged 7 commits into
mainfrom
chore/dotnet-10
May 14, 2026
Merged

Update to .NET 10, bump NuGet packages#63
rmunn merged 7 commits into
mainfrom
chore/dotnet-10

Conversation

@rmunn
Copy link
Copy Markdown
Contributor

@rmunn rmunn commented May 12, 2026

Fixes #62.

All package bumps come from running dotnet package update on each .csproj file.

Summary by CodeRabbit

  • Chores

    • Upgraded project target to .NET 10.0
    • Updated centralized NuGet versions: EF Core to 10.x, linq2db (added extensions), and various runtime/test dependencies; added Microsoft.Testing.Platform
  • Tests

    • Migrated testing stack to xUnit/Verify v3 and updated related test packages
    • Made test lifecycle methods return ValueTask for lighter async overhead
    • Made async test queries and file I/O cancellation-aware by passing the test cancellation token

Review Change Stack

All package bumps from running `dotnet package update`.
@rmunn rmunn self-assigned this May 12, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Update 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

Layer / File(s) Summary
Target framework upgrade
src/Directory.Build.props
The default target framework is changed from net8.0 to net10.0.
Centralized NuGet package version bumps
Directory.Packages.props
Central PackageVersion entries updated: EF Core packages → 10.0.7; multiple runtime, test, and tooling packages bumped; Verify/XUnit entries moved to V3 variants; Microsoft.Testing.Platform added.
Linq2DB wiring and project references
src/SIL.Harmony.Linq2db/Linq2dbKernel.cs, src/SIL.Harmony.Linq2db/SIL.Harmony.Linq2db.csproj, src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj
Swap LinqToDB.AspNet.LoggingLinqToDB.Extensions.Logging; add linq2db.Extensions package to library and test project; update test project Verify/XUnit package references to V3 variants and adjust GitHubActionsTestLogger metadata.
Tests: ValueTask lifecycles and cancellation-aware calls
src/SIL.Harmony.Tests/*, src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt
Multiple test classes' InitializeAsync/DisposeAsync changed from Task to ValueTask; many EF/Linq2DB async calls and file IO now pass TestContext.Current.CancellationToken; minor using and fixture-wait adjustments; EF model snapshot annotations and ProductVersion refreshed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • sillsdev/harmony#42: Also modifies Directory.Packages.props EF Core package entries (previous bump to 9.0.4).

Suggested reviewers

  • myieye
  • hahn-kev

Poem

🐰 A tiny hop to version ten,
Packages bumped and logs made new,
ValueTasks skip, tests run again,
linq2db joins the crew,
Harmony hums — a tidy view.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main changes: upgrading to .NET 10 and bumping NuGet packages, which aligns with the core objectives of the PR.
Linked Issues check ✅ Passed The PR successfully addresses issue #62 by updating the target framework to .NET 10 and bumping all NuGet packages to compatible versions, including EF Core 10.0.7, xUnit v3, and linq2db 6.x ecosystem packages.
Out of Scope Changes check ✅ Passed All changes are within scope of the .NET 10 upgrade objective. Updates to ValueTask signatures, cancellation token handling, and package migration from linq2db.AspNet to linq2db.Extensions are necessary adaptations for the upgrade.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/dotnet-10

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 12, 2026

@CodeRabbit - Running the tests with dotnet test fails; every single test throws the exception "System.TypeLoadException : Could not load type 'LinqToDB.Common.OptionsContainer`1' from assembly 'linq2db, Version=6.2.1.0, Culture=neutral, PublicKeyToken=e41013125f9e410a'." Please explain this error and suggest ways to fix it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me check the exact versions in use and look up compatibility information.

@rmunn, this is a classic assembly version mismatch between the linq2db and linq2db.EntityFrameworkCore packages. Here's what's happening and how to fix it:


Root Cause

A System.TypeLoadException of this form — Could not load type '...' from assembly 'linq2db, Version=6.2.1.0' — means that linq2db.EntityFrameworkCore was compiled against a version of linq2db that contains LinqToDB.Common.OptionsContainer<T>, but the version of linq2db actually resolved at runtime (6.2.1) does not contain that type.

This is a well-known class of errors in the linq2db ecosystem. Similar exceptions (e.g. "Could not load type 'LinqToDB.Expressions.Extensions' from assembly 'linq2db, Version=4.2.0.0'") occur when the two packages get out of sync.

The latest stable release of linq2db is 6.2.1, and the roadmap wiki already lists a Release 6.3.0 as pending. The OptionsContainer<T> type referenced in your error was most likely introduced in linq2db 6.3.0 (unreleased stable) and the version of linq2db.EntityFrameworkCore that was bumped in this PR was compiled against it.


What to Check

Look at what version of linq2db.EntityFrameworkCore is currently set in Directory.Packages.props. The latest stable linq2db.EntityFrameworkCore 10.3.0 on NuGet declares a dependency of linq2db (>= 6.2.1) and Microsoft.EntityFrameworkCore.Relational (>= 10.0.0).

If the project has been bumped to a prerelease version of linq2db.EntityFrameworkCore (e.g. one targeting linq2db 6.3.0-preview), that would explain the missing type.


Suggested Fixes

Option 1 (Recommended): Pin linq2db.EntityFrameworkCore to 10.3.0

Version 10.3.0 is the latest stable linq2db.EntityFrameworkCore and it pairs with linq2db >= 6.2.1. Update Directory.Packages.props to ensure both packages are on their latest stable versions:

<PackageVersion Include="linq2db" Version="6.2.1" />
<PackageVersion Include="linq2db.EntityFrameworkCore" Version="10.3.0" />

Option 2: Upgrade linq2db to a prerelease that has OptionsContainer<T>

If you specifically need a newer version of linq2db.EntityFrameworkCore that requires 6.3.0-era APIs, you would need to also bump linq2db to the corresponding prerelease. Run:

Then update Directory.Packages.props accordingly. However, note that prereleases may introduce other instabilities.

Option 3: Diagnose the exact mismatch

Run the following to see exactly which version of linq2db.EntityFrameworkCore is being resolved and what version of linq2db it transitively pulls:

This will confirm whether there is a transitive version conflict overriding the pinned linq2db 6.2.1.


Summary

The most likely cause is that dotnet package update bumped linq2db.EntityFrameworkCore to a prerelease version that depends on OptionsContainer<T> not yet present in the stable linq2db 6.2.1. The safest fix is to pin linq2db.EntityFrameworkCore back to 10.3.0 (the current stable), which is confirmed to be compatible with linq2db 6.2.1.


🏁 Script executed:

#!/bin/bash
# Read the Directory.Packages.props to see linq2db versions
cat Directory.Packages.props | grep -i linq

Length of output: 211


🏁 Script executed:

dotnet add package linq2db --prerelease
dotnet add package linq2db.EntityFrameworkCore --prerelease

Length of output: 90


🏁 Script executed:

dotnet list package --include-transitive | grep -i linq

Length of output: 45


Let me look up the exact NuGet dependencies for linq2db.EntityFrameworkCore 10.3.0.

@rmunn, now that we have the full picture, I can identify the actual root cause precisely.


Root Cause: Deprecated linq2db.AspNet package

The Directory.Packages.props has two linq2db-related packages:

  • linq2db.EntityFrameworkCore 10.3.0 depends on linq2db (>= 6.2.1) and Microsoft.EntityFrameworkCore.Relational (>= 10.0.0).
  • linq2db.AspNet 5.4.1.9 — a 5.x-era package compiled against linq2db 5.x.

linq2db.AspNet has been deprecated as it is legacy and no longer maintained, with linq2db.Extensions as the suggested alternative.

The critical migration note from the linq2db 6 upgrade guide explains exactly what happened: "linq2db.AspNet nuget was renamed to linq2db.Extensions to better reflect its purpose as it contains extensions for integration with Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Logging, not limited by ASP.NET applications only. The namespace was also renamed to LinqToDB.Extensions.DependencyInjection and LinqToDB.Extensions.Logging."

Additionally, linq2db 6.0 introduced a breaking namespace reorganization: general-use public APIs are now in LinqToDB.* and internal APIs in LinqToDB.Internal.* namespaces. If you cannot find some types/methods, check their new location in source.

When linq2db.EntityFrameworkCore 10.3.0 pulls in linq2db 6.2.1, the old linq2db.AspNet 5.4.1.9 package (compiled against linq2db 5.x) tries to load LinqToDB.Common.OptionsContainer<T> — a type that was reorganized away from LinqToDB.Common in linq2db 6.x — causing the TypeLoadException.


Fix

Step 1: In Directory.Packages.props, replace the deprecated linq2db.AspNet with the official replacement linq2db.Extensions:

<!-- Remove this: -->
<PackageVersion Include="linq2db.AspNet" Version="5.4.1.9" />

<!-- Add this: -->
<PackageVersion Include="linq2db.Extensions" Version="6.2.1" />

Step 2: In your .csproj files, replace <PackageReference Include="linq2db.AspNet" .../> with <PackageReference Include="linq2db.Extensions" />.

Step 3: Update any using directives in your code that reference the old namespace. Per the migration guide, LinqToDB.AspNet is replaced by LinqToDB.Extensions.DependencyInjection and LinqToDB.Extensions.Logging. Specifically, AddLinqToDBContext<T>() is still available but now lives under the new namespace.

For example:

- using LinqToDB.AspNet;
- using LinqToDB.AspNet.Logging;
+ using LinqToDB.Extensions.DependencyInjection;
+ using LinqToDB.Extensions.Logging;

Step 4: Also explicitly pin linq2db in Directory.Packages.props to avoid future transitive resolution surprises:

<PackageVersion Include="linq2db" Version="6.2.1" />

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 12, 2026

@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.Func2<VerifyTests.InnerVerifier,System.Threading.Tasks.Task1<VerifyTests.VerifyResult>>, Boolean)' to access method 'Guards.AgainstBadSourceFile(System.String)' failed." Explain the error and suggest fixes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the Verify-related package versions in Directory.Packages.props
grep -i "Verify\|verify" Directory.Packages.props

Length of output: 202


@rmunn, this is another assembly compatibility mismatch, this time within the Verify snapshot testing ecosystem. Here's the full explanation:


Root Cause: Mismatched Core Verify Dependency

A System.MethodAccessException of this form — Attempt by method 'VerifyXunit.Verifier.Verify(...)' to access method 'Guards.AgainstBadSourceFile(System.String)' failed — is a cross-assembly internal method access failure.

Guards.AgainstBadSourceFile is an internal method in the core Verify assembly. Verify.Xunit 31.12.5 declares a dependency on Verify (>= 31.12.5), and it calls Guards.AgainstBadSourceFile via [InternalsVisibleTo] friend assembly access — something that was set up between Verify and Verify.Xunit when they were compiled together at version 31.x.

The problem is that the three Verify-family packages in your Directory.Packages.props depend on vastly different versions of the core Verify package:

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.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 12, 2026

@CodeRabbit Suggest changes to the Harmony source code to port to Xunit V3.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me gather information about the current test setup and xUnit v3 migration requirements.


🏁 Script executed:

#!/bin/bash
# Find test project files
fd -e csproj | xargs grep -l -i "xunit\|test" 2>/dev/null

echo "---PACKAGES---"
cat Directory.Packages.props | grep -i "xunit\|Verify\|test\|coverlet"

echo "---TEST FILES---"
fd -e cs -g "*Test*" | head -30
fd -e cs --search-path . | xargs grep -l "using Xunit" 2>/dev/null | head -20

Length of output: 1870


🏁 Script executed:

#!/bin/bash
# Look at the test .csproj files for package references
fd -e csproj | xargs grep -l -i "test\|xunit" 2>/dev/null | while read f; do
  echo "=== $f ==="
  cat "$f"
done

Length of output: 3486


🌐 Web query:

xunit v3 migration guide from v2 2025

💡 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 → v3

1) Update NuGet package references (package renames)

Most packages moved from xunit.* to xunit.v3.*. Common mappings include: xunitxunit.v3, xunit.assertxunit.v3.assert, xunit.corexunit.v3.core, xunit.runner.consolexunit.v3.runner.console; xunit.abstractions is removed (no longer required); xunit.runner.visualstudio should be 3.x.y. [1]

2) Convert the test project from library to executable

v3 test projects are stand-alone executables. Update your .csproj to set:
<OutputType>Exe</OutputType> (v2 was typically Library). [1]

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

  • async void tests are no longer supported (they fail fast); change to Task/ValueTask. [1]
  • Disposal behavior changed: when both sync/async dispose exist, v3 calls DisposeAsync but not Dispose (can be breaking if you relied on both). [1]

Helpful companion docs

  • “What’s New in v3” (includes v3 project templates via dotnet new install xunit.v3.templates, then dotnet new xunit3). [2]
  • If you maintain extensions/runners: “Migrating Extensibility from v2 to v3”. [3]

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:

#!/bin/bash
echo "=== GlobalUsings.cs ==="
cat src/SIL.Harmony.Tests/GlobalUsings.cs

echo "=== DataModelTestBase.cs ==="
cat src/SIL.Harmony.Tests/DataModelTestBase.cs

echo "=== DbContextTests.cs ==="
cat src/SIL.Harmony.Tests/DbContextTests.cs

echo "=== MultiThreadingTests.cs ==="
cat src/SIL.Harmony.Tests/MultiThreadingTests.cs

echo "=== CommitTests.cs (first 60 lines) ==="
head -60 src/SIL.Harmony.Tests/CommitTests.cs

echo "=== DataModelPerformanceTests.cs (first 40 lines) ==="
head -40 src/SIL.Harmony.Tests/DataModelPerformanceTests.cs

Length of output: 15696


🌐 Web query:

Verify.Xunit v3 xunit replacement package 2025

💡 Result:

For xUnit.net v3, the replacement for Verify.Xunit is Verify.XunitV3. Verify.Xunit is now marked deprecated/legacy on NuGet and explicitly lists Verify.XunitV3 as the suggested alternative. [1]

Install:

dotnet add package Verify.XunitV3

Verify.XunitV3 targets xUnit v3 via xunit.v3.extensibility.core (so it’s the correct integration package for v3). [2]

(Separately, xUnit v3 itself uses the xunit.v3.* package naming scheme, e.g., xunit.v3 for writing tests.) [3]

Sources: [1] [2] [3]


@rmunn Here is a complete guide to porting the Harmony test project to xUnit v3.


xUnit v3 Migration

1. Directory.Packages.props — swap package versions

The official migration guide recommends updating NuGet packages as the first step. Most packages moved from xunit.* to xunit.v3.*; notably xunitxunit.v3, and xunit.runner.visualstudio should remain at 3.x.y. Verify.Xunit is deprecated and Verify.XunitV3 is its replacement for xUnit v3.

-    <PackageVersion Include="Verify.Xunit" Version="31.12.5" />
+    <PackageVersion Include="Verify.XunitV3" Version="31.12.5" />
-    <PackageVersion Include="xunit" Version="2.9.3" />
+    <PackageVersion Include="xunit.v3" Version="0.6.0" />

xunit.runner.visualstudio (already at 3.1.5) and coverlet.collector require no changes.


2. src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj — three changes

xUnit v3 test projects are stand-alone executables; set <OutputType>Exe</OutputType>, and the minimum .NET version is .NET 8+.

  <PropertyGroup>
      <IsPackable>false</IsPackable>
      <IsTestProject>true</IsTestProject>
      <RootNamespace>SIL.Harmony.Tests</RootNamespace>
+     <OutputType>Exe</OutputType>
  </PropertyGroup>

  <ItemGroup>
      ...
-     <PackageReference Include="Verify.Xunit" />
+     <PackageReference Include="Verify.XunitV3" />
-     <PackageReference Include="xunit" />
+     <PackageReference Include="xunit.v3" />
      ...
  </ItemGroup>

3. src/SIL.Harmony.Tests/DataModelTestBase.csIAsyncLifetime signature change

In xUnit v3 the IAsyncLifetime interface uses ValueTask instead of Task:

-public virtual Task InitializeAsync()
-{
-    return Task.CompletedTask;
-}
+public virtual ValueTask InitializeAsync()
+{
+    return ValueTask.CompletedTask;
+}

-public async Task DisposeAsync()
+public async ValueTask DisposeAsync()
 {
     await _services.DisposeAsync();
 }

Any subclass that overrides InitializeAsync() will also need its return type changed to ValueTask.


4. src/SIL.Harmony.Tests/MultiThreadingTests.cs and DataModelPerformanceTests.cs — remove Xunit.Abstractions

In xUnit v3 ITestOutputHelper was moved into the main Xunit namespace, so Xunit.Abstractions is no longer needed:

-using Xunit.Abstractions;

The ITestOutputHelper output constructor parameters themselves are unchanged and continue to work as-is.


5. No async void tests to fix

xUnit v3 no longer supports async void tests; they fail fast. A quick scan of the test files shows all async tests already return Task or ValueTask, so no changes are needed here.


Summary table

File Change
Directory.Packages.props xunitxunit.v3; Verify.XunitVerify.XunitV3
SIL.Harmony.Tests.csproj Add <OutputType>Exe</OutputType>; swap package refs
DataModelTestBase.cs IAsyncLifetime methods: TaskValueTask
MultiThreadingTests.cs Remove using Xunit.Abstractions;
DataModelPerformanceTests.cs Remove using Xunit.Abstractions;
GlobalUsings.cs No change needed

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 12, 2026

@CodeRabbit - src/SIL.Harmony.Tests/MultiThreadingTests.cs contains a call to fixture.InitializeAsync().Wait() which has become an error now that InitializeAsync() returns a ValueTask. Since this call is inside a var t = new Thread(() => { ... } block, it is not as simple as just adding the await keyword. Suggest a way to fix this error.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat src/SIL.Harmony.Tests/MultiThreadingTests.cs

Length of output: 2337


@rmunn ValueTask doesn't have a .Wait() method, but there are two clean ways to fix this synchronous blocking call inside a Thread body:


Option 1 (Recommended): GetAwaiter().GetResult()

This works directly on ValueTask without allocating a Task, and importantly it propagates exceptions directly (without wrapping them in AggregateException like .Wait() does):

- fixture.InitializeAsync().Wait();
+ fixture.InitializeAsync().GetAwaiter().GetResult();

This is the idiomatic pattern for synchronously blocking on either a Task or a ValueTask in a context where await isn't available (such as a Thread body).


Option 2: .AsTask().Wait()

Converts the ValueTask to a Task first, then blocks. This is a one-word change but does allocate an extra Task object:

- fixture.InitializeAsync().Wait();
+ fixture.InitializeAsync().AsTask().Wait();

Recommendation

Use Option 1 (GetAwaiter().GetResult()). It's the preferred pattern for synchronous blocking on ValueTask, it avoids the extra allocation from .AsTask(), and it gives cleaner exception propagation. Your existing .Result call on fixture.WriteNextChange(...) on the next line follows the same spirit — GetAwaiter().GetResult() is the ValueTask equivalent.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 12, 2026

@CodeRabbit - Next question. When running dotnet test, the error message "src/artifacts/obj/SIL.Harmony.Tests/debug/SelfRegisteredExtensions.cs(16,17): error CS0400: The type or namespace name 'GitHubActionsTestLogger' could not be found in the global namespace (are you missing an assembly reference?)" appears. Explain the error and how to fix it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me check the package version and compatibility with xUnit v3.


🏁 Script executed:

#!/bin/bash
echo "=== Directory.Packages.props (test logger / xunit entries) ==="
grep -i "GitHubActions\|xunit\|testlogger" Directory.Packages.props

echo "=== SIL.Harmony.Tests.csproj ==="
cat src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj

Length of output: 2242


🌐 Web query:

GitHubActionsTestLogger xunit v3 compatibility SelfRegisteredExtensions

💡 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 SelfRegisteredExtensions), it usually means you disabled entry-point generation (for example via <GenerateTestingPlatformEntryPoint>false</GenerateTestingPlatformEntryPoint>) or otherwise prevented MTP’s MSBuild integration from generating/including the auto-registration glue. (learn.microsoft.com)

GitHubActionsTestLogger + xUnit.net v3: what’s compatible

  • GitHubActionsTestLogger 3.x is explicitly an extension for both VSTest and Microsoft.Testing.Platform. (nuget.org)
  • xUnit.net v3 supports Microsoft.Testing.Platform natively, and you can choose MTP v1 vs v2 via xunit.v3.mtp-v1 / xunit.v3.mtp-v2 (or leave the default). (xunit.net)

The big gotcha: dotnet test --logger ... vs MTP

If you’re running xUnit v3 in MTP mode, VSTest-style options like --logger are ignored, so dotnet test --logger GitHubActions won’t activate the logger. (learn.microsoft.com)
In MTP there is no single centralized --logger switch; each reporter/extension contributes its own command-line option(s) (discoverable via dotnet test -?). (learn.microsoft.com)

Practical guidance

  • If you want the old behavior (dotnet test --logger GitHubActions), run under VSTest (keep the VSTest path/packages; don’t switch to MTP-only). (nuget.org)
  • If you want MTP, reference GitHubActionsTestLogger 3.x, ensure you didn’t disable MTP entry-point generation, and invoke the reporter’s MTP option (not --logger; check dotnet test -?). (nuget.org)

Citations:


@rmunn, this error is caused by a subtle interaction between xUnit v3's Microsoft.Testing.Platform (MTP) integration and how GitHubActionsTestLogger is referenced in the .csproj.


Root Cause: compile assets excluded, but MTP source generator needs them

xUnit v3 (3.2.2) natively supports Microsoft.Testing.Platform. When running in MTP mode, xUnit v3's source generator auto-produces SelfRegisteredExtensions.cs, which directly references any registered MTP extensions — including GitHubActionsTestLogger 3.x — using their fully-qualified type names (e.g. global::GitHubActionsTestLogger).

For this generated code to compile, the compiler must have a reference to the GitHubActionsTestLogger assembly (i.e. its compile-time assets must be available).

Looking at the current .csproj:

<PackageReference Include="GitHubActionsTestLogger">
  <PrivateAssets>all</PrivateAssets>
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>

Notice that compile is absent from IncludeAssets. This pattern was perfectly fine for the old VSTest-based logger (which plugged in via --logger GitHubActions at runtime with no compile-time reference needed), but it breaks MTP auto-registration because the generated SelfRegisteredExtensions.cs tries to reference a type the compiler can't see.


Fix

Add compile back to IncludeAssets:

 <PackageReference Include="GitHubActionsTestLogger">
   <PrivateAssets>all</PrivateAssets>
-  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
+  <IncludeAssets>compile; runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
 </PackageReference>

Or, more simply, drop the <IncludeAssets> line entirely (the default includes all assets). Keeping <PrivateAssets>all</PrivateAssets> is still correct — it prevents GitHubActionsTestLogger from flowing to consumers of the test project:

 <PackageReference Include="GitHubActionsTestLogger">
   <PrivateAssets>all</PrivateAssets>
-  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
 </PackageReference>

Either form tells the compiler to resolve global::GitHubActionsTestLogger from the package DLL, which unblocks the generated SelfRegisteredExtensions.cs from compiling.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

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?

@rmunn rmunn requested a review from myieye May 13, 2026 06:09
@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

@CodeRabbit fullreview

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

@rmunn, I'll kick off a full review of the PR now!

✅ Actions performed

Full review triggered.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

@CodeRabbit reviews remaining?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Review rate limit: 0/1 reviews remaining, refill in 10 minutes and 37 seconds.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

@CodeRabbit rate limit

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Review rate limit: 1/1 review remaining, available now.

@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

@CodeRabbit fullreview

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

@rmunn, kicking off a full review of the PR now!

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/SIL.Harmony.Tests/MultiThreadingTests.cs (1)

19-19: ⚡ Quick win

Prefer GetAwaiter().GetResult() over AsTask().Wait() for ValueTask.

While .AsTask().Wait() works, using .GetAwaiter().GetResult() is more efficient (avoids Task allocation) and provides better exception propagation for ValueTask.

⚡ 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

📥 Commits

Reviewing files that changed from the base of the PR and between f16f71b and 743e92c.

📒 Files selected for processing (13)
  • Directory.Packages.props
  • src/Directory.Build.props
  • src/SIL.Harmony.Linq2db/Linq2dbKernel.cs
  • src/SIL.Harmony.Linq2db/SIL.Harmony.Linq2db.csproj
  • src/SIL.Harmony.Tests/DataModelPerformanceTests.cs
  • src/SIL.Harmony.Tests/DataModelReferenceTests.cs
  • src/SIL.Harmony.Tests/DataModelTestBase.cs
  • src/SIL.Harmony.Tests/DataQueryTests.cs
  • src/SIL.Harmony.Tests/DbContextTests.VerifyModel.verified.txt
  • src/SIL.Harmony.Tests/MultiThreadingTests.cs
  • src/SIL.Harmony.Tests/RepositoryTests.cs
  • src/SIL.Harmony.Tests/SIL.Harmony.Tests.csproj
  • src/SIL.Harmony.Tests/SyncTests.cs
💤 Files with no reviewable changes (1)
  • src/SIL.Harmony.Tests/DataModelPerformanceTests.cs

Comment thread Directory.Packages.props
ValueTask.GetAwaiter().GetResult() doesn't allocate a Task.
@rmunn
Copy link
Copy Markdown
Contributor Author

rmunn commented May 13, 2026

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/SIL.Harmony.Tests/ModelSnapshotTests.cs (1)

84-87: ⚡ Quick win

Make ClearNonRootSnapshots cancellation-aware for consistency.

ClearNonRootSnapshots still uses parameterless ExecuteDeleteAsync(), while the rest of this file now passes TestContext.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

📥 Commits

Reviewing files that changed from the base of the PR and between 743e92c and 8c46b45.

📒 Files selected for processing (15)
  • src/SIL.Harmony.Tests/Adapter/CustomObjectAdapterTests.cs
  • src/SIL.Harmony.Tests/DataModelReferenceTests.cs
  • src/SIL.Harmony.Tests/DataModelSimpleChanges.cs
  • src/SIL.Harmony.Tests/DataQueryTests.cs
  • src/SIL.Harmony.Tests/DbContextTests.cs
  • src/SIL.Harmony.Tests/DefinitionTests.cs
  • src/SIL.Harmony.Tests/DeleteAndCreateTests.cs
  • src/SIL.Harmony.Tests/ModelSnapshotTests.cs
  • src/SIL.Harmony.Tests/MultiThreadingTests.cs
  • src/SIL.Harmony.Tests/PersistExtraDataTests.cs
  • src/SIL.Harmony.Tests/RepositoryTests.cs
  • src/SIL.Harmony.Tests/ResourceTests/RemoteResourcesTests.cs
  • src/SIL.Harmony.Tests/ResourceTests/WordResourceTests.cs
  • src/SIL.Harmony.Tests/SnapshotTests.cs
  • src/SIL.Harmony.Tests/SyncTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/SIL.Harmony.Tests/SyncTests.cs

@rmunn rmunn merged commit fab2754 into main May 14, 2026
3 of 5 checks passed
@rmunn rmunn deleted the chore/dotnet-10 branch May 14, 2026 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update target and nuget references to .net 10

2 participants