You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Populate reflection class property metadata - #7550
Reflection discovery now exposes declared public class properties, including static properties, with working getters and class back-references. All five cached reflection class metadata builders use the same helper, so discovery, property injection and hooks cannot seed the shared cache with an empty property array. Property reflection is cached by type.
Adds integration coverage for ordinary and generic fixtures in both discovery modes, plus a core-helper regression covering declared/public filtering and back-references. Failed dynamic-discovery placeholders were audited; they are uncached error metadata and remain unchanged.
Targeted PropertyMetadataTests: passed in generated and reflection modes (1 case each).
Targeted GenericPropertyMetadataTests*: passed in generated and reflection modes (3 cases each).
Targeted ClassMetadataHelperTests: passed (3 cases, including injection-first cache creation and nullable context).
dotnet build src/TUnit.Core -f netstandard2.0: passed, no warnings.
Native AOT publish of TUnit.TestProject for net10.0/osx-arm64: passed; both targeted native filters passed (1 + 3 cases). Publish emitted existing fixture/dependency trim and AOT warnings; none named the changed helper.
No generator output or public API changes. Engine test suite left to CI per repository instructions.
Review fixes preserve nullable reference annotations (including generic element annotations), cover the remaining injection cache builder, and retain cross-mode parity. Existing indexer getter behavior is tracked separately in #7553.
Summary by CodeRabbit
Bug Fixes
Class metadata now includes public properties declared directly on the class, with usable getters and correct property details, including nullability and static status.
Property metadata is available consistently across test discovery and instance creation.
Tests
Added coverage for property metadata, nullable properties, static properties, and metadata access on closed generic instances.
Review was skipped as selected files did not have any reviewable changes.
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 2cd30d77-1a73-4620-85f0-427ca572f885
Commits
Reviewing files that changed from the base of the PR and between 74b0e9d and 6a4eff1.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
Reflection-based class metadata now includes discovered public properties. The metadata records property reflection data, getters, static status, and nullability. Metadata creation paths use the shared property helper, and tests cover property metadata and getter access.
The helper caches properties declared directly on each type and creates property metadata with reflection data, getters, static status, nullability, and references to the class metadata. Unit tests cover instance and static properties, inherited-property exclusion, cache reuse, injection, and nullable context.
Metadata construction entry points src/TUnit.Core/DataGeneratorMetadataCreator.cs, src/TUnit.Engine/Building/ReflectionMetadataBuilder.cs, src/TUnit.Engine/Discovery/ReflectionHookDiscoveryService.cs, src/TUnit.Engine/Discovery/ReflectionInstanceFactory.cs, tests/TUnit.TestProject/Bugs/_7499/GenericPropertyMetadataTests.cs, tests/TUnit.TestProject/Bugs/_7522/PropertyMetadataTests.cs
These metadata creation paths now call WithProperties. Tests check public property metadata, nullability, static status, and property getters, including getters on a closed generic instance.
#7499 — The added generic property getter assertions cover related getter behavior. The issue’s Native AOT warning remains unaddressed.
#7553 — The helper includes declared public indexers, but creates getters without index arguments; the indexer getter limitation remains.
#7531 — The change adds reflection-side property enumeration that includes indexers, while the described no-index-argument getter behavior remains.
Possibly related PRs
thomhurst/TUnit#7524: The PR changes generated getters for generic test classes, and this change adds getter assertions to the same generic test fixture.
Merge Risk: Minimal · previous review 74b0e
No merge-blocking issue is established.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly and concisely describes the main change: populating reflection class property metadata.
Linked Issues check
Direct linked issue #7522 requires public property metadata with usable getters in both discovery modes. The reviewed head routes all five reflection class metadata builders through ClassMetadataHelpe…
Out of Scope Changes check
Every changed file connects to #7522. The five builder call-site changes (+2/-2 each) route metadata through the shared helper, which is the fix the issue requests. ClassMetadataHelper.cs adds the pro…
Full details: Linked Issues check
Explanation
Direct linked issue #7522 requires public property metadata with usable getters in both discovery modes. The reviewed head routes all five reflection class metadata builders through ClassMetadataHelper.WithProperties: DataGeneratorMetadataCreator, ReflectionMetadataBuilder, ReflectionHookDiscoveryService, ReflectionInstanceFactory, and the injection cache builder GetOrCreateClassMetadata. WithProperties (src/TUnit.Core/PropertyInjection/ClassMetadataHelper.cs) reads public instance and static properties declared on the type, excludes EqualityContract, and sets Getter = instance => p.GetValue(instance). Each PropertyMetadata gets its ClassMetadata and ContainingTypeMetadata back-references. Property reflection is cached by type. Integration fixture tests (tests/TUnit.TestProject/Bugs/_7522 and the _7499 generic fixture) read property values through the current test's metadata. Unit tests in ClassMetadataHelperTests cover declared/public filtering, back-references, static access, cache reuse, injection-first cache creation, and nullable context. The PR description reports these tests passed in both modes. Indexers are included for parity with generated metadata. Their getter behavior is tracked in #7553 and is not a requirement of this PR. Engine test results were not inspected; they are left to CI as stated.
Full details: Out of Scope Changes check
Explanation
Every changed file connects to #7522. The five builder call-site changes (+2/-2 each) route metadata through the shared helper, which is the fix the issue requests. ClassMetadataHelper.cs adds the property discovery, getter, back-reference, and nullability logic that the requested outcome needs. The nullability logic reads compiler annotations directly so the netstandard2.0 build behaves the same. This is an implementation detail for accurate property metadata, not an unrelated change. The _7499 generic test edit adds property-getter assertions to an existing fixture. The issue names #7499 as a related fixture, so this edit supports the same metadata outcome. The new _7522 integration fixture and ClassMetadataHelperTests unit tests support the change. The PR description reports no generator output or public API changes. The diff at the reviewed head shows no unrelated work.
Full details: Docstring Coverage
Explanation
Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 8 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks each property in the light, With getters ready, static fields in sight. Nullable notes are tucked beside each name, And cached reflections follow just the same. The test class fields now hop into the fold.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: bd2bda9d-d022-4dad-868a-b0426299edf6
Commits
Reviewing files that changed from the base of the PR and between 10a0af7 and 74b0e9d.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review.
The change looks good. It fixes the empty ClassMetadata.Properties problem at the root by putting the logic in one helper (ClassMetadataHelper.WithProperties) that all four cached builders call. That removes the risk of one builder seeding the shared cache with an empty array. The back-references are wired correctly (ClassMetadata and ContainingTypeMetadata both point at the final with-copy), and the tests cover ordinary and generic fixtures in both modes.
Minor points, none blocking:
Indexers.GetProperties(Public|Instance|Static|DeclaredOnly) includes indexer properties. Their Getter (p.GetValue(instance)) throws TargetParameterCountException when called. I haven't checked whether the generated metadata skips indexers, but filtering with p.GetIndexParameters().Length == 0 would match it and avoid a trap for consumers.
EqualityContract filter.EqualityContract is protected, so it is never returned by a Public binding. The Where clause is dead code and can go. If you meant to exclude other compiler-generated members, the comment should say so.
Allocation.WithProperties builds the PropertyMetadata[] and the lambdas per class, and the result is cached, so this is fine. Allocating a metadata with {...} copy only to overwrite Properties is slightly wasteful. Passing the properties into the initializer would avoid the copy, but that needs the back-reference fix-up, so the current approach is reasonable.
AOT.WithProperties is annotated RequiresUnreferencedCode only under #if NET8_0_OR_GREATER. That matches the file's existing pattern, so no action is needed.
Overall: approve, with the indexer guard worth adding.
Fixed nullable reference annotations and the remaining reflection injection cache builder in 74b0e9d; all three review threads are resolved.
Validation: PropertyMetadataTests passes in generated and reflection modes (1 each); ClassMetadataHelperTests passes (3); netstandard2.0 core build passes with no warnings; native AOT publish and the native property metadata test pass. Publish retains the existing fixture/dependency warnings; none names the changed helper. Engine tests remain for CI per repository instructions.
Claude's minor notes: generated metadata also includes indexers, so filtering only reflection would break parity; the existing getter-contract problem is tracked in #7553. The EqualityContract exclusion intentionally follows MetadataGenerationHelper's existing filter. The cached copy/back-reference implementation and target-specific annotations remain as reviewed.
The change is solid. All five reflection ClassMetadata builders now go through one WithProperties helper, so no builder can seed the shared cache with an empty Properties array. Back-references are wired after the with copy, so ClassMetadata and ContainingTypeMetadata point at the final instance. I found no correctness blockers. The tests cover both modes, static and generic properties, nullable context, and injection-first cache population.
Suggestions, none blocking:
Indexers:Getter = p.GetValue(instance) throws TargetParameterCountException for indexer properties, because GetProperties returns them. Class property metadata cannot represent public indexer getters #7553 tracks this. Filtering with p.GetIndexParameters().Length == 0 in WithProperties would be a one-line fix, and generated metadata likely skips indexers anyway. It would also prevent a latent runtime failure for any consumer that enumerates Properties.
Static cache:Properties is a ConcurrentDictionary<Type, PropertyInfo[]> that strongly roots every Type. That can keep collectible AssemblyLoadContexts alive. The class cache already roots the types, so this adds nothing new. Still, a ConditionalWeakTable<Type, PropertyInfo[]> would be safer, or at least note the choice in a comment.
Name clash: the static field Properties in ClassMetadataHelper is easy to confuse with ClassMetadata.Properties. A name like DeclaredPropertyCache would read better.
Allocations: each WithProperties call allocates a record copy and a fresh PropertyMetadata[], and Getter is a reflection closure. This only happens once per type thanks to the cache, so it is acceptable. Consider PropertyInfo.GetMethod.CreateDelegate later if getters become hot.
Nullability parsing:IsNullable hand-parses NullableAttribute and NullableContextAttribute. The comment explains this is for netstandard2.0, which is fine. Guarding it with #if NET6_0_OR_GREATER and using NullabilityInfoContext there would cut the custom logic on modern targets. The tests would need to run on both paths to keep them in agreement.
Overall this looks good to merge once CI is green.
Current head 74b0e9d has positive Claude and Codex verdicts and no unresolved threads. Disposition of Claude's remaining nonblocking suggestions: indexer behavior is already tracked in #7553 with the cross-mode parity rationale above; the property cache follows the existing class cache lifetime, so changing only this cache would not enable collectible assembly unloading. The field rename is an optional naming preference. Getter delegate optimization needs measurement, and the current cached construction cost was accepted by the reviewer. Keeping one nullability parser across targets avoids introducing divergent behavior and additional validation paths. These suggestions do not require another code change for this fix.
Marking ready to start CI and the required fresh Claude review. Auto-merge must wait until that review completes successfully and its findings are dispositioned.
The change is sound. All five reflection-mode class metadata builders now go through one helper, ClassMetadataHelper.WithProperties. That closes the gap where an injection-first or hook-first cache fill left Properties = []. I have not run the tests.
What I checked
Back-references are patched after the with copy, so property.ClassMetadata and ContainingTypeMetadata point at the final instance. There is no recursion through the class cache.
DeclaredOnly with Public, Instance and Static matches the generated-mode metadata, and the EqualityContract filter handles records.
The compiler-annotation nullability reader (NullableAttribute, then the getter's NullableContext, then the declaring types' NullableContext) is correct. It keeps netstandard2.0 working without NullabilityInfoContext.
The tests cover both discovery modes, generics, static properties, and injection-first cache creation.
Suggestions (non-blocking)
Duplicated factories. The five builders still hand-roll near-identical new ClassMetadata { ... } blocks and then call WithProperties. It would be more maintainable to have one factory in Core that takes the type and any overrides, so a sixth builder cannot forget the call. That would also remove the Properties = [] placeholder the helper then overwrites.
Cache layering.WithProperties adds a second static ConcurrentDictionary keyed by Type, which duplicates the outer class-metadata cache. The outer cache's factory already runs once per key. The inner cache therefore only helps when the same type is built under different keys, such as FullName against Type. If that is the intent, a comment saying so would help. If not, drop it.
Getter reads.Getter = instance => p.GetValue(instance) throws TargetParameterCountException for indexers. The PR notes this is tracked in Class property metadata cannot represent public indexer getters #7553. Filtering GetIndexParameters().Length > 0 here would avoid exposing a broken getter in the meantime.
Trimming.[RequiresUnreferencedCode] is applied only under #if NET8_0_OR_GREATER. Please confirm that the callers in TUnit.Engine and DataGeneratorMetadataCreator are already annotated or suppressed, so no new IL2026 warnings appear. The PR says the AOT publish was clean.
The earlier blocker is cleared: Claude run 41774511141 succeeded and its review has no blocking findings.
The subsequent .NET run 41774510637 failed on all three platforms at the core-generator snapshots (ClassArgumentParameterTypesTests.Test and GenericPropertyMetadataTests.GeneratesCachedAotCompatibleGetters). Those exact snapshot corrections have since merged in #7549 (4891fce). Rebased onto current main be37704 and pushed head 6a4eff1 rather than rerunning stale CI. No conflict resolution or additional source edits were needed.
Validation: full core generator suite on net10.0 passed: 331 passed, 1 skipped, 0 failed. The initial all-framework invocation could not launch net8.0 because this machine only has the net10 runtime; net8 validation remains for CI.
Disposition of the promotion review's nonblocking suggestions: centralizing the five factories is a separate refactor; the shared helper fixes this issue without broadening that scope. The property cache avoids repeating reflection across the existing metadata cache keys and concurrent factories. Indexer behavior remains tracked in #7553 to preserve parity between discovery modes. Confirmed the Engine callers and DataGeneratorMetadataCreator have existing RequiresUnreferencedCode annotations or IL2026 suppressions; the earlier AOT validation found no new helper warnings.
Fresh CI and automated reviews are required for the rebased head. Auto-merge remains disabled until that head passes the review gate.
The change is sound. All five reflection ClassMetadata builders now go through one WithProperties helper, so no builder can seed the shared cache with empty Properties. The tests cover both discovery modes, the injection-first cache path, nullable context, and the back-references. I have a few non-blocking points, and I didn't run the tests.
Set-only properties.GetProperties(Public|...) also returns properties that have only a public setter. For those, Getter = p.GetValue throws at call time. I'd leave Getter null for them, or check p.GetMethod?.IsPublic, if PropertyMetadata.Getter can tolerate that. The indexer case is already tracked in Class property metadata cannot represent public indexer getters #7553, and this is the same kind of gap, so it could go there.
Inconsistent cache keys.GetOrCreateClassMetadata and DataGeneratorMetadataCreator key the class cache by Type. The engine builders key it by type.FullName ?? type.Name. This PR doesn't introduce that, but closed generics that share a name could collide. Properties are now cached per Type in ClassMetadataHelper.Properties, while the class entry is cached by string. A short comment would help.
Magic string.EqualityContract is filtered out by name. It would be more robust to exclude properties that carry CompilerGeneratedAttribute, or to explain why the name filter is needed.
Allocation. Each property builds a closure (instance => p.GetValue(instance)) and a PropertyMetadata. That's acceptable because the cache amortizes it, but it runs during discovery. If it shows up in benchmarks, p.GetValue can be bound once as a method-group delegate.
Redundant copy.metadata with { Properties = ... } copies the whole record just to patch one field, and each builder then constructs the record with Properties = [] first. Passing the Type and building Properties before the single construction would avoid the copy. It would also remove the null! placeholders, which are patched afterward in a loop.
None of these block the PR. Fixing 1 and 3 would be worthwhile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
GitHub Actions
Failure
Summary
Reflection discovery now exposes declared public class properties, including static properties, with working getters and class back-references. All five cached reflection class metadata builders use the same helper, so discovery, property injection and hooks cannot seed the shared cache with an empty property array. Property reflection is cached by type.
Adds integration coverage for ordinary and generic fixtures in both discovery modes, plus a core-helper regression covering declared/public filtering and back-references. Failed dynamic-discovery placeholders were audited; they are uncached error metadata and remain unchanged.
Closes #7522
Validation
PropertyMetadataTests: passed in generated and reflection modes (1 case each).GenericPropertyMetadataTests*: passed in generated and reflection modes (3 cases each).ClassMetadataHelperTests: passed (3 cases, including injection-first cache creation and nullable context).dotnet build src/TUnit.Core -f netstandard2.0: passed, no warnings.TUnit.TestProjectfor net10.0/osx-arm64: passed; both targeted native filters passed (1 + 3 cases). Publish emitted existing fixture/dependency trim and AOT warnings; none named the changed helper.Review fixes preserve nullable reference annotations (including generic element annotations), cover the remaining injection cache builder, and retain cross-mode parity. Existing indexer getter behavior is tracked separately in #7553.
Summary by CodeRabbit