Fix disposal leak in skipped tests when constructor has run

[Copilot]

When a test is skipped but its constructor has already been called, the corresponding DisposeAsync/Dispose method was not being invoked, leading to potential resource leaks.

This issue occurs because skipped tests are handled by HandleSkippedTestInternalAsync() which doesn't perform any disposal logic, unlike normal test execution which properly disposes test instances in the finally block of ExecuteTestWithHooksAsync().

Example of the Issue

public class MyTest : IAsyncDisposable
{
    public MyTest()
    {
        // Constructor runs and allocates resources
        Console.WriteLine("Constructor called - resources allocated");
    }

    [Test]
    [Skip("Test should be skipped")]
    public void SkippedTest()
    {
        // This method doesn't run
    }

    public ValueTask DisposeAsync()
    {
        // This was NOT being called for skipped tests
        Console.WriteLine("Resources cleaned up");
        return ValueTask.CompletedTask;
    }
}

Solution

Modified HandleSkippedTestInternalAsync() in SingleTestExecutor.cs to:

1. Check for actual test instances - Only dispose real instances, not SkippedTestInstance or PlaceholderInstance
2. Follow the same disposal pattern - Use identical logic to normal test execution for consistency
3. Handle both disposal interfaces - Support both IAsyncDisposable and IDisposable
4. Dispose tracked objects - Also trigger OnDispose events for injected properties and constructor args
5. Robust error handling - Log disposal errors but don't fail the skipped test

Validation

Added a validation test in SkipConstructorTestValidation that ensures if a constructor is called for a skipped test, the corresponding disposal method must also be called, preventing future regressions.

The fix is surgical (44 lines added, 0 deleted) and only affects the skipped test execution path, maintaining full backward compatibility while ensuring proper resource cleanup.

Fixes #2563.

---

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

[claude]

Claude finished @Copilot's task —— View job

---

PR Review Complete ✅

I've conducted a comprehensive review of PR #2896 that fixes disposal leaks in skipped tests when constructor has run. Here's my detailed analysis:

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and implementation
• Review disposal logic implementation
• Evaluate test coverage and validation
• Check for potential bugs or issues
• Assess performance considerations
• Review security concerns
• Provide comprehensive feedback

---

✅ Overall Assessment: EXCELLENT

This is a well-implemented, surgical fix that addresses a legitimate resource leak issue while maintaining consistency with existing patterns.

Code Quality & Best Practices

✅ Strengths:

• Perfect pattern consistency: The disposal logic in HandleSkippedTestInternalAsync() (lines 264-297) exactly mirrors the pattern used in ExecuteTestWithHooksAsync() (lines 343-371)
• Proper disposal ordering: Test instance disposed first, then tracked objects via OnDispose events
• Robust error handling: Exceptions are logged but don't fail the skipped test (appropriate behavior)
• ConfigureAwait(false): Properly used throughout for library code
• Clear guard conditions: Instance type checks prevent disposal of SkippedTestInstance and PlaceholderInstance

✅ Implementation Details:

• Correct disposal interface handling: Both IAsyncDisposable and IDisposable supported
• Proper async patterns: ValueTask DisposeAsync() handling is correct
• Resource cleanup: Tracked objects disposal matches normal execution path

Security Concerns

✅ No security issues identified

• No sensitive data exposure
• No injection vulnerabilities
• Standard disposal patterns used

Performance Considerations

✅ Minimal performance impact

• Only affects skipped test execution path
• Guard conditions prevent unnecessary work
• No additional allocations or expensive operations

Test Coverage

✅ Excellent validation approach:

• SkipConstructorTestValidation class provides regression testing
• Validation logic correctly tests the core requirement: if constructor is called, disposal must be called
• Handles both scenarios: constructor called vs not called
• Clear error message for debugging

💡 Minor Enhancement Suggestion:

Consider adding a test case that validates the IDisposable (synchronous) interface as well, since the current test only covers IAsyncDisposable. Example:

public class SyncDisposableSkippedTest : IDisposable
{
    public static bool SyncDisposeCalled { get; set; }
    
    public void Dispose()
    {
        SyncDisposeCalled = true;
    }
    
    [Test]
    [Skip("Test should be skipped")]
    public void SkippedTest() { }
}

Bug Analysis

✅ No bugs found. The implementation correctly:

• Checks instance types to avoid disposing placeholder instances
• Maintains disposal ordering (instance first, then tracked objects)
• Handles exceptions gracefully
• Uses appropriate logging levels

Architecture & Design

✅ Well-architected solution:

• Single Responsibility: Only modifies the skipped test handling path
• DRY Principle: Reuses exact same disposal pattern as normal execution
• Backward Compatibility: Zero breaking changes
• Minimal Footprint: 44 lines added, surgical change

Code Readability

✅ Excellent documentation:

• Clear comments explaining the resource leak issue
• Inline comments describing disposal ordering rationale
• Meaningful variable names and clear control flow

Final Recommendation

✅ APPROVE - This is a high-quality fix that:

1. Solves a legitimate resource leak bug
2. Uses consistent, well-tested patterns
3. Includes proper validation tests
4. Has minimal risk and excellent error handling
5. Maintains full backward compatibility

The implementation demonstrates strong understanding of the codebase and follows established conventions perfectly.

---