Skip to content

Move libraries tests to xunit3#126277

Draft
Copilot wants to merge 72 commits intomainfrom
copilot/update-runtime-governance-docs
Draft

Move libraries tests to xunit3#126277
Copilot wants to merge 72 commits intomainfrom
copilot/update-runtime-governance-docs

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 29, 2026

Note

This PR was created with the assistance of GitHub Copilot.

This PR ports the test-only content from #125019 — moving libraries tests from xunit v2 to xunit v3. Production code is intentionally left unchanged.

Description

Key changes include:

  • xunit v3 package updates (eng/Versions.props, eng/testing/xunit/): Updates xunit dependencies from v2 to v3 packages
  • Build infrastructure (Directory.Build.targets, eng/testing/): Adds UseAppHost overrides and build validation for xunit v3 test projects
  • NuGet source (NuGet.config): Adds nuget.org feed for xunit v3 packages
  • Test runner updates (eng/testing/RunnerTemplate.sh, eng/testing/RunnerTemplate.cmd): Updates test execution to use xunit v3 runner
  • Test infrastructure (src/libraries/sendtohelix-browser.targets, src/libraries/sendtohelix-wasi.targets): Updates xunit trait filter syntax (-notrait-trait-) for xunit v3 compatibility
  • SingleFileTestRunner (src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs): Updates for xunit v3 in-process runner APIs with AssemblyMetadata(3, ...) for NativeAOT compatibility
  • NativeAOT single-file targets (eng/testing/tests.singlefile.targets): Disables xunit auto-generated entry point for NativeAOT tests (uses SingleFileTestRunner instead)
  • Test code migrations (~640 test files): Updates test patterns for xunit v3 compatibility including:
    • TheoryData → typed TheoryData<T> with .Data accessor
    • IAsyncLifetime adjustments
    • Assert.ThrowsAsync pattern updates
    • Collection/class fixture changes
    • Skip mechanism updates (throw SkipTestExceptionAssert.SkipUnless/Assert.SkipWhen/Assert.Skip)
  • De Morgan's law skip condition fixes (6 total across 5 files): Fixed inverted skip conditions where compound negated if conditions were incorrectly converted to Assert.SkipUnless/Assert.SkipWhen without properly applying De Morgan's law:
    • HttpClientHandlerTest.Authentication.cs: if (!A || !B) throw SkipAssert.SkipUnless(A && B) (was incorrectly A || !B)
    • HttpClientHandlerTest.Headers.cs: if (!A && B) throw SkipAssert.SkipWhen(!A && B) (was incorrectly Assert.SkipUnless(A && B))
    • TextWriterTests.cs (2 instances): if (!A && !B) throw SkipAssert.SkipUnless(A || B) (was incorrectly A && !B)
    • BoundedChannelTests.cs: if (!A && !B) throw SkipAssert.SkipUnless(A || B) (was incorrectly A && !B)
    • RendezvousChannelTests.cs: if (!A && !B) throw SkipAssert.SkipUnless(A || B) (was incorrectly A && !B)
  • Documentation (docs/workflow/testing/libraries/xunit3-migration.md): Adds migration guide for common xunit v2→v3 patterns

All changes to production/source code (coreclr JIT, nativeaot CoreLib, System.Text.RegularExpressions, native libs, AndroidAppBuilder) have been reverted per reviewer feedback. Only test code, test infrastructure, and documentation changes remain.

See #125019 for full context and discussion.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Port of #125019

Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 29, 2026
The merge from upstream/main reintroduced xunit v2 patterns in files that
were modified on both branches:

- EdgeCaseTests.cs: [ConditionalTheory] -> [Theory] (fixes CS0618 build
  error blocking 21 CI jobs), SkipTestException -> SkipException.ForSkip()
- Regex.Match.Tests.cs: SkipTestException -> SkipException.ForSkip()
- DumpTestBase.cs: SkipTestException -> SkipException.ForSkip()

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@agocke
Copy link
Copy Markdown
Member

agocke commented Mar 29, 2026

@copilot please revert all changes to production code

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 29, 2026

@copilot please revert all changes to production code

Reverted all 19 production code files to their original state in 7167df8. Files reverted:

  • src/coreclr/jit/codegenwasm.cpp, regallocwasm.cpp
  • src/coreclr/nativeaot/System.Private.CoreLib/src/.../ThunkedApis.cs
  • src/libraries/System.Text.RegularExpressions/src/... (3 files)
  • src/native/libs/... (9 files: JavaScript loader, CMakeLists, pal_*.c/.h/.m)
  • src/tasks/AndroidAppBuilder/Templates/monodroid-nativeaot.cs
  • src/libraries/sendtohelix-browser.targets, sendtohelix-wasi.targets

Only test code, test infrastructure (eng/testing/, Directory.Build.targets, NuGet.config, eng/Versions.props), and documentation changes remain.

@agocke
Copy link
Copy Markdown
Member

agocke commented Mar 29, 2026

@copilot these changes are test infra: src/libraries/sendtohelix-browser.targets, sendtohelix-wasi.targets

…-wasi.targets (test infra)

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/257c3fec-6b69-4b38-b147-c12d2df91422

Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 29, 2026

@copilot these changes are test infra: src/libraries/sendtohelix-browser.targets, sendtohelix-wasi.targets

Restored the xunit3 changes (-notrait-trait-) to both files in 4dd3f11.

Comment thread eng/testing/xunit/xunit.targets Outdated
</PropertyGroup>

<!--
Override the xunit.v3.core.mtp-v1 validation target. The original target
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This feels like a situation where you could be using xunit.v3.extensibility.core instead of xunit.v3.core, especially if your custom runner replaces what xunit.v3.runner.inproc.console does. That would make sense in your situation (with a custom runner) and then these rules wouldn't end up being applied. In essence, xunit.v3.core is the package we think of as the package that says "you're planning to run your tests exactly how we intended for .NET Framework and/or .NET", and when you don't fit that mold, stepping away from that package makes sense to me.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(I make this comment without understanding how your custom runner slots into anything. Feel free to ignore me. 😂)

xunit v3 relies on Assembly.Location which returns empty string in
NativeAOT, causing 'Argument was empty (Parameter AssemblyName)'.
xunit v4 (4.0.0-pre.33) fixes this internally.

Changes:
- Override XUnitV3Version to 4.0.0-pre.33 for TestNativeAot builds
- Update SingleFileTestRunner.cs for v4 API changes:
  - Namespace moves (ConsoleHelper, AssemblyMetadata, etc.)
  - Replace removed GetTargetFramework() with TargetFrameworkAttribute
  - Use non-obsolete Run() overload with resultWriters parameter

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

@agocke
Copy link
Copy Markdown
Member

agocke commented Mar 30, 2026

@copilot looks like there are api changes in the new xunit versions for the singlefiletestrunner: these failures are in the arm64 naot run: Running assembly:System.Collections.Tests, Version=11.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
Unhandled exception. System.ArgumentException: xunitVersion must be between 0 and 3 (Parameter 'xunitVersion')
at Xunit.Runner.Common.AssemblyMetadata..ctor(Int32 xunitVersion, String targetFramework) in //src/xunit.v3.runner.common/Utility/AssemblyMetadata.cs:line 42
at SingleFileTestRunner.Main(String[]) in /
/src/libraries/Common/tests/SingleFileTestRunner/SingleFileTestRunner.cs:line 60
at SingleFileTestRunner.Main(String[] args)
--- End of stack trace from previous location ---
at SingleFileTestRunner.

(String[] args)
DOTNET_DbgEnab

…ileTestRunner

The AssemblyMetadata constructor in xunit v3 only accepts xunitVersion
values 0-3. Reverts the v4 pre-release upgrade attempt (2c15bdf) and
restores the working xunit v3 code with AssemblyMetadata(3, ...).

Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b213ec25-ccc3-406a-a952-c9ea640003a5

Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
…ls is empty

On Windows, UninstallableSignals() returns Enumerable.Empty<object[]>().
In xunit v3, a Theory with no data rows fails with 'No data found'
instead of being silently skipped as in v2. Add SkipWhenEmpty = true.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

The NativeAOT attribute stub for ConditionalTheoryAttribute is a plain
Attribute (not inheriting from TheoryAttribute), so it lacks properties
added in xunit v3. Add SkipWhenEmpty so test code using it compiles in
NativeAOT builds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 23, 2026 02:32
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

The property on TheoryAttribute in xunit v3 (4.0.0-pre.81) is
SkipTestWithoutData, not SkipWhenEmpty. Update both the test and
the NativeAOT attribute stub.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

agocke and others added 3 commits April 23, 2026 22:48
ConditionalTheory without conditions is obsolete in xunit v3. The class
already has ConditionalClass gating execution, so plain Theory suffices.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
xunit v3 rejects null values in theory data arguments. Split the
Ctor_Exception theory into two separate Fact tests: one for the null
case and one for the non-null exception with a custom HResult.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ActiveIssueAttribute now extends BeforeAfterTestAttribute instead of
plain Attribute. This allows it to intercept test execution and skip
tests at runtime when conditions match (e.g., IsNativeAot).

Previously, tests with [Fact] + [ActiveIssue(..., IsNativeAot)] were
discovered by the AOT source generator (via [Fact]) but not skipped
(since [ActiveIssue] was a no-op stub), causing failures.

The condition evaluation handles:
- Type + member name conditions (via reflection)
- TestPlatforms flags (via OperatingSystem.IsXxx())
- TargetFrameworkMonikers and TestRuntimes flags

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 24, 2026 01:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

agocke and others added 3 commits April 24, 2026 07:31
xunit v3 changes test execution order, which changes the shared
static Random state. This causes (-100 * s_random.NextDouble()) to
produce -0.0 (when NextDouble() returns 0.0). The test then compares
(-0.0).ToString() ("-0") with BigInteger(0).ToString() ("0"),
which fails since BigInteger has no negative zero concept.

Skip the ToString comparison when the expected value is negative zero.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The xunit v3 AOT source generator instantiates BeforeAfterTestAttribute
subclasses using a parameterless constructor. Without one, NativeAOT
builds fail with CS1729.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ime-governance-docs

# Conflicts:
#	src/libraries/System.Diagnostics.StackTrace/tests/System.Diagnostics.StackTrace.Tests.csproj
#	src/native/managed/cdac/tests/DumpTests/cdac-dump-xplat-test-helix.proj
Copilot AI review requested due to automatic review settings April 28, 2026 04:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

The xUnit v3 AOT source generator always instantiates BeforeAfterTestAttribute
subclasses via parameterless constructors (new T()), discarding all constructor
arguments. This caused ActiveIssueAttribute to unconditionally skip ALL decorated
tests with empty 'Active issue:' messages, regardless of the specified platform/
runtime conditions.

Fix: Before() now reads the original attribute metadata from the test method,
class, and assembly using CustomAttributeData and evaluates the skip conditions
from the actual constructor arguments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 28, 2026 06:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

The browser/wasi smoke test projects were commented out due to
XHarness xunit v2 runner incompatibility with xunit v3 test
assemblies. However, CI shows the non-smoke browser-wasm library
tests (202 work items) run successfully with xunit v3, proving
the compatibility issue was resolved.

Re-enable the browser smoke test projects (Mono and CoreCLR) and
remove the browser/wasi exclusion from System.Runtime.Tests smoke
test. This fixes the 'No helix work items' error in browser-wasm
smoke CI jobs that occurred because zero test projects were selected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126277

Note

This review was generated by GitHub Copilot using Claude Opus 4.6, with additional analysis from Claude Haiku 4.5 and GPT-5.3-Codex.

Holistic Assessment

Motivation: This PR fixes a real, significant bug in the NativeAOT xUnit v3 AOT test infrastructure. The xUnit v3 AOT source generator always instantiates BeforeAfterTestAttribute subclasses via their parameterless constructor, discarding all constructor arguments. The old code stored those arguments in private fields, which were always zero-initialized — meaning every [ActiveIssue]-decorated test was skipped unconditionally regardless of platform/runtime conditions. This is a genuine correctness fix.

Approach: The fix uses CustomAttributeData reflection to read attribute metadata at runtime instead of relying on constructor arguments. This is the correct approach given the xUnit v3 AOT source generator's design constraint. The ConditionalFact fix and browser smoke test re-enablement are logical follow-ups.

Summary: ⚠️ Needs Human Review. The core logic is sound and well-structured, but there are a couple of items that warrant human attention: the fail-open behavior when method resolution fails (could silently miss [ActiveIssue] on overloaded methods), and whether the browser/wasi smoke test re-enablement has been validated in CI.


Detailed Findings

✅ CustomAttributeData Parsing — Correct for all overloads

The ReadActiveIssueData method (AttributeStubs.cs lines 167-219) correctly handles all three ActiveIssueAttribute constructor patterns:

  • Single-arg (string issue): Returns default Any/Any/Both — correct.
  • Type + params string[]: Properly detects args[1].ArgumentType == typeof(Type) and handles both array and non-array params representation via SelectMany.
  • Enum-based overloads: Dynamically checks ArgumentType against each enum type — correctly handles all combinations. The (TestPlatforms)(int)args[i].Value! cast is correct because CustomAttributeTypedArgument stores enum values as their underlying int type.

Verified by all three models (Opus 4.6, Haiku 4.5, GPT-5.3-Codex).

✅ ConditionalFact Fix — Correct

The change from [ConditionalFact(nameof(IsNotStrictKeyValidatingPlatform))] to [ConditionalFact(typeof(X25519DiffieHellmanBaseTests), nameof(IsNotStrictKeyValidatingPlatform))] is correct. The AOT stub's ConditionalFactAttribute only has (Type, params string[]) and (params Type[]) overloads — no single-string overload. IsNotStrictKeyValidatingPlatform is a static property on X25519DiffieHellmanBaseTests, so the type reference is accurate.

✅ Metadata Hierarchy Traversal — Thorough

GetActiveIssueMetadata (lines 106-134) correctly reads attributes from method → class → base class → assembly, which matches how the real ActiveIssueAttribute is designed to work at all three AttributeTargets. The base class check via methodInfo.DeclaringType handles inherited test methods correctly.

⚠️ ResolveMethod Fail-Open on Overloaded Methods — Advisory

ResolveMethod (lines 155-164) uses GetMethod(name, BindingFlags) which throws AmbiguousMatchException when multiple methods share the same name (common with [Theory] overloads). The bare catch { return null; } causes the entire method-level [ActiveIssue] check to be skipped — the test will run instead of skip. This is the correct fail-open direction (better to run a test that should be skipped than to skip a test that should run), but it means [ActiveIssue] on overloaded methods may not be honored in NativeAOT builds.

The code comment on line 155 says "Use GetMethods to handle overloads gracefully" but actually uses GetMethod (singular). Consider updating the comment to accurately describe the catch-based approach.

Flagged by all three models.

⚠️ Base-Class Chain Traversal — Incomplete but low risk

GetActiveIssueMetadata reads from testClass.Class and methodInfo.DeclaringType but does not walk the full inheritance chain. If an [ActiveIssue] is applied to a grandparent class, it would be missed. In practice, test class hierarchies in dotnet/runtime are typically shallow (1-2 levels), so this is very low risk. The real ActiveIssueAttribute from the Extensions package also operates via xUnit's attribute discovery which handles this differently, so exact parity isn't necessary for this stub.

Flagged by GPT-5.3-Codex.

⚠️ Browser/WASI Smoke Test Re-enablement — Needs CI validation

The PR removes browser and wasi from the System.Runtime.Tests smoke test exclusion and uncomments 8 browser smoke test projects. The original exclusion comment cited "wasm gets ReflectionAssemblyInfo constructor error" — if that xunit v2/v3 incompatibility is resolved, re-enabling is correct. A human reviewer should confirm that browser smoke test CI legs are green. If browser CI is not exercised in this PR's pipeline, these could introduce failures on merge. Apple platform exclusions correctly remain in place.

✅ NativeAOT Trimming Safety

CustomAttributeData is preserved by the NativeAOT toolchain (reads from metadata, not instantiated objects). The DynamicallyAccessedMembers annotations on ConditionType are correctly propagated to the ActiveIssueData record struct. If metadata is trimmed, ReadActiveIssueData returns no results, causing the test to run (fail-open) — this is the safe default.

✅ ActiveIssueData Record Struct — Clean design

Using a readonly record struct for the parsed data is appropriate — value semantics, no heap allocation, works well with yield return.

Generated by Code Review for issue #126277 ·

agocke and others added 2 commits April 28, 2026 18:07
The xUnit v3 AOT source generator discovers [Fact]/[Theory] methods inside
classes decorated with [ConditionalClass], but ConditionalClassAttribute is
a no-op stub in NativeAOT mode. This causes COM interop tests to run on
NativeAOT where they crash with SIGSEGV.

Use the xUnit v3 built-in SkipUnless property on each [Fact]/[Theory] in
ThrowExceptionForHRTests, GetExceptionForHRTests, and GetExceptionCodeTests
so these tests are properly skipped at runtime when running on NativeAOT.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ConditionalClass is a no-op stub in NativeAOT mode, causing tests that
should be skipped to run and crash (SIGSEGV on arm64). Replace all ~214
usages with Assert.SkipUnless calls in constructors, which works correctly
in both regular and NativeAOT test execution.

For static test classes (e.g. ParallelQueryCombinationTests), inject
Assert.SkipUnless at the start of each test method since static classes
have no instance constructor.

Also removes ConditionalClassAttribute from AttributeStubs.cs entirely.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 28, 2026 19:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Meta linkable-framework Issues associated with delivering a linker friendly framework

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants