Fix data and properties being injected/initialised too eagerly

[thomhurst]

Fixes #2647

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've reviewed this pull request focusing on code quality, potential bugs, performance, security concerns, and test coverage.

Review Tasks:

• Analyze all changed files to understand the implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide constructive feedback

Analysis Summary:

This PR addresses issue #2647 by preventing eager initialization of test class instances and data properties during test discovery. The main changes introduce a PlaceholderInstance pattern to defer instance creation until test execution time.

---

🟢 Positive Aspects

Well-designed Lazy Loading Pattern

• The PlaceholderInstance singleton is a clean, purpose-built solution that clearly distinguishes between placeholder instances (during discovery) and skipped test instances
• The lazy factory pattern with TestClassInstanceFactory properly defers expensive operations until execution time

Consistent Implementation

• All instance creation paths properly use the new factory pattern (TestBuilder.cs:557, TestMetadata.cs:79, ReflectionTestMetadata.cs:48)
• Good separation of concerns between discovery-time placeholder and execution-time instance creation

Performance Improvement

• Removes 23 lines of eager property initialization from ClassDataSources.cs (lines 138-162), which was happening during discovery
• Prevents unnecessary constructor calls and data source property injection during test discovery phase

---

🟡 Areas for Improvement

1. Missing XML Documentation

PlaceholderInstance.cs:7-15

// Good: Has summary documentation explaining purpose
// Consider: Adding <example> tags to clarify usage vs SkippedTestInstance

2. Error Handling Considerations

TestMetadata<T>.cs:79-82 and ReflectionTestMetadata.cs:48-51

• Factory invocation could benefit from better error context if the factory throws an exception
• Consider wrapping factory calls with more descriptive error messages

3. Code Consistency

TestBuilder.cs:148 and TestBuilder.cs:221

TestClassInstanceFactory = () => Task.FromResult<object>(null!)

This pattern appears multiple times. Consider extracting to a static readonly field:

private static readonly Func<Task<object>> NullInstanceFactory = () => Task.FromResult<object>(null!);

---

🟢 Security Assessment: No Concerns

• Changes don't introduce security vulnerabilities
• Lazy loading pattern doesn't expose new attack vectors
• Factory pattern maintains proper encapsulation

---

🟡 Test Coverage Considerations

Missing Test Coverage

The PR doesn't include test changes, which raises some concerns:

• No tests verify that instance creation is properly deferred until execution
• No tests ensure PlaceholderInstance is used correctly during discovery
• No tests validate the factory pattern works correctly with generic types
• No regression tests to prevent eager initialization from being re-introduced

Recommendation

Consider adding integration tests that:

1. Verify expensive constructors aren't called during discovery
2. Ensure property injection only occurs during execution
3. Test the generic type resolution still works with deferred instance creation

---

🟢 Performance Impact: Positive

• Discovery Phase: Significantly faster due to avoiding unnecessary instance creation and property injection
• Execution Phase: Minimal overhead from factory pattern
• Memory: Better memory usage during discovery as expensive objects aren't created upfront

---

🔧 Minor Suggestions

1. Consider Factory Method Naming

ExecutableTestCreationContext.cs:17
The property TestClassInstanceFactory could be more descriptive:

public Func<Task<object>>? LazyTestClassInstanceFactory { get; init; }

2. Null Safety

TestBuilder.cs:287
The captured variables pattern is good, but consider adding null checks in the factory:

instanceFactory = async () => {
    ArgumentNullException.ThrowIfNull(capturedMetadata);
    return await CreateInstance(capturedMetadata, capturedClassGenericArgs, capturedClassData, capturedContext);
};

---

Overall Assessment: ✅ Approved with Minor Suggestions

This is a well-implemented performance optimization that effectively solves the eager initialization problem. The architecture is sound, the code is clean, and the changes are consistent throughout the codebase. The main recommendation is to add test coverage to ensure the lazy loading behavior is properly validated and to prevent regressions.

Code Quality: 8.5/10
Performance: 9/10
Security: 10/10
Test Coverage: 6/10 (needs tests)

[claude]

Claude finished @thomhurst's task —— View job

---

Code Review Complete ✅

I've reviewed this pull request focusing on code quality, potential bugs, performance, security concerns, and test coverage.

Review Tasks:

• Analyze all changed files to understand the implementation
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide constructive feedback

Analysis Summary:

This PR addresses issue #2647 by preventing eager initialization of test class instances and data properties during test discovery. The main changes introduce a PlaceholderInstance pattern to defer instance creation until test execution time.

---

🟢 Positive Aspects

Well-designed Lazy Loading Pattern

• The PlaceholderInstance singleton is a clean, purpose-built solution that clearly distinguishes between placeholder instances (during discovery) and skipped test instances
• The lazy factory pattern with TestClassInstanceFactory properly defers expensive operations until execution time

Consistent Implementation

• All instance creation paths properly use the new factory pattern (TestBuilder.cs:287, TestMetadata.cs:79, ReflectionTestMetadata.cs:48)
• Good separation of concerns between discovery-time placeholder and execution-time instance creation

Performance Improvement

• Removes 23 lines of eager property initialization from ClassDataSources.cs (lines 138-162), which was happening during discovery
• Prevents unnecessary constructor calls and data source property injection during test discovery phase

---

🟡 Areas for Improvement

1. Missing XML Documentation

PlaceholderInstance.cs:7-15

// Good: Has summary documentation explaining purpose
// Consider: Adding <example> tags to clarify usage vs SkippedTestInstance

2. Error Handling Considerations

TestMetadata<T>.cs:79-82 and ReflectionTestMetadata.cs:48-51

• Factory invocation could benefit from better error context if the factory throws an exception
• Consider wrapping factory calls with more descriptive error messages

3. Code Consistency

TestBuilder.cs:148 and TestBuilder.cs:221

TestClassInstanceFactory = () => Task.FromResult<object>(null!)

This pattern appears multiple times. Consider extracting to a static readonly field:

private static readonly Func<Task<object>> NullInstanceFactory = () => Task.FromResult<object>(null!);

---

🟢 Security Assessment: No Concerns

• Changes don't introduce security vulnerabilities
• Lazy loading pattern doesn't expose new attack vectors
• Factory pattern maintains proper encapsulation

---

🟡 Test Coverage Considerations

Missing Test Coverage

The PR doesn't include test changes, which raises some concerns:

• No tests verify that instance creation is properly deferred until execution
• No tests ensure PlaceholderInstance is used correctly during discovery
• No tests validate the factory pattern works correctly with generic types
• No regression tests to prevent eager initialization from being re-introduced

Recommendation

Consider adding integration tests that:

1. Verify expensive constructors aren't called during discovery
2. Ensure property injection only occurs during execution
3. Test the generic type resolution still works with deferred instance creation

---

🟢 Performance Impact: Positive

• Discovery Phase: Significantly faster due to avoiding unnecessary instance creation and property injection
• Execution Phase: Minimal overhead from factory pattern
• Memory: Better memory usage during discovery as expensive objects aren't created upfront

---

🔧 Minor Suggestions

1. Consider Factory Method Naming

ExecutableTestCreationContext.cs:17
The property TestClassInstanceFactory could be more descriptive:

public Func<Task<object>>? LazyTestClassInstanceFactory { get; init; }

2. Null Safety

TestBuilder.cs:287
The captured variables pattern is good, but consider adding null checks in the factory:

instanceFactory = async () => {
    ArgumentNullException.ThrowIfNull(capturedMetadata);
    return await CreateInstance(capturedMetadata, capturedClassGenericArgs, capturedClassData, capturedContext);
};

---

Overall Assessment: ✅ Approved with Minor Suggestions

This is a well-implemented performance optimization that effectively solves the eager initialization problem. The architecture is sound, the code is clean, and the changes are consistent throughout the codebase. The main recommendation is to add test coverage to ensure the lazy loading behavior is properly validated and to prevent regressions.

Code Quality: 8.5/10
Performance: 9/10
Security: 10/10
Test Coverage: 6/10 (needs tests)

---