Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Aug 3, 2025

The MappableResultAssertionBuilder constructors were marked as internal, preventing third-party developers from implementing custom assertions as described in the extensibility documentation.

The documentation shows how to create custom assertions that return data via await by wrapping BaseAssertCondition<TActual> implementations within MappableResultAssertionBuilder<,,> instances:

public static MappableResultAssertionBuilder<IEnumerable<TInner>, EnumerableContainsExpectedFuncAssertCondition<IEnumerable<TInner>, TInner>, TInner> Contains<TInner>(this IValueSource<IEnumerable<TInner>> valueSource, Func<TInner, bool> matcher)
{
    var condition = new EnumerableContainsExpectedFuncAssertCondition<IEnumerable<TInner>, TInner>(matcher, expression);
    
    return new MappableResultAssertionBuilder<IEnumerable<TInner>, EnumerableContainsExpectedFuncAssertCondition<IEnumerable<TInner>, TInner>, TInner>(
        valueSource.RegisterAssertion(condition, [expression]),
        condition,
        (_, assertCondition) => assertCondition.FoundItem
    );
}

However, attempting to use this pattern resulted in compilation errors because the constructors were inaccessible.

Changes

Changed visibility of both MappableResultAssertionBuilder constructors from internal to public:

  • MappableResultAssertionBuilder<TActual, TExpected>(InvokableAssertionBuilder<TActual>, Func<TActual?, TExpected?>)
  • MappableResultAssertionBuilder<TActual, TAssertCondition, TExpected>(InvokableAssertionBuilder<TActual>, TAssertCondition, Func<TActual?, TAssertCondition, TExpected?>)

This enables third-party developers to create custom assertions that return data when awaited, as documented, without breaking any existing functionality.

Fixes #2628.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

@thomhurst thomhurst marked this pull request as ready for review August 3, 2025 21:39
@thomhurst thomhurst enabled auto-merge (squash) August 3, 2025 21:39
Copilot AI changed the title [WIP] MappableResultAssertionBuilder constructors should be public Make MappableResultAssertionBuilder constructors public for third-party extensibility Aug 3, 2025
Copilot AI requested a review from thomhurst August 3, 2025 21:39
@claude
Copy link
Contributor

claude bot commented Aug 4, 2025

Claude finished @Copilot's task —— View job


PR Review Complete ✅

Reviewed this PR that makes MappableResultAssertionBuilder constructors public for third-party extensibility.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Examine the constructor visibility changes
  • Review API change tracking in verification files
  • Analyze security and encapsulation implications
  • Check alignment with documented extensibility patterns
  • Assess test coverage needs
  • Provide comprehensive review feedback

Code Review Feedback

This PR correctly addresses issue #2628 by making the MappableResultAssertionBuilder constructors public to enable third-party extensibility. The change is minimal, well-justified, and properly documented.

Positive Aspects

  1. Well-Defined Problem: The issue is clearly documented - the extensibility documentation at /docs/docs/assertions/extensibility/extensibility-returning-items-from-await.md:81 shows exactly how to use these constructors, but they were inaccessible due to internal visibility.

  2. Minimal Surface Area: Only two constructors changed from internal to public:

    • MappableResultAssertionBuilder<TActual, TExpected>(InvokableAssertionBuilder<TActual>, Func<TActual?, TExpected?>) (Line 10)
    • MappableResultAssertionBuilder<TActual, TAssertCondition, TExpected>(InvokableAssertionBuilder<TActual>, TAssertCondition, Func<TActual?, TAssertCondition, TExpected?>) (Line 36)
  3. Proper API Tracking: All API verification files correctly updated:

    • .NET 8.0, 9.0, and .NET Framework 4.7 API snapshots properly reflect the constructor visibility changes
    • Changes follow the existing TUnit.Assertions API patterns
  4. Consistent with Framework Design: The framework already has a public API surface for extensibility through BaseAssertCondition<TActual> and documented patterns.

🔍 Security & Encapsulation Analysis

  • Low Security Risk: These constructors only accept well-typed parameters and don't expose internal state
  • Controlled Extensibility: Third-party developers still need to provide proper BaseAssertCondition<TActual> implementations and follow the documented patterns
  • Inheritance Safety: The class inherits from InvokableValueAssertionBuilder<TActual> which is already part of the public API

📚 Alignment with Documentation

Perfect alignment with the documented extensibility pattern in TUnit.Assertions/Assertions/Generics/DoesExtensions_Generic.cs:23 where the framework itself uses these constructors in the same way third-party developers would.

🧪 Test Coverage Considerations

  • Existing Framework Tests: The current Contains<TInner>() method in DoesExtensions_Generic.cs:19-29 exercises these constructors
  • Recommendation: Consider adding a dedicated test that validates third-party constructor usage to prevent regression

📈 Performance & Design

  • No Performance Impact: Constructor visibility changes have zero runtime performance impact
  • Clean API Design: Maintains existing patterns and generic constraints, ensuring type safety

Overall Assessment: ✅ APPROVED - This is a clean, minimal change that correctly enables the documented extensibility pattern without introducing security risks or breaking changes.


@claude
Copy link
Contributor

claude bot commented Aug 6, 2025

Claude finished @Copilot's task —— View job


PR Review Complete ✅

Reviewed this PR that makes MappableResultAssertionBuilder constructors public for third-party extensibility.

Todo List:

  • Read repository guidelines (CLAUDE.md) - No CLAUDE.md found
  • Examine the constructor visibility changes
  • Review API change tracking in verification files
  • Analyze security and encapsulation implications
  • Check alignment with documented extensibility patterns
  • Assess test coverage needs
  • Provide comprehensive review feedback

Code Review Feedback

This PR correctly addresses issue #2628 by making the MappableResultAssertionBuilder constructors public to enable third-party extensibility. The change is minimal, well-justified, and properly documented.

Positive Aspects

  1. Well-Defined Problem: The issue is clearly documented - the extensibility documentation at /docs/docs/assertions/extensibility/extensibility-returning-items-from-await.md:81 shows exactly how to use these constructors, but they were inaccessible due to internal visibility.

  2. Minimal Surface Area: Only two constructors changed from internal to public:

    • MappableResultAssertionBuilder<TActual, TExpected>(InvokableAssertionBuilder<TActual>, Func<TActual?, TExpected?>) (Line 10)
    • MappableResultAssertionBuilder<TActual, TAssertCondition, TExpected>(InvokableAssertionBuilder<TActual>, TAssertCondition, Func<TActual?, TAssertCondition, TExpected?>) (Line 36)
  3. Proper API Tracking: All API verification files correctly updated:

    • .NET 8.0, 9.0, and .NET Framework 4.7 API snapshots properly reflect the constructor visibility changes
    • Changes follow the existing TUnit.Assertions API patterns
  4. Consistent with Framework Design: The framework already has a public API surface for extensibility through BaseAssertCondition<TActual> and documented patterns.

🔍 Security & Encapsulation Analysis

  • Low Security Risk: These constructors only accept well-typed parameters and don't expose internal state
  • Controlled Extensibility: Third-party developers still need to provide proper BaseAssertCondition<TActual> implementations and follow the documented patterns
  • Inheritance Safety: The class inherits from InvokableValueAssertionBuilder<TActual> which is already part of the public API

📚 Alignment with Documentation

Perfect alignment with the documented extensibility pattern in TUnit.Assertions/Assertions/Generics/DoesExtensions_Generic.cs:23 where the framework itself uses these constructors in the same way third-party developers would.

🧪 Test Coverage Considerations

  • Existing Framework Tests: The current Contains<TInner>() method in DoesExtensions_Generic.cs:19-29 exercises these constructors
  • Recommendation: Consider adding a dedicated test that validates third-party constructor usage to prevent regression

📈 Performance & Design

  • No Performance Impact: Constructor visibility changes have zero runtime performance impact
  • Clean API Design: Maintains existing patterns and generic constraints, ensuring type safety

Overall Assessment: ✅ APPROVED - This is a clean, minimal change that correctly enables the documented extensibility pattern without introducing security risks or breaking changes.


@thomhurst thomhurst merged commit 00e67b2 into main Aug 6, 2025
8 checks passed
@thomhurst thomhurst deleted the copilot/fix-2628 branch August 6, 2025 19:24
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.

MappableResultAssertionBuilder constructors should be public

2 participants