Skip to content

Conversation

@marcpiechura
Copy link
Contributor

As discussed, I've used the new taskAssert expression in all of my tests and it's working as expected, so I think it's good to go.

closes #3971

@claude
Copy link
Contributor

claude bot commented Dec 4, 2025

Code Review: F# taskAssert Computation Expression

Thank you for this contribution! This is a clean and idiomatic F# enhancement that significantly improves the assertion experience for F# users. Here's my detailed review:


✅ Strengths

1. Excellent F# Idioms

  • The computation expression approach is idiomatic and aligns with common F# patterns (taskResult, taskOption, etc.)
  • Reusing the built-in task builder via let taskAssert = task is elegant and avoids unnecessary code duplication
  • The Bind extension for IAssertion is minimal and focused

2. Clean Implementation

  • TaskAssert.fs:1-15: Very concise implementation (~15 lines) that leverages existing F# infrastructure
  • Proper use of AutoOpen modules ensures smooth developer experience
  • The #nowarn "FS1204" suppression is appropriate for the type extension pattern

3. Good Documentation

  • docs/docs/assertions/fsharp.md:24-42: The documentation update clearly explains the new feature with practical examples
  • Shows side-by-side comparison with the check approach

4. Comprehensive Test Coverage

  • TaskAssertTests.fs:12-55: Tests cover 1-5 ClassDataSource scenarios, which mirrors the existing ClassDataSourceDrivenTests.fs pattern
  • All tests follow the same structure, making them easy to understand and maintain

🔍 Observations & Minor Suggestions

1. Test Duplication
The new TaskAssertTests.fs duplicates test scenarios from ClassDataSourceDrivenTests.fs (lines 26-67). While this demonstrates the feature works, consider:

  • Option A (Current): Keep as-is to provide explicit coverage for the new CE
  • Option B (Optimization): Add 1-2 simpler tests in TaskAssertTests.fs and convert existing tests in ClassDataSourceDrivenTests.fs to use taskAssert to reduce duplication

I recommend Option A for this PR since:

  • It provides clear regression coverage
  • Existing tests remain stable
  • Future refactoring can consolidate if desired

2. Performance Considerations
The implementation should have minimal overhead since:

  • taskAssert = task is just an alias (zero overhead)
  • The Bind extension delegates to the standard task builder
  • IAssertion.AssertAsync() returns a Task, which is efficiently handled

No performance concerns identified

3. Type Signature Clarity
TaskAssert.fs:13: The Bind signature is correct:

member inline x.Bind(assertion: IAssertion, continuation: Unit -> TaskCode<'TOverall, 'TResult2>)

The Unit parameter in continuation is standard for assertions that don't return values. This is correct.


🛡️ Security & Safety

No security concerns

  • No dynamic code generation
  • No reflection usage
  • No external dependencies
  • Type-safe F# code

📋 Checklist (Based on CLAUDE.md)

Let me verify against the project's requirements:

  • Dual-Mode Implementation: N/A - This is an F#-specific syntactic enhancement that works with both source-gen and reflection modes transparently
  • Snapshot Testing: N/A - No source generator or public API changes (pure F# library addition)
  • No VSTest: ✅ Correctly uses TUnit's own testing infrastructure
  • Performance First: ✅ Zero-overhead implementation (just type extensions)
  • AOT/Trimming: ✅ No reflection or dynamic code generation
  • Modern C# Syntax: N/A - This is F# code, follows F# best practices
  • Test Coverage: ✅ Comprehensive tests covering multiple scenarios

🎯 Final Assessment

Recommendation: ✅ APPROVE with minor suggestion

This PR is well-implemented and ready to merge. The code is:

  • Correct: Properly implements the computation expression pattern
  • Idiomatic: Follows F# community conventions
  • Well-tested: Includes comprehensive test coverage
  • Well-documented: Clear documentation with examples
  • Safe: No security or performance concerns

Optional Enhancement (Non-blocking)

Consider adding one additional test case demonstrating assertion failure behavior to ensure the CE properly propagates exceptions:

[<Test>]
[<Category("Fail")>]
member _.FailingAssertion_ThrowsCorrectly() : Task =
    taskAssert {
        do! Assert.That(1 + 1).IsEqualTo(3)  // Should fail
    }

This would verify the error handling path works correctly, though it's likely already covered by existing assertion tests.


Great work, @marcpiechura! This is a valuable addition to TUnit's F# support. 🎉

@thomhurst
Copy link
Owner

Thanks @marcpiechura ! Do you mind adding the negative assertion test as suggested by Claude? So we can confirm it correctly throws if an assertion fails.

@marcpiechura
Copy link
Contributor Author

Sure thing, I've added the test.

@thomhurst thomhurst merged commit 9a75b51 into thomhurst:main Dec 5, 2025
6 of 10 checks passed
@claude claude bot mentioned this pull request Dec 6, 2025
1 task
This was referenced Dec 11, 2025
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.

Better support for Assert in F#

2 participants