Skip to content

Conversation

@thomhurst
Copy link
Owner

No description provided.

@github-actions
Copy link
Contributor

Summary

This PR reduces redundant allocations in hot paths by caching parameter type arrays, attribute lookups, and assembly lists.

Critical Issues

None found ✅

Suggestions

1. IsTrxEnabled static caching may be too aggressive (TestExtensions.cs:177-195)

The new _cachedIsTrxEnabled static field is cached globally and never invalidated. While TRX configuration is unlikely to change during a test run, the cache is shared across:

  • Multiple test sessions (if the process is reused)
  • Different TestContext instances that might have different service configurations

Recommendation: Consider making this cache per-TestContext or per-session, or document why global static caching is safe here. If TRX state truly never changes in a process lifetime, the current approach is fine.

2. Consider defensive null check in GetParameterTypes (MethodDataSourceAttribute.cs:269-281)

The new GetParameterTypes helper correctly handles null and empty arrays. However, the loop doesn't validate that parameters[i].Type is non-null. While this is likely guaranteed by the framework, a defensive check could prevent future null reference exceptions if the contract changes.

Optional: Add a null check or document the invariant that ParameterMetadata.Type is never null.

3. Field keyword usage for Assemblies property (ReflectionTestDataCollector.cs:36)

The field keyword (C# 13 semi-auto property) is modern and appropriate here, replacing the previous lock-based caching. Good use of modern C#!

Just verify that:

  • The project targets C# 13+ (<LangVersion>preview</LangVersion> or <LangVersion>13</LangVersion>)
  • The team is aligned on using C# 13 features

Performance Impact Assessment

All changes align with TUnit Rule 4: Performance First. The optimizations target hot paths:
✅ Data generation (MethodDataSourceAttribute) - avoids repeated LINQ allocations per iteration
✅ Test discovery (ReflectionAttributeExtractor) - caches combined attributes
✅ Assembly scanning (ReflectionTestDataCollector) - uses semi-auto property vs lock overhead

These are solid micro-optimizations. Great work!

Verdict

APPROVE - No critical issues. Suggestions are optional improvements for consideration.

@thomhurst thomhurst linked an issue Dec 29, 2025 that may be closed by this pull request
@thomhurst thomhurst merged commit 7bc8026 into main Dec 29, 2025
10 of 13 checks passed
@thomhurst thomhurst deleted the perf/redundant-allocations branch December 29, 2025 11:52
This was referenced Dec 29, 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.

perf: Performance Optimization - Hot Path Improvements

2 participants