Skip to content

Conversation

@thomhurst
Copy link
Owner

  • Added a new property CurrentRetryAttempt to the TestContext class to record the current retry attempt for each test execution.
  • Updated the ExecuteWithRetry method in RetryHelper.cs to set the CurrentRetryAttempt property on each retry loop iteration.

@claude
Copy link
Contributor

claude bot commented Oct 3, 2025

Claude finished @thomhurst's task —— View job


PR Review: Expose CurrentRetryAttempt on TestContext

Review Checklist:

  • Read repository guidelines (CLAUDE.md)
  • Analyze changed files for implementation quality
  • Check dual-mode implementation requirements
  • Evaluate test coverage needs
  • Assess performance impact
  • Review security considerations
  • Provide comprehensive feedback

✅ Overall Assessment: APPROVED

This is a well-implemented, focused change that enhances the testing framework's capabilities. The implementation is clean, follows project conventions, and properly maintains the public API snapshots.

🎯 Strengths

Code Quality

  • Clean implementation: Simple property with appropriate visibility (public get; internal set;)
  • Proper placement: Added to line 323 in TestContext.cs without disrupting existing code structure
  • Correct timing: Property is set at the beginning of each retry loop in RetryHelper.cs:13
  • Modern C# conventions: Follows the codebase's established patterns

API Design

  • Well-named: CurrentRetryAttempt clearly indicates its purpose
  • Zero-indexed: Starts at 0 for the first attempt, which is logical and consistent
  • Thread-safe: Property access is inherently thread-safe for reads
  • Immutable from test code: internal set prevents misuse while allowing framework control

Process Adherence

  • ✅ Snapshot tests updated: All three public API snapshot files properly updated
  • ✅ Minimal footprint: Only 2 net additions to core functionality
  • ✅ Clean diff: No unnecessary whitespace or formatting changes

🔍 Technical Analysis

Dual-Mode Compatibility ✅

This change is reflection-mode only, which is appropriate because:

  • The property is a simple runtime state tracked by the execution engine
  • Source generator doesn't need to generate code for this property
  • Retry logic happens entirely within TUnit.Engine (reflection mode)
  • No source generation is needed for this runtime behavior

Performance Impact ✅

  • Negligible overhead: Single integer assignment per retry attempt
  • Hot path optimized: Assignment happens only during retries, not every test
  • No allocations: Simple value type assignment

Security Considerations ✅

  • No security implications: Read-only information exposure
  • No sensitive data: Simple attempt counter with no privacy concerns
  • Controlled access: internal set prevents external manipulation

🧪 Test Coverage Assessment

Current State

  • Existing retry tests: Found comprehensive retry test coverage in RetryTests.cs
  • Manual counting: Tests currently track retry attempts manually with static counters

Recommendation: Add Test Coverage

Consider adding a test that verifies CurrentRetryAttempt is properly set:

[Test]
[Retry(2)]
public void CurrentRetryAttempt_ShouldTrackAttemptNumber()
{
    if (TestContext.Current!.CurrentRetryAttempt == 0)
    {
        throw new Exception("First attempt fails");
    }
    
    // Should succeed on second attempt (CurrentRetryAttempt = 1)
    Assert.That(TestContext.Current.CurrentRetryAttempt).IsEqualTo(1);
}

📋 Minor Suggestions

  1. Documentation: Consider adding XML documentation to the property:

    /// <summary>
    /// Gets the current retry attempt number, starting from 0 for the initial attempt.
    /// </summary>
    public int CurrentRetryAttempt { get; internal set; }
  2. Test enhancement: The existing RetryTests.cs could be enhanced to verify the new property works correctly alongside the manual counters.

✅ Repository Guidelines Compliance

  • ✅ Performance first: No performance impact
  • ✅ Public API changes: Snapshots properly updated
  • ✅ Modern C# syntax: Follows established patterns
  • ✅ Thread safety: Property is thread-safe
  • ✅ Single responsibility: Clear, focused change

This PR successfully exposes retry attempt information to test authors in a clean, efficient manner. The implementation is solid and ready for merge.


@thomhurst
Copy link
Owner Author

@Dona278 - A value of 0 would indicate its not retried yet (it's the first initial run), and counts higher will be how many times it has retried

@thomhurst thomhurst enabled auto-merge (squash) October 3, 2025 17:56
@Dona278
Copy link

Dona278 commented Oct 3, 2025

@thomhurst awesome! Is what I'm searching for.. thank you! I'll try

@thomhurst thomhurst disabled auto-merge October 3, 2025 18:23
@thomhurst thomhurst merged commit d099480 into main Oct 3, 2025
10 of 13 checks passed
@thomhurst thomhurst deleted the feature/3273 branch October 3, 2025 18:23
@thomhurst
Copy link
Owner Author

@thomhurst awesome! Is what I'm searching for.. thank you! I'll try

Its not released yet but it'll be out in a couple of hours or so 😄

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.

3 participants