feat(tunit): profile variants for Compono.TUnit (PLAN-0040 Phase 1) - #76
Merged
Conversation
…e variants Mirrors Compono.XunitV3's profile-attribute family exactly, including ConfigProfileBinder's reflection-based TConfig/TProfile construction. Extends the AOT smoke test to exercise the config-generic form, which surfaced a real Native AOT gap (ADR-0041 Amendment 1): the trimmer strips constructors on closed generic type arguments unless annotated, so ConfigProfileBinder failed at runtime with "0 public constructors" on a type that plainly has one. Fixed with DynamicallyAccessedMembers annotations end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ding test coverage BindingPlan.ValidateSignature now detects more than one Compose-family attribute stacked on a method, mirroring Compono.XunitV3's identical check - adapted to what MethodMetadata exposes: a parameter's ReflectionInfo.Member gives the declaring MethodInfo directly, with a Type.GetMethod(name, Type.EmptyTypes) fallback for zero-parameter methods. Adds ComposeAttributeConfigBindingTests.cs (ConfigProfileBinder coverage, mirroring Compono.XunitV3.Tests) and stacked-attribute BindingPlanTests cases, both parameter and zero-parameter method shapes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…run for real under TUnit Adds Compono.NSubstitute to Compono.TUnit.SampleTests' local-feed pack chain and mirrors Compono.XunitV3.SampleTests/NSubstituteTests.cs exactly: [Shared] IOrderRepository composed via [Compose<NSubstituteTestProfile>], UseNSubstitute() wired through the profile, reused inside a composed constructor parameter - the real packaged Compono.NSubstitute -> Compono.TUnit -> Compono dependency chain, under a real TUnit runner. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…N-0040 Phase 1 Extends docs/packages/compono-tunit.md and skills/compono/references/tunit.md with [Compose<TProfile>]/[Compose<TProfile, TConfig>] usage and the real stacked-attribute rejection behavior, replacing the earlier "not shipped yet"/"stacking is undefined" language. Adds the missing inline-values- combined-with-a-profile test case PLAN-0040 Phase 1 called for. Checks off every Phase 1 task and records the AOT-gate finding in the plan's Notes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ncipollina
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a7fdd18f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ttributes ComposeAttribute<TProfile>/ComposeAttribute<TProfile, TConfig> are new public types - the CI drift-detection check (generate-api-reference.sh) flagged the docs/reference/api snapshot as stale for this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…p, fix package description BindingPlan.ResolveMethodInfo's zero-parameter fallback used Type.GetMethod(name, Type.EmptyTypes), which matches by parameter types only, not generic arity - a class declaring both a zero-parameter Run() and a zero-parameter-but-generic Run<T>() threw AmbiguousMatchException instead of reaching the existing generic-method CompositionException. Now filters GetMethods() by both zero declared parameters and this test's own GenericTypeCount. Also restores Compono.TUnit's NuGet description to describe the full shipped attribute family - it still said the profile variants "ship in a later phase" despite Phase 1 having just shipped them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e36facd4b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ild for TUnit test projects
Root cause of the recurring CI flake ("Could not load ... Microsoft.Testing
.Extensions.MSBuild, Version=2.3.3.0 ... cannot find the file", a different
TFM each run): that assembly ships in Microsoft.Testing.Platform.MSBuild,
which had no Central-Package-Management-pinned PackageVersion in this repo.
TestingPlatformDotnetTestSupport=true relies on the .NET SDK implicitly
auto-adding that package reference, and under CPM with no pinned version
that implicit reference resolved inconsistently across a solution-wide
parallel multi-TFM build - never reproducible via a single-project local
build, exactly matching what CI showed three times in a row (net10.0, then
net8.0+net11.0, then net11.0 again).
Compono.XunitV3.Tests never hit this because it already references
Microsoft.Testing.Platform (pinned) explicitly via test/Directory.Build
.targets' shared xUnit-only ItemGroup. Compono.TUnit.Tests and
Compono.TUnit.SampleTests had no equivalent explicit, pinned reference -
added Microsoft.Testing.Platform.MSBuild (matching Microsoft.Testing
.Platform's own 2.3.3 pin) to both, plus the missing PackageVersion entry.
Verified locally: Microsoft.Testing.Extensions.MSBuild.dll now present in
both projects' build output for all four TFMs (net8.0/9.0/10.0/11.0), full
solution build clean, Compono.TUnit.Tests 196/196 passing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hException bug ValidateSignature's comment (and the plan's own Phase 1 Notes entry) still described the original Type.EmptyTypes-only zero-parameter lookup after e36facd replaced it with an arity-aware filter - contradicting the actual implementation and risking a future "simplification" back to the broken version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3db9bcca6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…I drift Root cause of the recurring Microsoft.Testing.Extensions.MSBuild.dll flake (previous commit ff452f9's fix was real but insufficient): the workflow's floating "11.0.x" dotnetVersion input let actions/setup-dotnet install whatever the newest 11.0 preview SDK happens to be. CI was actually running 11.0.100-preview.7.26381.103 - a newer build than global.json's own pinned 11.0.100-preview.6.26359.118 - and global.json's rollForward: latestFeature policy silently accepted the mismatch instead of failing loudly. Verified locally: preview.6 (installed here) builds every project to its own ordinary bin/<config>/<tfm>/ directory, exactly as expected. preview.7's CI logs show every project across the whole .slnx solution - libraries and test projects alike - building into one shared publish/<tfm>/ directory instead, a new preview-SDK behavior change that creates exactly the kind of concurrent-write race (many projects' builds writing runtime dependencies into the same folder) that would explain a runtime dependency nondeterministically going missing for a different project/TFM combination on every run. Pins the PR build workflow's dotnetVersion to the exact SDK build global.json already specifies, so CI can no longer drift onto an untested newer preview. package-validation.yaml/docs.yml/publish-*.yaml still use the floating 11.0.x pattern - not touched here since they don't hit this same failure mode and deserve their own audit, not a blind copy-paste fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ld output into one shared directory The actual root cause of the recurring Microsoft.Testing.Extensions.MSBuild .dll flake (both previous fixes - ff452f9's explicit package reference and 3a6e7cb's exact SDK pin - were real improvements but not the cause): devops-templates' reusable PR-build workflow sets a GITHUB_ENV variable named "outputPath" for its own later Blazor/Lambda publish steps, but that variable stays a process env var for every subsequent step in the same job - including this repo's own plain "dotnet build"/"dotnet test" steps. MSBuild auto-imports environment variables as property values (case-insensitively) whenever no project file has already set that property, so the leaked "outputPath" env var satisfied MSBuild's standard OutputPath property for every project in the solution, collapsing all of them - libraries and test projects alike, every TFM - into one shared directory instead of each project's own bin/<config>/<tfm>/. Reproduced locally by setting the same env var before `dotnet build Compono.slnx`: every project's output merged into the env var's target directory, exactly matching CI's observed publish/<tfm>/ layout (which never appears locally without it) - a shared directory across many concurrently-building projects is exactly the kind of setup where one project's copy of a shared runtime dependency can lose a race against another's, matching the flake's own signature (same exception, a different TFM/project combination on every run, never reproducible via a single local build). Directory.Build.props now unconditionally resets OutputPath to empty, letting the SDK's own conditional default-path computation fire fresh regardless of what the CI environment leaks in - restoring isolated per-project/per-TFM output. Verified locally under the exact leaked-env- var condition: no shared directory, Microsoft.Testing.Extensions.MSBuild .dll present for all four TFMs, full solution build + `dotnet test --solution` both 1873/1873 passing, and `dotnet pack` unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…le fails
ApplyProfile runs while the base class's Lazy<Composer> is still being
built - before ComposeRow ever calls Composer.CreateRow, so there's no
CompositionRow/row.Seed yet if TProfile.Configure itself throws. This
form's ApplyProfile called builder.AddProfile<TProfile>() unwrapped, so
even a configured Seed went unreported - unlike
ComposeAttribute<TProfile, TConfig>'s identical failure path, which
already wraps with CompositionException.WithSeedInMessage. Same gap
exists in Compono.XunitV3.ComposeAttribute{TProfile} (mirrored
faithfully from there) - worth its own follow-up, out of scope here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4ee05db2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ComposeAttribute<TProfile>.ApplyProfile's new seed-wrapping try/catch (e4ee05d) let a negative configured seed slip through to the profile- failure path when TProfile.Configure also throws - reporting the profile failure with "Seed: -1" embedded instead of the documented negative-seed diagnostic. The negative-seed check must run before any profile work is attempted, matching ComposeAttribute<TProfile, TConfig>'s identical precedence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cf355451f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…claim Same drift as the BindingPlan.cs comment fixed in e3db9bc, missed in that pass - skills/compono/references/tunit.md still described the zero-parameter fallback as a plain GetMethod(name, Type.EmptyTypes) call, which would throw AmbiguousMatchException for a class declaring both Run() and Run<T>(). Corrected to describe the real arity-aware GetMethods() filter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05c85e688d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- tunit.md's ComposeAttribute<TProfile, TConfig> section described a bad TConfig/TProfile constructor shape as a "binding-plan-construction time" failure - it's actually raised during composer/profile initialization (ApplyProfile, inside the cached Lazy<Composer>), before BindingPlan is ever built. - SampleTestMethods.cs's own fixture comment still described the zero-parameter fallback as a plain Type.GetMethod(name, Type.EmptyTypes) call - the third copy of this same stale claim found across the diff, corrected to describe the real arity-aware GetMethods() filter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
📋 Summary
Implements PLAN-0040 Phase 1:
ComposeAttribute<TProfile>andComposeAttribute<TProfile, TConfig>forCompono.TUnit, mirroringCompono.XunitV3's profile-attribute family exactly, plus stacked-attribute rejection and full Native AOT verification.Compono.TUnitnow has parity withCompono.XunitV3's complete attribute family.📝 Changes
Profile attributes
ComposeAttribute<TProfile>— fixed, default-constructed profile applied to the row'sComposer.ComposeAttribute<TProfile, TConfig>+ConfigProfileBinder— profile built from attribute-constructor-supplied config arguments, ported byte-for-byte fromCompono.XunitV3.Binding.ConfigProfileBinder.Stacked-attribute validation
BindingPlan.ValidateSignaturenow rejects a method carrying more than one Compose-family attribute, resolving the method's realMethodInfovia a parameter'sReflectionInfo.Member(with aType.GetMethod(name, Type.EmptyTypes)fallback for zero-parameter methods, sinceMethodMetadatadoesn't expose the attribute list directly).Native AOT gate (ADR-0041 Amendment 1)
test/Compono.TUnit.AotSmokeTestto exercise[Compose<TProfile, TConfig>]alongside the plain form.ConfigProfileBinder'sConstructorInfo.Invoke-based construction failed at runtime ('ProfileConfig' must have exactly one public constructor... but has 0). Fixed withDynamicallyAccessedMembers(PublicConstructors)annotations onConfigProfileBinderand onComposeAttribute<TProfile, TConfig>'s own type parameters.Real end-to-end proof
test/Compono.TUnit.SampleTests/NSubstituteTests.cs— the plan's own Goal-section scenario ([Shared] IOrderRepositorycomposed via[Compose<NSubstituteTestProfile>], reused inside a composed constructor parameter), run for real under TUnit, mirroringCompono.XunitV3.SampleTests/NSubstituteTests.cs.Docs
docs/packages/compono-tunit.mdandskills/compono/references/tunit.mdupdated with the shipped profile-attribute forms and real stacked-attribute-rejection behavior (replacing "not shipped yet" / "stacking is undefined" language).docs/plans/0040-compono-tunit-package-design.md— Phase 1 checked off,Status: Done, Notes updated with the AOT-gate finding.🧪 Validation
Compono.slnx) builds clean in Release;Compono.TUnit.Tests188/188 passing across all four TFMs (net8.0/net9.0/net10.0/net11.0);Compono.TUnit.SampleTests20/20 passing (5 tests × 4 TFMs), including the real NSubstitute end-to-end scenario.dotnet publish -c Release -p:PublishAot=true -r osx-arm64 --self-contained true+ run against the packagedCompono/Compono.TUnitdependency chain — both[Compose]and[Compose<TProfile, TConfig>]pass as real native binaries. A-p:TrimmerSingleWarn=falsepass confirms zero trim warnings fromCompono.TUnit's own code (only the two pre-existing harness-onlyIL2072warnings from Phase 0 remain).Nullable<T>unwrapping), stacked-attribute rejection for both a parameter-bearing and zero-parameter method, inline values combined with a profile-applied composer.💬 Notes for Reviewers
The AOT-gate finding (see Changes above) is the most interesting part of this PR — it's a genuine bug that a real
dotnet publish -p:PublishAot=truerun caught and a unit-test-only verification pass would have missed entirely.