Skip to content

Conversation

@slang25
Copy link
Contributor

@slang25 slang25 commented Dec 28, 2025

Description

I've had a go at adding TUnit.FsCheck, with some tweaks to the core to better support this extensibility.
I'm opening as a draft for visibility, but will properly fill out the checklist below when it's ready.

Related Issue

Fixes #

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Performance improvement
  • Refactoring (no functional changes)

Checklist

Required

  • I have read the Contributing Guidelines
  • If this is a new feature, I started a discussion first and received agreement
  • My code follows the project's code style (modern C# syntax, proper naming conventions)
  • I have written tests that prove my fix is effective or my feature works

TUnit-Specific Requirements

  • Dual-Mode Implementation: If this change affects test discovery/execution, I have implemented it in BOTH:
    • Source Generator path (TUnit.Core.SourceGenerator)
    • Reflection path (TUnit.Engine)
  • Snapshot Tests: If I changed source generator output or public APIs:
    • I ran TUnit.Core.SourceGenerator.Tests and/or TUnit.PublicAPI tests
    • I reviewed the .received.txt files and accepted them as .verified.txt
    • I committed the updated .verified.txt files
  • Performance: If this change affects hot paths (test discovery, execution, assertions):
    • I minimized allocations and avoided LINQ in hot paths
    • I cached reflection results where appropriate
  • AOT Compatibility: If this change uses reflection:
    • I added appropriate [DynamicallyAccessedMembers] annotations
    • I verified the change works with dotnet publish -p:PublishAot=true

Testing

  • All existing tests pass (dotnet test)
  • I have added tests that cover my changes
  • I have tested both source-generated and reflection modes (if applicable)

Additional Notes

#pragma warning disable IL2046 // RequiresUnreferencedCode attribute mismatch
#pragma warning disable IL3051 // RequiresDynamicCode attribute mismatch
#pragma warning disable IL2072 // DynamicallyAccessedMembers warning
public class FsCheckPropertyTestExecutor : ITestExecutor
Copy link
Owner

Choose a reason for hiding this comment

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

This is pretty cool - I was wondering if someone could give property based testing a go.

Did you give creating a custom data attribute a go at all? Wondering what that'd look like too. Might just fill up the IDE a lot.

@github-actions
Copy link
Contributor

Summary

Adds TUnit.FsCheck library with property-based testing support, extending the test discovery to support custom test attributes inheriting from BaseTestAttribute.

Critical Issues

1. AOT Compatibility - Missing [DynamicallyAccessedMembers] Annotations ⚠️

The FsCheckPropertyTestExecutor.GetMethodInfo method uses reflection without proper AOT annotations (TUnit.FsCheck/FsCheckPropertyTestExecutor.cs:48-84):

private static MethodInfo GetMethodInfo(Type classType, string methodName, ParameterMetadata[] parameters)
{
    var methods = classType
        .GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static)
        .Where(m => m.Name == methodName && m.GetParameters().Length == parameters.Length)
        .ToArray();
    // ...
}

Issue: The classType parameter lacks [DynamicallyAccessedMembers] annotation. Using UnconditionalSuppressMessage (line 43-45) suppresses warnings instead of properly preserving the necessary metadata for AOT.

Fix Required: Add proper annotation:

private static MethodInfo GetMethodInfo(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | 
                                 DynamicallyAccessedMemberTypes.NonPublicMethods)] 
    Type classType, 
    string methodName, 
    ParameterMetadata[] parameters)

Remove the UnconditionalSuppressMessage attributes once proper annotations are in place.

2. Snapshot Tests Not Verified ⚠️

The PR checklist shows snapshot tests are unchecked. Since this PR modifies the source generator (TestMetadataGenerator.cs), you must:

  1. Run TUnit.Core.SourceGenerator.Tests
  2. Review any .received.txt files generated
  3. Accept them as .verified.txt if the changes are intentional
  4. Commit the updated .verified.txt files

Per CLAUDE.md Rule #2: "Changes to source generator output or public APIs require running snapshot tests. Commit .verified.txt files. NEVER commit .received.txt."

Suggestions

Dual-Mode Implementation ✅

Good work! Both paths are correctly updated:

  • Source generator: TUnit.Core.SourceGenerator/Generators/TestMetadataGenerator.cs:39-97 (new GetCustomTestMethodMetadata)
  • Reflection: TUnit.Engine/Discovery/ReflectionTestDataCollector.cs:1028-1041 (new IsTestMethod checks BaseTestAttribute)

The logic properly mirrors between both modes.

Performance Note

The new GetCustomTestMethodMetadata method iterates through all method attributes and checks base types in a loop. This is in the hot path (test discovery). Consider if this could be optimized, though it may be acceptable given discovery is a one-time operation.

Previous Review Status

No previous comments found.

Verdict

⚠️ REQUEST CHANGES - Critical AOT compatibility and snapshot test issues must be addressed before merge.

@slang25
Copy link
Contributor Author

slang25 commented Dec 29, 2025

Example failing property output
image

@github-actions
Copy link
Contributor

Summary

This PR adds a new TUnit.FsCheck library that provides property-based testing support using FsCheck, allowing tests to be run with automatically generated input values.

Critical Issues

1. AOT Compatibility - Missing [DynamicallyAccessedMembers] Annotations ⚠️

Location: TUnit.FsCheck/FsCheckPropertyTestExecutor.cs:44-84

The GetMethodInfo method uses reflection to discover methods without proper AOT annotations:

[UnconditionalSuppressMessage("Trimming", "IL2070", Justification = "FsCheck requires reflection")]
private static MethodInfo GetMethodInfo(Type classType, string methodName, ParameterMetadata[] parameters)
{
    var methods = classType
        .GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Static)
        .Where(m => m.Name == methodName && m.GetParameters().Length == parameters.Length)
        .ToArray();
    // ...
}

Issue: Per CRITICAL RULE #5 in CLAUDE.md: "All code must work with Native AOT. Annotate reflection with [DynamicallyAccessedMembers]."

Using UnconditionalSuppressMessage hides the warning instead of properly preserving metadata for AOT compilation. This will cause runtime failures when published with Native AOT.

Required Fix:

private static MethodInfo GetMethodInfo(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | 
                                 DynamicallyAccessedMemberTypes.NonPublicMethods)] 
    Type classType, 
    string methodName, 
    ParameterMetadata[] parameters)
{
    // Remove UnconditionalSuppressMessage attribute
    // ...
}

The same issue affects other methods that use reflection via MakeGenericMethod (lines 158-209). These also need proper annotations or should be marked with RequiresDynamicCode if they genuinely cannot work with AOT.

2. Blocking on Async in Hot Path 🚫

Location: TUnit.FsCheck/FsCheckPropertyTestExecutor.cs:467-483

private static void HandleVoidResult(object? result)
{
    switch (result)
    {
        case Task task:
            task.GetAwaiter().GetResult();  // ❌ BLOCKING
            break;
        case ValueTask valueTask:
            valueTask.GetAwaiter().GetResult();  // ❌ BLOCKING
            break;
    }
}

private static bool HandleResult(object? result)
{
    switch (result)
    {
        case Task<bool> taskBool:
            return taskBool.GetAwaiter().GetResult();  // ❌ BLOCKING
        // ...
    }
}

Issue: Per CRITICAL RULE in CLAUDE.md: "NEVER block on async - No .Result or .GetAwaiter().GetResult()."

This violates TUnit's async-first architecture and can cause deadlocks in certain synchronization contexts.

Required Fix: The ExecuteTest method signature should be async:

public async ValueTask ExecuteTest(TestContext context, Func<ValueTask> action)
{
    // ... setup code ...
    await RunPropertyCheckAsync(methodInfo, classInstance, config);
}

Suggestions

1. Consider Performance Impact

The FsCheck integration uses reflection heavily in the test execution path. While property-based tests run multiple iterations (default 100), consider if there are opportunities to cache MethodInfo or generic method references to reduce repeated reflection overhead.

2. Error Message Formatting

The FormatCounterexample method (lines 291-340) does good work parsing FsCheck's output. Nice attention to user experience!

3. Snapshot Tests

I don't see any changes to source generator output or public API test surface in this PR. The changes appear to be purely additive (new library). Snapshot test verification may not be required unless there were generator changes not shown in the diff.

Previous Review Status

The previous review (by github-actions) raised the same AOT compatibility concern. It appears unaddressed in the current state of the PR.

Verdict

⚠️ REQUEST CHANGES - Two critical issues must be fixed:

  1. Add proper [DynamicallyAccessedMembers] annotations for AOT compatibility
  2. Remove blocking calls on async code (.GetAwaiter().GetResult())

These violations directly conflict with TUnit's CRITICAL RULES #5 (AOT Compatible) and the principle "NEVER block on async".

@slang25
Copy link
Contributor Author

slang25 commented Dec 29, 2025

I realise that I made this way more complicated that it needed to be 😅 should be a bit simpler now

@github-actions
Copy link
Contributor

Summary

Adds TUnit.FsCheck library for property-based testing integration with FsCheck.

Critical Issues

1. AOT Compatibility Violation - Missing Proper Annotations ⚠️

Location: TUnit.FsCheck/FsCheckPropertyTestExecutor.cs:43-86

The code uses UnconditionalSuppressMessage to hide AOT warnings instead of properly annotating reflection usage:

[UnconditionalSuppressMessage("Trimming", "IL2070", Justification = "FsCheck requires reflection")]
private static MethodInfo GetMethodInfo(
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    Type classType,
    // ...

Issues:

  1. The annotation only includes PublicMethods, but line 53 calls GetMethods with BindingFlags.NonPublic, which requires DynamicallyAccessedMemberTypes.NonPublicMethods
  2. Using UnconditionalSuppressMessage hides the warning instead of fixing the root cause

Per CLAUDE.md Rule #5: "AOT Compatible - All code must work with Native AOT. Annotate reflection with [DynamicallyAccessedMembers]."

Required Fix:

private static MethodInfo GetMethodInfo(
    [DynamicallyAccessedMembers(
        DynamicallyAccessedMemberTypes.PublicMethods | 
        DynamicallyAccessedMemberTypes.NonPublicMethods)] 
    Type classType,
    string methodName,
    ParameterMetadata[] parameters)
{
    // Remove the UnconditionalSuppressMessage attribute
    var methods = classType.GetMethods(
        BindingFlags.Public | BindingFlags.NonPublic | 
        BindingFlags.Instance | BindingFlags.Static)
    // ...
}

2. FsCheck Library Requires Reflection - AOT Compatibility Concern

Location: TUnit.FsCheck/FsCheckPropertyTestExecutor.cs:115-127

[UnconditionalSuppressMessage("Trimming", "IL2060", Justification = "FsCheck requires reflection")]
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "FsCheck requires dynamic code")]
private static void RunPropertyCheck(MethodInfo methodInfo, object classInstance, Config config)
{
    Check.Method(config, methodInfo, classInstance);
}

Issue: FsCheck's Check.Method fundamentally requires reflection and dynamic code generation. This means TUnit.FsCheck cannot work with Native AOT.

Required Action: Since this library cannot be made AOT-compatible due to FsCheck's requirements:

  1. Add [RequiresDynamicCode] and [RequiresUnreferencedCode] attributes to the entire FsCheckPropertyTestExecutor class
  2. Document in the library README/docs that TUnit.FsCheck is not compatible with Native AOT
  3. Consider whether this should block the PR, given TUnit's CRITICAL RULE Or assertion conditions #5

This is an architectural decision for the maintainer: Is it acceptable to ship an optional package that violates the AOT compatibility rule?

Suggestions

Missing Tests

The PR doesn't include any unit tests for the new library. Consider adding:

  • Tests verifying FsCheck integration works correctly
  • Tests for error message formatting (FormatCounterexample, ParseTupleValues)
  • Tests for configuration mapping

Example Project Cleanup

TUnit.Example.FsCheck.TestProject/TUnit.Example.FsCheck.TestProject.sln appears unnecessary - the example project should just be part of the main solution (TUnit.sln already includes it).

Previous Review Status

Previous reviews by github-actions bot raised the same AOT compatibility concerns. The author mentioned simplifying the implementation, but the current diff shows the same code with the same issues.

Verdict

⚠️ REQUEST CHANGES - Critical AOT compatibility violations must be addressed:

  1. Fix the incomplete DynamicallyAccessedMembers annotation in GetMethodInfo
  2. Acknowledge that FsCheck fundamentally cannot work with Native AOT and decide if this is acceptable for an optional TUnit package
  3. If proceeding, properly annotate the class with [RequiresDynamicCode] and [RequiresUnreferencedCode] and document the AOT incompatibility

The maintainer needs to make a policy decision: Should TUnit ship packages that cannot support Native AOT, or is AOT compatibility a hard requirement even for optional extension packages?

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.

2 participants