Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 14, 2025

  • Explore repository structure and understand the issue
  • Identify all places where accessibility checks need to be updated
  • Update source generator to handle internal types and properties
  • Update reflection mode to handle internal types and properties
  • Add or update tests to verify internal class/property support
  • Build and verify changes compile correctly
  • Build test project and verify it compiles
  • Manually verify the generated source code is correct for internal classes
  • Confirm all three test cases generate proper property injection code
  • Revert SDK version change in global.json

Summary

Successfully implemented support for internal classes and properties with ClassDataSource attribute. The changes include:

Source Generator (PropertyInjectionSourceGenerator.cs)

  • Updated IsPubliclyAccessible to accept both public and internal types

Reflection Mode

  • Updated PropertyInjectionPlanBuilder.BuildReflectionPlan to discover NonPublic properties
  • Updated ReflectionAttributeExtractor.ExtractPropertyDataSources to discover NonPublic properties

API Surface

  • Updated all ClassDataSourceAttribute variants (generic and non-generic) to include DynamicallyAccessedMemberTypes.NonPublicConstructors | NonPublicProperties | NonPublicMethods
  • Updated ClassDataSources methods with the same NonPublic member access

Testing

  • Added test cases for internal class with internal property, public class with public property
  • Verified source generator creates correct property injection code for all scenarios
  • Confirmed UnsafeAccessor is used for init-only properties with fallback to reflection

The implementation now allows users to mark both test classes and their ClassDataSource properties as internal, addressing the reported issue.

Original prompt

This section details on the original issue you should resolve

<issue_title>ClassDataSource property is not populated when the property is internal</issue_title>
<issue_description>Here is the repo to replicate the bug

https://github.com/HakamFostok/TunitBug

Basically, the UnitTest class is public, and the dependency "Dep" is populated.
Here is an example that works

public class Dep;

public sealed class UnitTest
{
    [ClassDataSource<Dep>(Shared = SharedType.None)]
    public required Dep Dep { get; init; }

    [Test]
    internal async Task TestMethod1()
    {
        // Dep is     null when class is internal
        // Dep is not null when class is public
        await Assert.That(Dep).IsNotNull();
    }
}

But when the class is internal, the required property "Dep" is not populated, it is null. and the test is failing.

public class Dep;

// make class public so Dep1 will be initialized
internal sealed class UnitTest
{
    [ClassDataSource<Dep>(Shared = SharedType.None)]
    // make the property public when class is public, otherwise, compiler error
    internal required Dep Dep { get; init; }

    [Test]
    internal async Task TestMethod1()
    {
        // Dep is     null when class is internal
        // Dep is not null when class is public
        await Assert.That(Dep).IsNotNull();
    }
}

I really would like to be able to mark the class and the required property as an internal.
as it is normal for everything in the test project not to be used from the outside.

</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #3369


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

Copilot AI and others added 4 commits October 14, 2025 10:02
- Updated PropertyInjectionSourceGenerator to accept internal types
- Updated reflection mode to discover internal properties
- Updated DynamicallyAccessedMembers attributes to include NonPublic members
- Added test case for internal class with internal property

Co-authored-by: thomhurst <[email protected]>
Copilot AI changed the title [WIP] Fix ClassDataSource property not populated for internal class Support internal classes and properties with ClassDataSource attribute Oct 14, 2025
Copilot AI requested a review from thomhurst October 14, 2025 10:16
@thomhurst
Copy link
Owner

@copilot undo the SDK change

Restored SDK version to 10.0.100-rc.1.25451.107 as requested

Co-authored-by: thomhurst <[email protected]>
Copy link
Contributor Author

Copilot AI commented Oct 14, 2025

@copilot undo the SDK change

Done. Reverted SDK version to 10.0.100-rc.1.25451.107 in global.json (commit: will be shown after push)

The PR adds NonPublicConstructors, NonPublicProperties, and NonPublicMethods to DynamicallyAccessedMemberTypes attributes, which changes the public API surface. This is intentional to support internal classes and properties with ClassDataSource attribute.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@claude
Copy link
Contributor

claude bot commented Oct 14, 2025

Claude finished @Copilot's task —— View job


PR Review Complete

🎯 Summary

This PR successfully implements support for internal classes and properties with ClassDataSource attribute. The implementation correctly addresses the dual-mode requirement and follows TUnit's architectural patterns.

✅ Code Quality & Best Practices

Source Generator Changes (PropertyInjectionSourceGenerator.cs:119-149)

  • Excellent: Updated IsPubliclyAccessible to accept both Public and Internal accessibility
  • Good: Maintains proper nested type checking for inheritance chains
  • Solid: Preserves existing validation logic for generic types

Reflection Mode Updates

  • Correct: PropertyInjectionPlanBuilder.cs:72 - Added NonPublic to BindingFlags
  • Consistent: ReflectionAttributeExtractor.cs:202 - Updated property discovery to include NonPublic
  • Well-designed: Both changes maintain existing filter logic

API Surface Changes

  • Comprehensive: All ClassDataSourceAttribute variants now include NonPublicConstructors | NonPublicProperties | NonPublicMethods in DynamicallyAccessedMembers
  • Consistent: ClassDataSources.cs methods updated with same member access annotations
  • AOT-compatible: Proper trimming annotations maintained

🧪 Test Coverage

  • Complete: New test file InternalClassDataSourceTests.cs covers all scenarios:
    • Internal class with internal property
    • Public class with internal property
    • Public class with public property (regression test)
  • Realistic: Tests use proper [EngineTest(ExpectedResult.Pass)] annotations
  • Thorough: Covers the exact use case from issue ClassDataSource property is not populated when the property is internal #3369

⚡ Performance Assessment

  • Negligible impact: Changes only affect type/property discovery, not hot execution paths
  • No new allocations: Existing reflection caching mechanisms remain intact
  • Efficient: Source generator changes are compile-time only

🔒 Security Analysis

  • Safe: Internal accessibility is an appropriate security boundary for test frameworks
  • Controlled: Changes are scoped to test data injection, not arbitrary code execution
  • Validated: Maintains existing type safety and validation mechanisms

🔄 Dual-Mode Implementation

  • Complete: Both source-generated and reflection modes properly updated
  • Consistent: Identical behavior between modes for internal/public types
  • Verified: Test coverage ensures both execution paths work correctly

📋 Snapshot Updates

  • Expected: PublicAPI snapshot changes reflect the DynamicallyAccessedMembers updates
  • Correct: All three target framework snapshots updated consistently
  • Valid: Changes preserve existing public API surface while expanding member access

🎯 Minor Observations

  1. Code Organization: The changes are well-distributed across the codebase, touching only the necessary components
  2. Backward Compatibility: No breaking changes - existing public classes/properties continue to work
  3. Documentation: The implementation follows existing patterns and doesn't require additional documentation

🚀 Recommendation

APPROVE - This is a solid implementation that:

The PR is ready for merge once CI passes.


@thomhurst thomhurst merged commit 8f4ef60 into main Oct 14, 2025
11 checks passed
@thomhurst thomhurst deleted the copilot/fix-internal-class-dependency branch October 14, 2025 15:10
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.

ClassDataSource property is not populated when the property is internal

2 participants