Skip to content

Conversation

@thomhurst
Copy link
Owner

Summary

API

// Simplest - generic with new() constraint (no factory needed)
public static T GetOrCreate<T>(SharedType sharedType, DataGeneratorMetadata metadata, string? key) where T : new()
public static T GetOrCreate<T>(SharedType sharedType, Type? testClassType, string? key) where T : new()

// Generic with custom factory
public static T GetOrCreate<T>(SharedType sharedType, DataGeneratorMetadata metadata, string? key, Func<T> factory)
public static T GetOrCreate<T>(SharedType sharedType, Type? testClassType, string? key, Func<T> factory)

// Non-generic (runtime types, factory required)
public static object? GetOrCreate(SharedType sharedType, Type type, DataGeneratorMetadata metadata, string? key, Func<object?> factory)
public static object? GetOrCreate(SharedType sharedType, Type type, Type? testClassType, string? key, Func<object?> factory)

Usage Example

public class MyCustomDataSourceAttribute<T> : DataSourceGeneratorAttribute<T> where T : new()
{
    public SharedType Shared { get; set; } = SharedType.None;
    public string Key { get; set; } = "";

    protected override IEnumerable<Func<T>> GenerateDataSources(DataGeneratorMetadata metadata)
    {
        yield return () => SharedDataSources.GetOrCreate<T>(Shared, metadata, Key);
    }
}

Test plan

  • Verify TUnit.Core builds successfully
  • Verify API is accessible from test projects
  • Test custom data source implementation using the new helper

🤖 Generated with Claude Code

Add a public SharedDataSources class that allows users to implement custom
data source attributes with the same sharing behavior (SharedType and Key)
as ClassDataSourceAttribute.

This addresses the request in discussion #4536 for a way to extend or reuse
the functionality from ClassDataSources without exposing internal implementation
details.

The API provides multiple overloads:
- Generic with new() constraint for simple default construction
- Generic with factory for custom initialization
- Non-generic for runtime type scenarios

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Replace ArgumentNullException.ThrowIfNull with null-coalescing throw pattern
- Add null-forgiving operators to TestDataContainer return values
- Add null-forgiving operators to key parameter after null check

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

Summary

Adds a new public SharedDataSources helper class for custom data source attributes with shared instance management.

Critical Issues

1. Missing AOT Compatibility Annotations

The new public API is missing [DynamicallyAccessedMembers] annotations required for Native AOT compatibility (TUnit rule #5). This violates the mandatory rules in CLAUDE.md.

Problem: Non-generic overloads accept Type parameters without AOT annotations:

  • Line 128: GetOrCreate(SharedType sharedType, Type type, Type? testClassType, ...)
  • Line 108: GetOrCreate(SharedType sharedType, Type type, DataGeneratorMetadata dataGeneratorMetadata, ...)

Required Fix: Add [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)] to all Type parameters that will be instantiated.

Reference: See TUnit.Core/Attributes/TestData/ClassDataSourceAttribute.cs:18-67 and TUnit.Core/Attributes/TestData/ClassDataSources.cs:21,42,55,69,74 for the exact pattern.

2. Missing Public API Snapshots

This PR adds a new public class to TUnit.Core but does not update the .verified.txt snapshot files (TUnit rule #2).

Required Fix: Run the public API snapshot tests and commit the updated .verified.txt files in TUnit.PublicAPI/ directory.

NEVER commit .received.txt files - only .verified.txt.

Suggestions

Lines 146, 155, 166, 175, 186, 195 use hardcoded strings in ArgumentNullException. Consider using nameof() in the message for better maintainability.

Verdict

REQUEST CHANGES - Critical AOT compatibility and snapshot test issues must be fixed before merge.

Add SharedDataSources class to the public API verified files for
DotNet8_0, DotNet9_0, and DotNet10_0.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

Summary

Adds a new public SharedDataSources helper class that allows users to implement custom data source attributes with shared instance management.

Critical Issues

Missing AOT Compatibility Annotations

The non-generic GetOrCreate overloads are missing [DynamicallyAccessedMembers] annotations on their Type parameters, which violates TUnit rule #5 (AOT Compatible).

Problem locations:

  • Line 108: GetOrCreate(SharedType sharedType, Type type, DataGeneratorMetadata dataGeneratorMetadata, ...)
  • Line 128: GetOrCreate(SharedType sharedType, Type type, Type? testClassType, ...)

Required fix: Add the annotation to all Type parameters that will be instantiated:

public static object? GetOrCreate(
    SharedType sharedType,
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | 
                                 DynamicallyAccessedMemberTypes.NonPublicConstructors | 
                                 DynamicallyAccessedMemberTypes.PublicProperties | 
                                 DynamicallyAccessedMemberTypes.NonPublicProperties)]
    Type type,
    DataGeneratorMetadata dataGeneratorMetadata,
    string? key,
    Func<object?> factory)

Reference patterns:

  • TUnit.Core/Attributes/TestData/ClassDataSourceAttribute.cs:18-67
  • TUnit.Core/Attributes/TestData/ClassDataSources.cs:55,74

This is critical for Native AOT compatibility per CLAUDE.md mandatory rules.

Previous Review Status

The public API snapshot issue has been resolved ✅ - all three .verified.txt files have been properly updated. The AOT annotation issue from the previous review remains unaddressed.

Verdict

⚠️ REQUEST CHANGES - Critical AOT compatibility issue must be fixed before merge.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

Summary

Adds a public SharedDataSources helper class to allow custom data source attributes with the same sharing behavior (SharedType and Key) as ClassDataSourceAttribute.

Critical Issues

1. Missing AOT Annotations ❌

Rule #5 Violation: AOT Compatible

The non-generic GetOrCreate overloads use reflection internally (via TestDataContainerActivator.CreateInstance) but lack required [DynamicallyAccessedMembers] annotations on the Type type parameter.

Location: TUnit.Core/SharedDataSources.cs:107, 128

Required fix: Add annotations matching the internal implementation in ClassDataSources.cs:74:

public static object? GetOrCreate(
    SharedType sharedType, 
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
    Type type, 
    DataGeneratorMetadata dataGeneratorMetadata, 
    string? key, 
    Func<object?> factory)

public static object? GetOrCreate(
    SharedType sharedType, 
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
    Type type, 
    Type? testClassType, 
    string? key, 
    Func<object?> factory)

2. Potential NullReferenceException ⚠️

Location: TUnit.Core/SharedDataSources.cs:130

Line 130 has:

_ = type ?? throw new ArgumentNullException(nameof(type));

But the parameter signature at line 128 declares Type type as non-nullable. This null check will never execute in null-safe contexts. Either:

  • Make the parameter nullable: Type? type (matches the generic overloads pattern)
  • Remove the null check (if callers are guaranteed to pass non-null)

Same issue at line 141.

Suggestions

Minor: XML Documentation

The XML docs for the non-generic overloads mention the factory parameter creates instances "if not already cached", but when SharedType.None is used, the factory is always called. Consider clarifying this edge case in the docs.

Verdict

⚠️ REQUEST CHANGES - Critical AOT annotation missing (Rule #5 violation)

Address PR review feedback:
- Add [DynamicallyAccessedMembers] attribute to Type parameters in
  non-generic GetOrCreate overloads for Native AOT compatibility
- Update public API snapshots with the new signatures

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

Summary

Adds a new public SharedDataSources helper class that allows users to implement custom data source attributes with the same sharing behavior (SharedType and Key) as ClassDataSourceAttribute.

Critical Issues

None found ✅

Previous Review Status

All previous issues have been addressed:

  1. AOT annotations fixed - The latest commit (d612991) added [DynamicallyAccessedMembers(PublicConstructors | NonPublicConstructors)] annotations to both non-generic GetOrCreate overloads (lines 111 and 137), matching the pattern in ClassDataSources.cs:74
  2. Public API snapshots updated - All four .verified.txt files have been properly updated to include the new SharedDataSources API

Code Review

The implementation looks solid:

  • Clean API design: Provides both generic (with new() constraint) and factory-based overloads, plus non-generic versions for runtime type scenarios
  • Consistent with existing patterns: Mirrors the internal ClassDataSources implementation while exposing a clean public API
  • Proper error handling: Validates required parameters (testClassType for PerClass/PerAssembly, key for Keyed) with clear error messages
  • AOT compatible: All Type parameters that flow to Activator.CreateInstance have proper [DynamicallyAccessedMembers] annotations
  • .NET Standard 2.0 compatible: Uses null-coalescing throw pattern instead of ArgumentNullException.ThrowIfNull
  • Modern C# practices: Uses static lambdas in lines 26 and 44 for zero-allocation factory delegates

No TUnit rule violations detected:

Verdict

APPROVE - All critical issues resolved, implementation follows TUnit best practices

The PublicApiGenerator doesn't render DynamicallyAccessedMembers attributes
in netstandard2.0/net472 target frameworks.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst
Copy link
Owner Author

Summary

This PR adds a new public SharedDataSources helper class that allows users to implement custom data source attributes with the same sharing behavior as ClassDataSourceAttribute.

Critical Issues

None found ✅

Suggestions

1. Consider adding a test to verify the API works as expected

The PR description mentions testing the API, but I do not see any new test files in the diff. Consider adding a test that demonstrates a custom data source attribute using the new SharedDataSources API and verification that the sharing behavior works correctly.

Example test location: TUnit.TestProject/CustomSharedDataSourceTests.cs

2. Documentation opportunity

This is a great extensibility point. Consider adding documentation in docs/docs/customization-extensibility/ showing how users can leverage this new API to create custom data source attributes with sharing behavior.

Analysis

What This PR Does Well:

  • Public API snapshots correctly updated (all .verified.txt files)
  • Proper AOT annotations with [DynamicallyAccessedMembers] on all reflection-based overloads
  • Clean abstraction that exposes the right level of functionality without leaking internal details (TestDataContainer remains internal)
  • Consistent error handling and validation (null checks, proper exceptions)
  • Good API design with both generic and non-generic overloads
  • Static lambda factories minimize allocations - follows TUnit performance-first principle

TUnit Rules Compliance:

  • AOT Compatible (Rule 5): All reflection properly annotated
  • Performance First (Rule 4): Uses static lambdas to avoid allocations
  • Public API Changes (Rule 2): Snapshots updated correctly
  • Modern C# features used throughout

No concerns regarding:

  • Dual-mode implementation (not applicable - this is just a helper API)
  • VSTest usage (none present)
  • Performance issues (well optimized with static lambdas and proper caching via TestDataContainer)

Verdict

✅ APPROVE - No critical issues. The suggestions are optional enhancements for discoverability and verification.

This enables cleaner API calls:
- SharedDataSources.GetOrCreate<T>(SharedType.PerTestSession)
- SharedDataSources.GetOrCreate<T>(SharedType.Keyed, key: "mykey")

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@thomhurst thomhurst enabled auto-merge (squash) January 27, 2026 00:03
@thomhurst thomhurst disabled auto-merge January 27, 2026 00:10
@thomhurst thomhurst merged commit bfb27e3 into main Jan 27, 2026
13 checks passed
@thomhurst thomhurst deleted the feat/shared-data-sources-helper branch January 27, 2026 00:10
This was referenced Jan 27, 2026
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