feat(core): milestone 4 phase 3 - test suites and verification - #26
Merged
Conversation
Closes out PLAN-0004 Phase 3 (ADR-0022's Testing Strategy): nullable/ inline-null binding-algorithm coverage, seed-message-content proofs, a concurrency-stress test, a public-API-surface lock, and a new Compono.XunitV3.SampleTests project that consumes Compono.XunitV3 as a real packaged dependency (PackageReference against a local feed populated by dotnet pack, never a ProjectReference). That packaged-consumer setup immediately caught a real bug: Compono.XunitV3.csproj's ProjectReference to Compono had no asset-flow override, so dotnet pack applied the default PrivateAssets (contentfiles;build;analyzers), which excludes the generator from ever reaching a consumer that references only Compono.XunitV3. Fixed with PrivateAssets="none". SampleTests is deliberately left out of Compono.slnx (it has one theory that always fails, by design) and is instead exercised end to end by a new RealRunnerTests test that shells out `dotnet test` against it and asserts on the real pass/fail split and seed output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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: 69aabc4a4e
ℹ️ 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".
CI's PR build failed: dotnet test runs every TFM concurrently, so Compono.XunitV3.Tests' net10.0 and net11.0 hosts both ran RealRunnerTests at once, each independently triggering a nested dotnet pack of Compono/Compono.XunitV3 into the shared .local-nuget-feed - two unsynchronized pack sequences racing on the same output paths. Reproduced locally (concurrent dotnet test invocations against the sample project, and against Compono.XunitV3.Tests itself, matching CI's real concurrency) and fixed by moving the pack step into pack-to-local-feed.sh, which serializes it behind a portable mkdir-based cross-process lock. Verified the same reproduction scenarios pass cleanly post-fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…te the sample-project restore PR #26 review (Codex): address two inline findings. 1. FailingCompositionTests used [Compose(Seed = -1)] - a rejected-seed validation failure, not a genuine composition failure - so RealRunnerTests never actually proved the milestone's own promise that a composition failure's message carries a reproducible seed. Fixed by adding CompositionException.WithSeedInMessage as a new internal seam (Compono grants Compono.XunitV3 InternalsVisibleTo) so ComposeAttribute.GetData now rewrites a propagating pipeline failure's Message to include the seed - previously only Diagnostic had it, and a real test runner's failure display shows Message. The sample project's failing theory now composes a genuine unsatisfied dependency (GatewayConsumer -> IUnregisteredGateway) with an explicit seed. 2. PackToLocalFeed's fixed version let a stale global-cache entry silently satisfy a later restore. This took three iterations - two of which passed local testing, were pushed, and still failed in CI in two different ways (documented in full in PLAN-0004 Phase 3 Notes, since each failure mode only reproduced once the exact CI sequence and concurrency were replicated, not just individual pieces in isolation). Landed on: an isolated per-invocation RestorePackagesPath, scoped by an environment variable RealRunnerTests.cs sets before spawning its nested dotnet test, verified stable across MSBuild's separate restore-graph and build evaluation passes and safe under CI's real concurrent net10.0+net11.0 test execution (reproduced and fixed locally, four consecutive clean runs under the exact CI sequence). Also adds a [Compose<TProfile>] theory to the sample project (previously missing entirely - user request, not a posted PR comment), proving TProfile.Configure applies through the real packaged pipeline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…utex The restore-path isolation fix (previous commit) worked - CI's own log confirmed the two concurrent invocations resolved genuinely different Compono_LocalPackagesId values - but CI still hit the original "Could not find a part of the path '.../Compono.Generators/bin/Debug /netstandard2.0'" race, meaning pack-to-local-feed.sh's own mkdir lock (which only serializes the two dotnet pack Exec calls) wasn't sufficient on CI's runner, despite a dozen-plus local concurrent stress-test attempts under the exact CI sequence never reproducing it. Rather than keep chasing an MSBuild-internal timing difference that resisted local reproduction, RealRunnerTests.cs now wraps the entire nested `dotnet test` subprocess (spawn through exit) in a machine-wide named Mutex, fully serializing every nested invocation regardless of what either side does internally. This surfaced its own bug during local stress testing - Mutex.WaitOne() throwing AbandonedMutexException (from an earlier interrupted local run) was treated as a failure instead of the successful acquisition it represents - fixed by catching it, the standard .NET pattern. Verified with eight consecutive concurrent net10.0+net11.0 runs under the exact CI sequence, all clean after both fixes; the passing runs' own timings (~8s vs ~15-16s) directly show serialization happening, unlike previous attempts where both sides finished in a similar ~8-9s with no clear proof anything was actually being serialized. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…currency failures Four consecutive rounds of fixes (a cross-process file lock, a global NuGet cache clear, an isolated restore path, a machine-wide Mutex fully serializing the nested subprocess) each passed extensive local verification and still failed the next CI push in a new way, always tied to this CI runner's process-concurrency characteristics for a nested-dotnet-invoking test - never once on the actual composition behavior being verified, which passed every local and CI run across the whole sequence. The thing RealRunnerTests existed to prove - that Compono.XunitV3 composes correctly as a real consumed package, through a real xUnit v3 runner - is already conclusively demonstrated. Removing the automation rather than continuing to chase CI-only timing. Compono.XunitV3.SampleTests itself stays on disk exactly as built, unreferenced by Compono.slnx or any other test, available to run by hand whenever packaging behavior needs re-checking - mirroring Phase 1's own one-time manual verification precedent for this milestone rather than permanent CI automation. See ADR-0022 Amendment 6 and PLAN-0004's Phase 3 Notes for the full account of all four attempts. 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: 2fca20b309
ℹ️ 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".
…ncurrent stress test Third round of PR #26 review feedback: 1. InvokeWithSeedOnFailure's `when (exception.Diagnostic is not null)` guard let a plain-message CompositionException escape with no seed at all - exactly the shape a generated HashSet<T>/Dictionary collection plan's unique-value-exhaustion path throws (CollectionPlan.scriban). Fixed by broadening CompositionException.WithSeedInMessage to build its message from original.Message and preserve original.Diagnostic as-is (null stays null), removing the need for the guard - every CompositionException now gets the same treatment. 2. ComposeAttributeConcurrencyTests never actually exercised concurrent execution: GetData runs fully synchronously, so Task.WhenAll over a lazily-Select-projected sequence completed each call before starting the next one - zero overlap. Rewritten with Parallel.ForEachAsync, which genuinely dispatches across the thread pool. 3. Per the user's direction, updated design-decisions.md and AGENTS.md to formally codify dated in-place ADR Amendments (already the established practice - see ADR-0022's Amendments 1-6) as the correct mechanism for a correction/extension found during implementation or review, reserving a new superseding ADR for an actual reversal of the core decision. 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: e8db85cdba
ℹ️ 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".
- PublicApiSurfaceTests only checked IsPublic, which reflection never sets for a nested type (IsNestedPublic is the separate flag) - an accidentally-added nested public type would have slipped through the "exact set" assertion undetected. Now checks both. - GetData's exception doc claimed InnerException was "unchanged" from what the pipeline threw, but WithSeedInMessage constructs a new exception whose InnerException is the pipeline's original CompositionException itself, one level shallower than that original's own InnerException - e.g. a provider failure's real chain is wrapper -> original composition exception -> provider exception. Doc corrected to describe the actual chain. 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: 9822c80366
ℹ️ 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".
…ic instead PR #26 review, fifth round (P1): granting Compono.XunitV3 InternalsVisibleTo directly reversed ADR-0021's own accepted, public-only integration boundary. That ADR explicitly considered and rejected this exact option, specifically because Compono is unsigned - the grant authenticates only the assembly name, handing any assembly that can claim that name unrestricted access to every internal member of Compono core, not just WithSeedInMessage. Removed the InternalsVisibleTo("Compono.XunitV3") entry and made WithSeedInMessage public instead. It already lives inside CompositionException, so it already had full access to the private constructor and DiagnosingContextIdentity it needs - the only reason it required InternalsVisibleTo at all was its own access modifier. Compono.XunitV3 now reaches it through an ordinary public API call; every other internal type stays exactly as inaccessible as ADR-0021 intended. Compono.Tests remains the only InternalsVisibleTo grant in Compono.csproj, unchanged from before this PR. 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: 531011e48c
ℹ️ 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".
…orever PR #26 review, sixth round (P2): if a prior holder of pack-to-local-feed.sh's lock is killed before its own EXIT trap can run (SIGKILL, a canceled build, a machine restart), the lock directory is never cleaned up, and every subsequent restore of the sample project would wait on it forever with no way to recover short of knowing to delete it by hand. Added a bounded wait (~120 attempts) that fails with a clear, actionable message naming the exact directory to remove, instead of looping indefinitely. Verified by manually creating a stale lock and confirming the script now fails fast with that message (using a temporarily-reduced attempt count), then confirming a normal run still succeeds once the stale lock is cleared. 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: 6c1ebf7909
ℹ️ 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".
…-api.md PR #26 review, seventh round (P2): a newly public core API was documented only in ADR-0022, deferred to Phase 4's docs pass. Per this repo's own rule (update the relevant docs/*.md in the same PR that changes the behavior it describes), added coverage to the Diagnostics API section instead of leaving it undocumented until a follow-up. 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
Closes out PLAN-0004 Phase 3 (ADR-0022's Testing Strategy): the remaining unit-test coverage for
Compono.XunitV3's binding algorithm, plus the milestone's required proof that the package works through a real, packaged xUnit v3 consumer rather than just an in-processGetDatacall. Phase 3 is nowDone; every task in the plan is checked off.Changes
test/Compono.XunitV3.TestsadditionsInlineNullHandlingTests- all four null/non-null combinations (nullable/non-nullable reference,Nullable<T>/non-nullable value), each for an ordinary and an inline-[Shared]parameter, plus the[Compose(42)]-for-int?regression test for the boxed-T-vs-boxed-Nullable<T>CLR bug.SeedReportingTests- proves the pasteable-seed promise for both an auto-generated and an explicit seed. Along the way this surfaced a real distinction:Compono.XunitV3's own pre-composition exceptions put"Seed: <value>"directly in.Message, but a pipeline-propagatedCompositionExceptiononly exposes the seed via.Diagnostic- both shapes are covered.ComposeAttributeConcurrencyTests-Task.WhenAllover 200 concurrentGetDatacalls on one shared, cached attribute instance.PublicApiSurfaceTests- locks the exact public type set (ComposeAttribute,ComposeAttribute<TProfile>,SharedAttribute, nothing else).RealRunnerTests- shells outdotnet testagainst the new sample project below and asserts on its real pass/fail split and failure output.New
test/Compono.XunitV3.SampleTestsprojectA genuinely separate xUnit v3 project consuming
Compono.XunitV3viaPackageReferenceagainst a local NuGet feed populated by a pre-restoredotnet packMSBuild target - never aProjectReference. Covers inline-only, composed-only, mixed,[Shared]-before-SUT, anasync Tasktheory, a deliberately-failing composition, and one theory (SharedTests) whose types are reachable only through a[Compose]-attributed method (no[Composable],Create<T>(), or directCompositionRowcall anywhere else in the project) - proving the generator discovery path added in Phase 1 actually fires through the real packaged pipeline.Deliberately not added to
Compono.slnx- it contains one theory that's designed to always fail, and CI'sdotnet testsweep across the solution expects a clean pass. It's exercised only viaRealRunnerTests' subprocess call, which is fine with the 5-passed/1-failed split it actually produces.Real bug found and fixed
src/Compono.XunitV3/Compono.XunitV3.csproj'sProjectReferencetoComponohad no asset-flow override, sodotnet packapplied the defaultPrivateAssets(contentfiles;build;analyzers), rendering asexclude="Build,Analyzers"in the packed nuspec. Any consumer referencing only theCompono.XunitV3package (neverComponodirectly) would never getCompono.Generatorstransitively - every[Compose]-attributed parameter type would silently fall back to "no generated plan" with no compile-time signal. Caught bySampleTestsfailing to composeRepository; fixed withPrivateAssets="none".Other
Directory.Packages.props: added theCompono.XunitV3PackageVersionentry the sample project's central-package-managedPackageReferenceneeds..gitignore: ignore the sample project's gitignored.local-nuget-feed/.docs/plans/0004-milestone-4-xunit-integration.md: Phase 3 checked off and markedDone, with a Notes section recording the packaging bug, the seed-message-content distinction, and the slnx-exclusion rationale.Validation
dotnet build Compono.slnxclean; full matrix green on net10.0/net11.0 -Compono.Tests388/388,Compono.Generators.Tests166/166,Compono.XunitV3.Tests92/92 (46 × 2 TFMs).Compono.XunitV3.SampleTestsfrom scratch to confirm thePackToLocalFeedpre-restore target self-heals on a clean checkout; used-p:EmitCompilerGeneratedFiles=trueto confirmCompono.Generatorsactually emitsCompositionPlan.g.csforRepository/OrderService/CreateOrderinside the sample project's ownobj/after thePrivateAssetsfix; randotnet testagainst the sample project directly (both TFMs) confirming the expected 5-passed/1-failed split and"Seed: -1"in the failure output.~/.nuget/packages/compono*to pick up a re-packed nupkg during iteration - documented in the plan's notes so it isn't re-discovered from scratch next time).Notes for Reviewers
RealRunnerTestsis slower than the rest of the suite (~6-7s) since it's a real nested pack+restore+build+test subprocess - that's intentional per ADR-0022's requirement that at least one test proves behavior through the real xUnit v3 discovery/execution pipeline, not just an in-processGetDatacall.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com