perf: remove cache and use IReadOnlyList<Attribute>

[TimothyMakkison]

Instead of creating a lazy initalizer, pass in the list of Attributes. Removed the method GetAllAttributes and added the property AllAttributes

See #4303

Tricky to track the savings but this saves at least 10MB, maybe more.

Before

After

[thomhurst]

Summary

Removes lazy-initialized attribute caching and replaces it with a direct AllAttributes property passed during initialization for improved memory efficiency.

Critical Issues

1. Missing snapshot test updates ⛔
This PR changes the public API of TestDetails (removing GetAllAttributes() method, adding AllAttributes property). According to TUnit Rule #2, snapshot tests must be updated when public APIs change.

Required action:

• Run the public API snapshot tests
• Update .verified.txt files (NOT .received.txt)
• Commit the updated snapshot files

The following files need to be updated based on the grep results:

• TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.Net4_7.verified.txt
• TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet8_0.verified.txt
• TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet9_0.verified.txt
• TUnit.PublicAPI/Tests.Core_Library_Has_No_API_Changes.DotNet10_0.verified.txt

Suggestions

1. Documentation update 💡
The ITestDetailsMetadata.GetAllAttributes() interface method documentation at TUnit.Core/Interfaces/ITestDetailsMetadata.cs:40 still says "Cached after first access for performance." This is no longer accurate since caching has been removed. Consider updating to reflect the new implementation.

2. Verify dual-mode compliance ✅
The changes properly handle both source-gen and reflection modes:

• ✅ Source-gen mode: AttributeFactory already returns the attribute array directly
• ✅ Reflection mode: All TestDetails initialization points in TestBuilder.cs and TestBuilderPipeline.cs properly set both AttributesByType and AllAttributes
• ✅ The AllAttributes property is correctly populated from the same source as AttributesByType in all cases

No action needed here - the implementation looks correct.

Verdict

⚠️ REQUEST CHANGES - Missing required snapshot test updates for public API change

[thomhurst]

Can we keep the method so we don't break the API

[TimothyMakkison]

Can we keep the method so we don't break the API

Sure

[TimothyMakkison]

If I need to avoid changing the API how should I pass in the collection, should I keep it a property or pass it in via the constructor?

[thomhurst]

I think constructor makes sense - And is available to us earlier then too

[TimothyMakkison]

I've changed the existing constructor so there is no longer a parameterless one, not sure if this is breaking.

I did think of keeping the paramterless contructor in addition to this, adding a debug check to ensure that the AttributesByType matches allAttributes

[thomhurst]

Technically a breaking change but user's shouldn't really be new'ing up that class themselves so it's okay imo.

You will probably need to run the TUnit.PublicAPI tests and update the snaps

[TimothyMakkison]

Still getting the Sourcy errors so I manually edited the snapshots, hopefully I got i right 😄

[thomhurst]

Public API Snapshot Fix

The public API snapshot tests are failing. I've updated the .verified.txt files and pushed to origin/all_attributes on this repo.

To incorporate the fix, you can either:

1. Cherry-pick the commit:

   git fetch origin all_attributes
   git cherry-pick 307b2c331
   git push

2. Or manually update the verified files by running:

   dotnet test --project TUnit.PublicAPI/TUnit.PublicAPI.csproj -c Release
   # Then copy .received.txt to .verified.txt
   cd TUnit.PublicAPI
   for f in *.received.txt; do mv "$f" "${f%.received.txt}.verified.txt"; done

The changes update 4 snapshot files for the TestDetails constructor signature change.

[thomhurst]

Summary

Removes lazy initialization of attributes cache, instead passing attributes directly to TestDetails constructor to reduce memory allocation.

Critical Issues

Line 68 in TUnit.Core/TestDetails.cs: The XML documentation comment was accidentally converted to a regular comment.

-    /// <summary>
+    // <summary>

This should remain as /// for proper XML documentation generation. The entire comment block (lines 68-72) should use /// not //.

Suggestions

None - the performance optimization approach is sound. The change eliminates the Lazy<> wrapper and its associated allocation overhead by computing the flattened attribute list once at construction time instead of lazily. All call sites correctly pass attributes where available or empty arrays for error cases.

Verdict

⚠️ REQUEST CHANGES - Fix the XML documentation comment syntax

[thomhurst]

Summary

Removes lazy initialization cache from TestDetails and passes attributes list directly to constructor, saving ~10MB+ of memory.

Critical Issues

None found ✅

Suggestions

None - the implementation is clean and all call sites are correctly updated.

TUnit Rules Compliance

✅ Dual-mode implementation: Not required - this change is in the unified code path after metadata collection.
✅ Snapshot testing: All .verified.txt files correctly updated, no .received.txt files committed.
✅ Performance: Excellent improvement - removes Lazy wrapper allocation overhead and passes pre-computed data directly.
✅ AOT compatibility: No reflection changes, existing [DynamicallyAccessedMembers] annotations remain.

Code Review

The changes correctly:

• Remove the _cachedAllAttributes Lazy field
• Accept attributes list via constructor parameter
• Update all 6 call sites in TUnit.Engine to pass attributes
• Handle edge cases (empty arrays for failed tests)
• Maintain the required AttributesByType initialization

The performance improvement is measurable (~10MB reduction) and the approach is sound - since attributes are already computed at all call sites, eliminating the lazy wrapper is pure overhead reduction.

Verdict

✅ APPROVE - Clean performance optimization with all rules satisfied and call sites properly updated.

[thomhurst]

Summary

This PR removes lazy initialization of the attributes cache, instead passing attributes directly to the TestDetails constructor to reduce memory allocation (saves ~10MB).

Critical Issues

None found ✅

Suggestions

1. Update interface documentation 💡
The documentation at TUnit.Core/Interfaces/ITestDetailsMetadata.cs:40 still says "Cached after first access for performance" but caching was removed. Consider updating:

     /// <summary>
     /// Gets all attributes as a flattened collection.
-    /// Cached after first access for performance.
     /// </summary>

This is minor since the implementation documentation in TestDetails.cs was correctly updated.

Previous Review Status

✅ XML documentation issue resolved - The documentation now correctly uses /// (not //)
✅ Public API snapshots updated - All 4 .verified.txt files properly updated
✅ API compatibility maintained - GetAllAttributes() method kept for backwards compatibility
✅ Dual-mode compliance verified - Both source-gen and reflection paths work correctly

All previous critical issues have been addressed.

Verdict

✅ APPROVE - No critical issues. The performance optimization is well-implemented and all previous review feedback has been addressed.