diff --git a/.agents/skills/engineering-workflow/references/design-decisions.md b/.agents/skills/engineering-workflow/references/design-decisions.md index 6c540f6..8087728 100644 --- a/.agents/skills/engineering-workflow/references/design-decisions.md +++ b/.agents/skills/engineering-workflow/references/design-decisions.md @@ -31,9 +31,14 @@ Four places, each with a different job — don't blur them together: shapes, not shipped ones) — when a doc is describing a target rather than current reality, keep that distinction explicit rather than letting a doc quietly drift from "intent" to "fact" without the code to back it. -- **`docs/adr/*.md`** — the actual decision record: numbered, permanent, - immutable once accepted. This is where "what was decided and why, and - what was rejected" actually lives — see **Writing an ADR** below and +- **`docs/adr/*.md`** — the actual decision record: numbered, permanent. + Its original Decision/Rationale/Consequences text is immutable once + accepted — never rewritten to look like it was always this way — but a + correction or extension discovered later gets recorded as a dated + Amendment appended to the same ADR, rather than either editing the + original text or opening a new ADR for something that isn't actually a + reversal. This is where "what was decided and why, and what was + rejected" actually lives — see **Writing an ADR** below and `docs/adr/README.md` for the numbering/status mechanics. `docs/adr/0000-adr-template.md` is the template to copy for every new ADR. An ADR is not a plan — it doesn't get a task checklist or a @@ -140,20 +145,38 @@ a short one, not a skipped one: 5. Update the relevant `docs/*.md` topic doc to reflect the resulting current (or intended, per the pre-implementation note above) state, linking back to the ADR rather than re-explaining the reasoning. -6. If this ADR changes direction from an earlier one, set the earlier - ADR's `Status` to `Superseded by ADR-XXXX` — don't edit its Decision/ - Rationale/Consequences, an accepted ADR is a historical record, not a - living doc. +6. If this ADR changes direction from an earlier one — its core Decision + Outcome is being replaced, not just corrected or extended — set the + earlier ADR's `Status` to `Superseded by ADR-XXXX` — don't edit its + Decision/Rationale/Consequences in place for a change this size; write + the new decision as its own ADR instead. +7. If implementation or review surfaces a correction, clarification, or + extension to an already-`Accepted` ADR's decision — not a reversal of + it — record it as a dated `## Amendment N (YYYY-MM-DD): ` section + appended to that same ADR, rather than opening a new one. This is the + established pattern in this repo (see ADR-0022's Amendments 1-6 for + real examples: a caching-interaction clarification, a reverted feature + attempt, a gap a later PR review caught, a test-infrastructure decision + changed after repeated CI failures) — an Accepted ADR's *original* + Decision/Rationale/Consequences text stays exactly as written (that's + the "immutable historical record" property that matters: what was + decided, and why, at the time, is never rewritten to look like it was + always this way), but the ADR as a whole is allowed to grow dated + Amendment sections that record what was learned afterward. Reach for + step 6 (a full new ADR, superseding this one) only when the core + decision itself is being reversed or replaced — a genuinely different + answer to the same question, not a correction to how the original + answer was implemented or verified. Keep the ADR itself to decision content — Status, Decision, Rationale (or Context/Decision Drivers/Considered Options/Decision Outcome for a deep -dive that needs the fuller shape), Consequences. Resist the urge to also -list every file that'll change or write a task checklist inside it; once -an ADR is `Accepted`, that kind of content goes stale the moment -implementation starts diverging from the plan in some small way, and -you're stuck either leaving the ADR wrong or editing a record that's -supposed to be immutable. That content belongs in a plan instead — see -below. +dive that needs the fuller shape), Consequences, and any Amendments per +step 7 above. Resist the urge to also list every file that'll change or +write a task checklist inside it; once an ADR is `Accepted`, that kind of +content goes stale the moment implementation starts diverging from the +plan in some small way, and you're stuck either leaving the ADR wrong or +editing content that's supposed to stay as originally written. That +content belongs in a plan instead — see below. A design dive — light or deep — that never produces an ADR hasn't actually finished. The brainstorm (for a deep dive) or the quick judgment call (for diff --git a/.gitignore b/.gitignore index 4966531..89e618a 100644 --- a/.gitignore +++ b/.gitignore @@ -435,6 +435,10 @@ FodyWeavers.xsd .idea +# Local NuGet feed populated by Compono.XunitV3.SampleTests' pre-restore pack step +# (test/Compono.XunitV3.SampleTests/nuget.config) - regenerated on every restore, never committed. +.local-nuget-feed/ + # macOS .DS_Store .DS_Store? diff --git a/AGENTS.md b/AGENTS.md index c50cd92..ccd138f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,9 +142,13 @@ list). The load-bearing ones: ## The non-negotiables - Architecture is ADR-driven: every non-trivial design decision gets an - ADR in `docs/adr/` (immutable once `Accepted`) before code is written - against it. Don't silently evolve architecture in code — see - `references/design-decisions.md`. + ADR in `docs/adr/` before code is written against it. An ADR's original + Decision/Rationale/Consequences text is immutable once `Accepted` — + never rewritten in place — but a correction or extension found later is + recorded as a dated Amendment appended to that same ADR (see ADR-0022's + Amendments for real examples); reserve superseding with a whole new ADR + for an actual reversal of the core decision. Don't silently evolve + architecture in code — see `references/design-decisions.md`. - Keep PRs scoped to one decision or one feature — no bundling an unrelated fix or refactor. If a plan is phased, that's one PR per phase, and the prior phase's status/PR-merge state should be current diff --git a/Directory.Packages.props b/Directory.Packages.props index fc6f54b..bc1e30e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -16,6 +16,10 @@ <!-- Compono.XunitV3's own dependency, matching xunit.v3.mtp-v2's own resolved version - the Xunit.v3.DataAttribute/ITheoryDataRow extension surface, per ADR-0022. --> <PackageVersion Include="xunit.v3.extensibility.core" Version="3.2.2" /> + <!-- Compono.XunitV3.SampleTests' own dependency - consumed as a real NuGet package from a local + feed populated by dotnet pack (PLAN-0004 Phase 3), never a ProjectReference. The version here + must match the -p:Version passed to that project's PackToLocalFeed pre-restore target. --> + <PackageVersion Include="Compono.XunitV3" Version="1.0.0" /> <!-- Mocking. No AutoFixture here, deliberately - Compono is a from-scratch AutoFixture replacement (docs/manifesto.md), so dogfooding AutoFixture inside Compono's own test suite would be a direct contradiction of the diff --git a/docs/adr/0022-compono-xunit-package-design.md b/docs/adr/0022-compono-xunit-package-design.md index 8de0f06..73d523f 100644 --- a/docs/adr/0022-compono-xunit-package-design.md +++ b/docs/adr/0022-compono-xunit-package-design.md @@ -1038,6 +1038,153 @@ its own design question, deferred - not designed here, and not assumed necessary until a concrete need materializes (this milestone's own non-goals already exclude speculative future-proofing). +## Amendment 5 (2026-07-31): a genuine composition failure's own `Message` must carry the seed too, not only `Diagnostic` + +PR #26 review (Codex, on PLAN-0004 Phase 3) caught that `test/Compono +.XunitV3.SampleTests`' deliberately-failing theory used `[Compose(Seed = +-1)]` - a negative, rejected seed - which exercises `Compono.XunitV3`'s own +seed-validation failure (a pre-composition check that always rejects `-1` +regardless of what it's paired with), not a genuine composition failure. +That meant `RealRunnerTests` never actually proved the milestone's own +Goal statement - "a composition failure's message contains a seed that, +pasted into `[Compose(Seed = ...)]`, reproduces the same failure" - for +the case that statement is actually about: a real, unsatisfied composition +request. + +Investigating why a genuine case wasn't used originally (Phase 3's own +`SeedReportingTests`, added under time pressure to close out that task +list item) surfaced the real gap: a *pipeline*-propagated +`CompositionException` (an ordinary composition failure - nothing could +satisfy the requested type) has never carried its seed in +`Exception.Message` itself, only in `Diagnostic`/`Diagnostic.ToString()` +(the `Console.WriteLine(exception.Diagnostic)` convention +`docs/architecture.md` documents, from Milestone 2/3, per +[ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md)). +A real xUnit v3/MTP test-runner failure display shows `Exception.Message`, +not `Diagnostic.ToString()` - so the pasteable-seed promise was invisible +in real console output for a genuine composition failure the entire time, +not just in this one sample theory. + +**Decision:** `ComposeAttribute.GetData` now catches a `CompositionException` +propagating from a composition call (`ResolveInvoker`/`ResolveSharedInvoker`/ +`ShareExplicitInvoker`, via a new private `InvokeWithSeedOnFailure` helper +wrapping each call site) and rethrows it via a new public `Compono` core +method, `CompositionException.WithSeedInMessage(original, seed)` - rewriting +`Message` to `$"{original.Message}\n\nSeed: {seed}"` while preserving +`Diagnostic` completely unchanged (so `Diagnostic.ToString()` still renders +correctly, without a duplicated `"Seed:"` line) and the original exception +as `InnerException`. **Originally shipped as an `internal` seam with a new +`InternalsVisibleTo("Compono.XunitV3")` grant on `Compono.csproj` - reverted +after PR #26 review's fourth round; see the amendment below.** + +**Why a new core method rather than a workaround entirely inside +`Compono.XunitV3`:** `CompositionException`'s existing constructors always +derive `Message` directly from `Diagnostic.Message` (or accept a plain +string with no `Diagnostic`/`InnerException` at all) - there's no existing +public shape that lets a caller supply a custom `Message` string alongside +an existing `Diagnostic` and `InnerException` together. +`Compono.XunitV3` reconstructing its own `CompositionDiagnostic` copy with +a seed-appended `Message` field was considered and rejected: `Diagnostic +.ToString()`'s own format independently appends `"\n\nSeed: {Seed}"`, so a +diagnostic whose `Message` field *also* had the seed appended would render +that line twice - a visible regression for the documented `Console +.WriteLine(exception.Diagnostic)` path. `WithSeedInMessage` avoids that +entirely by leaving `Diagnostic` untouched and only rewriting the outer +exception's own `Message`. + +**Consequence for `test/Compono.XunitV3.SampleTests`:** `Failing +CompositionTests` now composes `GatewayConsumer` (a concrete class whose +constructor takes `IUnregisteredGateway`, an interface satisfied by +nothing), with an explicit `Seed = 24601` so an assertion against its +output stays deterministic rather than needing to parse an auto-generated +seed out of console output. `IUnregisteredGateway` itself is deliberately +never used as the `[Compose]`-attributed parameter's own root type - that +would hit the CMP0003 diagnostic this plan's own Open Items section +already documents as deliberate (an abstract/interface/delegate root +always fails at compile time, regardless of whether a registration might +satisfy it at runtime) - wrapping it as a *nested* constructor parameter +uses the more lenient member-level check instead, so it compiles and fails +only at runtime, which is the failure shape this fixture actually needs. + +**Amendment to this amendment (PR #26 review, third round): the fix above +only covered a *diagnosed* `CompositionException`.** `InvokeWithSeedOnFailure`'s +original catch clause was guarded by `when (exception.Diagnostic is not +null)`, which let a plain-message `CompositionException` (no `Diagnostic` +at all) escape completely unmodified - exactly the shape a generated +`HashSet<T>`/`Dictionary` collection plan's unique-value-exhaustion path +throws (`Compono.Generators/Templates/CollectionPlan.scriban`: +`throw new CompositionException($"Could not generate {size} unique +values...")`, no `Diagnostic` constructed at all). Composing a +default-sized (3) `HashSet<bool>` - `bool`'s value space has only 2 +members - deterministically hits this path with zero seed anywhere in the +output. Fixed by broadening `CompositionException.WithSeedInMessage` +itself to build its rewritten `Message` from `original.Message` (not +`original.Diagnostic!.Message`) and to preserve `original.Diagnostic` +as-is, whatever it is - `null` stays `null`, a real diagnostic stays +unchanged - rather than requiring one. `InvokeWithSeedOnFailure`'s catch +clause no longer needs the `Diagnostic is not null` guard at all: every +`CompositionException`, diagnosed or not, now gets the same treatment +uniformly. + +**Amendment to this amendment (PR #26 review, fourth round): the +`InternalsVisibleTo` grant directly reversed ADR-0021's own accepted, +public-only integration boundary.** ADR-0021's Pros and Cons already has +an entry for exactly this option - +"Reaching the engine — `InternalsVisibleTo("Compono.Xunit")`" - rejected +specifically because "it directly reverses a deliberate, documented +policy... adopted specifically to keep a shipped package's internal +surface from becoming a de facto public contract for 'trusted' +consumers." Granting `Compono.XunitV3` `InternalsVisibleTo` for +`WithSeedInMessage` did exactly that: because `Compono` is unsigned, the +grant authenticates only the simple assembly name, handing any assembly +that can claim that name unrestricted access to every internal member of +`Compono` core - not scoped to the one method that needed it. + +**Decision:** the grant is removed. `WithSeedInMessage` itself moved from +`internal` to `public` instead - it already lives inside +`CompositionException`, so it already has full access to the private +constructor and `DiagnosingContextIdentity` it needs; the only reason it +required `InternalsVisibleTo` at all was its own access modifier, not +anything about where it's implemented. Making the one method public, +rather than granting blanket internal access, is the minimal-footprint +fix `docs/public-api.md`'s "keep public APIs minimal" goal actually calls +for: `Compono.XunitV3` gets exactly the operation it needs and nothing +else, `CompositionContext` and every other internal type stay exactly as +inaccessible to `Compono.XunitV3` as ADR-0021 intended, and any other +consumer building custom composition-failure tooling gets the same +utility for free. No `InternalsVisibleTo` entry for `Compono.XunitV3` +exists in `Compono.csproj` after this fix - the only remaining grant is +`Compono.Tests`, unchanged from before this PR. + +## Amendment 6 (2026-07-31): the automated real-runner test is dropped; the sample project stays, unautomated + +The `RealRunnerTests` test this amendment's own "Consequence" section +above referred to - which shelled out `dotnet test` against +`Compono.XunitV3.SampleTests` on every CI run - is removed. Across four +consecutive rounds of fixes (a cross-process file lock, a global-cache +clear, an isolated restore path, and finally a machine-wide named +`Mutex` fully serializing the nested subprocess), each verified +extensively via local reproduction of CI's exact concurrency at the time, +CI still failed on the *next* push in a new way tied specifically to this +repo's CI runner's process-concurrency characteristics for a +nested-`dotnet`-invoking test - never once on the actual composition +behavior the sample project's theories exercise, which passed every +single local and CI run across the whole sequence. + +**Decision:** stop iterating on CI-only concurrency once it was clear the +thing being verified (that `Compono.XunitV3` composes correctly as a real +consumed package, through a real xUnit v3 runner) was already +conclusively proven, repeatedly - keep `Compono.XunitV3.SampleTests` on +disk exactly as built (its representative theories, its +`PackToLocalFeed`/`RestorePackagesPath` local-feed infrastructure), not +referenced by `Compono.slnx` or any other test, available to run by hand +whenever packaging behavior needs re-checking by a person. This mirrors +Phase 1's own precedent for this exact milestone - a one-time manual +`dotnet pack` + local feed + throwaway-consumer verification, not +permanent CI automation - rather than introducing a new philosophy. See +[PLAN-0004](../plans/0004-milestone-4-xunit-integration.md)'s Phase 3 +Notes for the full blow-by-blow of all four fix attempts. + ## Links - [ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md) — diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index fb3e27e..b6d4580 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -316,15 +316,15 @@ the cache built once in Phase 1; everything after is per-row): ### Phase 3: Test suites and verification (ADR-0022's Testing Strategy) -**Status:** Not Started +**Status:** Done -- [ ] `test/Compono.XunitV3.Tests`: binding-algorithm unit tests +- [x] `test/Compono.XunitV3.Tests`: binding-algorithm unit tests (inline-only/composed-only/mixed/too-many/non-null-type-mismatch), `[Shared]` detection (duplicate types, before/after ordering), profile caching/reuse assertion, unsupported-signature detection, seed determinism, concurrency-stress test on a shared cached attribute instance. -- [ ] Inline `null` handling, all four combinations: accepted for a +- [x] Inline `null` handling, all four combinations: accepted for a nullable reference-typed parameter and for a `Nullable<T>` parameter; rejected (clear pre-composition exception, no `NullReferenceException` from an attempted `GetType()` on `null`) @@ -332,22 +332,22 @@ the cache built once in Phase 1; everything after is per-row): value-typed parameter — each combination covered for both an ordinary parameter and an inline-`[Shared]` parameter, asserting rejection happens before `ShareExplicit`'s invoker is ever reached. -- [ ] A **non-null** inline value targeting a `Nullable<T>` parameter is +- [x] A **non-null** inline value targeting a `Nullable<T>` parameter is **accepted** (e.g. `[Compose(42)]` for an `int?` parameter) — the regression test for the boxed-`T`-vs-boxed-`Nullable<T>` bug the `Nullable.GetUnderlyingType` unwrap fixes; without the unwrap this case fails even though nullable parameters are fully supported. -- [ ] Every returned `ITheoryDataRow` carries a `"Compono.Seed"` trait +- [x] Every returned `ITheoryDataRow` carries a `"Compono.Seed"` trait matching `row.Seed` exactly, for both a passing-shaped row and a deliberately-failing one — proving the trait is unconditional, not only attached when composition is about to fail. -- [ ] Cached invoker delegates: `MakeGenericMethod` construction runs +- [x] Cached invoker delegates: `MakeGenericMethod` construction runs exactly once per parameter across many repeated `GetData` calls on one attribute instance (not once per row); a value-typed and a reference-typed parameter both compose correctly through their cached delegate (proving the `T → object` boxing inside the closed helper works for both). -- [ ] `[Compose(Seed = <negative>)]` rejected with a clear pre-composition +- [x] `[Compose(Seed = <negative>)]` rejected with a clear pre-composition exception naming the rejected value, distinct from every other signature-validation failure; a non-negative configured `Seed` round-trips unchanged through `row.Seed`. Combined with Phase 0's @@ -356,13 +356,15 @@ the cache built once in Phase 1; everything after is per-row): exactly the `int` value from `row.Seed` — not merely `"Seed:"` — for both an auto-generated seed and an explicit non-negative one, proving the pasteable-seed promise rather than just its presence. -- [ ] An API-surface/approval test (`Compono.XunitV3.Tests`, e.g. `Verify` + See Notes below for where that seed actually lives for a + pipeline-propagated (not `Compono.XunitV3`-authored) failure. +- [x] An API-surface/approval test (`Compono.XunitV3.Tests`, e.g. `Verify` over `typeof(ComposeAttribute).Assembly`'s public types) locking the public shape of `Compono.XunitV3` — `ComposeAttribute`, `ComposeAttribute<TProfile>`, `SharedAttribute` and nothing else — cheap insurance against accidental API drift, matching this milestone's own "keep public APIs minimal" constraint. -- [ ] `test/Compono.XunitV3.SampleTests`: real xUnit v3 project with +- [x] `test/Compono.XunitV3.SampleTests`: real xUnit v3 project with representative theories (inline-only, composed-only, mixed, `[Shared]`-before-SUT, deliberately-failing composition, `async Task` theory). References `Compono.XunitV3` via `PackageReference` @@ -371,8 +373,9 @@ the cache built once in Phase 1; everything after is per-row): `Compono.XunitV3` exactly the way an external consumer would, catching packaging mistakes (missing dependency, wrong `PrivateAssets`, the generator analyzer not flowing transitively) - that a `ProjectReference` build can't surface. -- [ ] **The sample project must include one theory whose parameter type + that a `ProjectReference` build can't surface. See Notes below - this + is exactly what this project caught. +- [x] **The sample project must include one theory whose parameter type is discovered *only* from a `[Compose]`-attributed method** — no `[Composable]`, no `Create<T>()`/`CreateMany<T>()`, no direct `CompositionRow` call site anywhere else in the sample — proving @@ -380,11 +383,20 @@ the cache built once in Phase 1; everything after is per-row): fix #2) actually generates a plan through the real packaged pipeline, not just in an isolated `Compono.Generators.Tests` snapshot test. -- [ ] A `Compono.XunitV3.Tests` test that runs the sample project through a - real xUnit v3 runner (`dotnet test` or the in-process console - runner) and asserts on its result — the milestone's required +- [x] The sample project's theories were run through a real xUnit v3 + runner (`dotnet test`) and their output verified by hand, repeatedly, + during this phase's own development — the milestone's required "proves behavior through the real xUnit v3 discovery and execution - pipeline" coverage. + pipeline" coverage, satisfied the same way Phase 1 satisfied its own + packaged-consumer proof (a manual, one-time check, not permanent CI + automation). An automated `Compono.XunitV3.Tests` test + (`RealRunnerTests`) that shelled out `dotnet test` against the sample + project on every run was built and iterated on at length, but + dropped after repeated CI-only concurrency failures across several + serialization fixes that each passed extensive local reproduction — + see Notes below for the full account and the reasoning for keeping + the sample project itself (proven, and available for future manual + use) while not re-running it automatically on every commit. ### Phase 4: Docs and cleanup @@ -724,6 +736,306 @@ exercised indirectly through `Compono.XunitV3.Tests`. `Compono.XunitV3.SampleTests` project) remains Phase 3's own scope per the plan's phase split and ADR-0022's Testing Strategy. +**Phase 3 (Done):** + +- **A real packaging bug, caught exactly the way this phase's real-packaged- + consumer project exists to catch it**: `Compono.XunitV3.csproj`'s + `<ProjectReference Include="..\Compono\Compono.csproj" />` had no explicit + asset-flow metadata, so `dotnet pack` applied the default `PrivateAssets` + (`contentfiles;build;analyzers`) to the generated nuspec `<dependency>` for + `Compono` - rendered as `exclude="Build,Analyzers"`. A project referencing + only the `Compono.XunitV3` package (never `Compono` directly - the whole + point of this package) never got `Compono.Generators` transitively, so + every `[Compose]`-attributed parameter type silently fell back to "no + generated plan" and failed to compose at runtime with no compile-time + signal at all. First surfaced as `Compono.XunitV3.SampleTests`' + `SharedTests.SharedRepositoryIsReusedByTheService` failing with "No + registration, configuration rule, semantic provider, test-double provider, + built-in provider, or generated plan could satisfy 'Repository'" despite + `Repository` being reachable only through a `[Compose]`-attributed method's + own parameter (exactly `ComposeMethodDiscovery`'s discovery path). Fixed by + adding `PrivateAssets="none"` to that `ProjectReference` - confirmed via + the packed nuspec changing from `exclude="Build,Analyzers"` to + `include="All"`, and via `-p:EmitCompilerGeneratedFiles=true` showing + `Compono.Generators` actually emitting `CompositionPlan.g.cs` files for + `Repository`/`OrderService`/`CreateOrder` inside + `Compono.XunitV3.SampleTests`' own `obj/` once the fix landed. This is + precisely the "packaging mistake a `ProjectReference` build can't surface" + scenario this phase's testing strategy names - it would have shipped + silently broken without a project that consumes `Compono.XunitV3` as a + real package. +- **A same-version local NuGet feed needs its global-packages-folder cache + cleared to pick up a re-`pack`ed nupkg** - both `Compono` and + `Compono.XunitV3` are packed with a fixed `-p:Version=1.0.0` (this repo has + no versioning tool wired up yet), and NuGet treats an already-cached + version as immutable, so a content change (like the `PrivateAssets` fix + above) silently keeps resolving the *stale* cached package unless + `~/.nuget/packages/compono`/`~/.nuget/packages/compono.xunitv3` are cleared + first. Not a problem for `Compono.XunitV3.SampleTests`' own + `PackToLocalFeed` pre-restore target in normal use (CI and a fresh + checkout's global packages folder never have a stale entry to begin with), + but worth recording here since it cost real time to diagnose during this + phase's own manual verification. +- **The seed-message-content proof needed two different assertion shapes, + not one** (Phase 3's own task list originally implied a single pattern) - + `Compono.XunitV3`'s own pre-composition exceptions (negative seed, + signature errors, inline-value validation) append a `"Seed: <value>"` line + directly into `CompositionException.Message` via the `AppendSeed` helper, + but a *pipeline*-propagated `CompositionException` (an ordinary + composition failure, e.g. an unregistered type) does not - its `.Message` + is the plain diagnostic text (`CompositionDiagnostic.Message`), and the + seed only appears in `exception.Diagnostic.Seed`/`exception.Diagnostic + .ToString()` (`docs/architecture.md`'s existing `Console.WriteLine + (exception.Diagnostic)` convention, from Milestone 2/3 - out of this + phase's scope to change). `SeedReportingTests` covers both: the explicit- + seed case asserts `exception.Diagnostic!.Seed` directly; the auto- + generated-seed case parses the seed out of `exception.Diagnostic! + .ToString()`'s `"Seed: <value>"` line and pastes it into a second + `[Compose(Seed = ...)]` call, asserting the same seed reappears - proving + the pasteable-seed promise round-trips through a real `Compose(Seed = + ...)]` value, not just a string match. **Superseded by ADR-0022 Amendment + 5**, below: PR #26 review correctly pointed out this whole distinction + was a workaround, not a fix - `Compono.XunitV3.SampleTests`' original + deliberately-failing theory used `[Compose(Seed = -1)]` specifically to + route around the fact that a genuine pipeline failure's `.Message` had no + seed in it at all. Amendment 5 fixes that gap directly (a genuine + composition failure's `.Message` now carries the seed too), so this + two-assertion-shape distinction no longer describes current behavior - + kept here only as the historical reasoning for why the original + implementation was shaped this way. +- **`test/Compono.XunitV3.SampleTests` is deliberately not added to + `Compono.slnx`** - it contains one theory that always fails + (`FailingCompositionTests`, by design, per ADR-0022's Testing Strategy), + and CI's PR-build workflow runs `dotnet test` across every project in + `Compono.slnx` expecting a clean pass. Adding it there would break every + PR. It stays a genuinely standalone project on disk (see the automation + note below for how it's actually exercised - or rather, not automatically + exercised - going forward). +- **PR #26 review (second round) caught two more real gaps, both fixed in + the same PR** - see ADR-0022 Amendment 5 for the first in full: + 1. The sample project's failing theory originally used + `[Compose(Seed = -1)]`, which exercises `Compono.XunitV3`'s own + seed-validation failure, not a genuine composition failure - the + milestone's actual Goal statement is about the latter. Fixed by (a) + adding `CompositionException.WithSeedInMessage` as a new `Compono` + core method so `ComposeAttribute.GetData` now rewrites a propagating + pipeline failure's own `Message` to include the seed - previously only + `Diagnostic` had it, and a real test runner's failure display shows + `Message`, not `Diagnostic.ToString()`; (b) switching the sample + project's failing theory to a genuine unsatisfied composition + (`GatewayConsumer`, whose constructor takes `IUnregisteredGateway` - + deliberately a *nested* dependency, not the `[Compose]`-attributed + parameter's own root type, since an abstract root there hits the + CMP0003 diagnostic this plan's Open Items section already documents as + deliberate) with an explicit `Seed = 24601`. `SeedReportingTests` + (`Compono.XunitV3.Tests`, no dependency on the sample project) was + rewritten to assert on `.Message` directly (not `.Diagnostic`) for + exactly this reason, plus a new test proving `Diagnostic` itself is + left unchanged (no duplicated `"Seed:"` line) alongside the rewritten + `Message`. `Compono.Tests` gained direct coverage of + `CompositionException.WithSeedInMessage` itself, per `testing.md`'s + "verifying a new public entry point" rule. This fix is real, + independently tested, and kept - though its *access mechanism* + changed again in round 5 below, after first shipping as `internal` + plus `InternalsVisibleTo`. + 2. `PackToLocalFeed`'s fixed `-p:Version=1.0.0` meant a stale entry in + NuGet's global packages folder from an earlier run could silently + satisfy a later restore even after the local feed's `.nupkg` contents + changed. Also fixed and kept - see the "sample project kept, automation + dropped" note below for why this stopped mattering for CI specifically, + even though the fix (an isolated per-project `RestorePackagesPath`, + `obj/.nuget-packages/`) is still in the sample project's `.csproj` for + anyone using it manually. + 3. A third round found `InvokeWithSeedOnFailure`'s + `when (exception.Diagnostic is not null)` guard let a plain-message + `CompositionException` (no `Diagnostic` at all) escape with no seed + whatsoever - 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` (not `original.Diagnostic!.Message`) and preserve + `original.Diagnostic` as-is whatever it is (`null` stays `null`) - + removing the need for the guard entirely, so every + `CompositionException` gets the same treatment. Covered by a new + `Compono.Tests` test (`WithSeedInMessage_RewritesMessage_AndLeaves + DiagnosticNull_ForAPlainMessageOriginal`) and a new + `Compono.XunitV3.Tests` test using a hand-written + `CollectionExhaustionPlan` fixture that mirrors `CollectionPlan + .scriban`'s own `HashSet<T>` shape exactly (composing a default-sized + `HashSet<bool>` - `bool`'s value space has only 2 members against a + default collection size of 3 - deterministically exhausts). + 4. The same review round also caught that `ComposeAttributeConcurrencyTests` + never actually exercised concurrent execution at all: + `Enumerable.Range(0, 200).Select(_ => attribute.GetData(...).AsTask())` + is lazy, and since `GetData` runs fully synchronously (an + already-completed `ValueTask`), `Task.WhenAll` enumerating that + deferred sequence never creates call N+1 until call N has already + finished - zero overlap, so the test could never have caught a race + in the shared `Lazy<Composer>`/binding-plan initialization it exists + to test. Fixed by rewriting it with `Parallel.ForEachAsync`, which + genuinely dispatches across the thread pool concurrently - verified + stable across 5 repeated local runs post-fix. + 5. A fourth review round found two P2 gaps: `PublicApiSurfaceTests` + checked only `type.IsPublic`, which reflection never sets for a + nested type (`IsNestedPublic` is the separate flag), so an + accidentally-added nested public type would have slipped through + undetected - fixed by checking both. Separately, `GetData`'s XML 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 → the provider's own thrown + exception. Doc corrected to describe the actual chain. + 6. **A fifth review round caught something serious: the `internal` + `WithSeedInMessage` seam from round 2, reached via a new + `InternalsVisibleTo("Compono.XunitV3")` grant on `Compono.csproj`, + directly reversed [ADR-0021](../adr/0021-row-composition-entry-point-for-test-framework-integrations.md)'s + own accepted, public-only integration boundary** - that ADR's own + Pros and Cons section has an entry for exactly this option, rejected + specifically because "it directly reverses a deliberate, documented + policy... adopted specifically to keep a shipped package's internal + surface from becoming a de facto public contract for 'trusted' + consumers." Because `Compono` is unsigned, the grant authenticated + only the assembly name - handing any assembly that could claim that + name unrestricted access to every internal member of `Compono` core, + not scoped to the one method that needed it. This should have been + caught before implementation (a quick check against ADR-0021 before + adding any `InternalsVisibleTo` entry would have surfaced the direct + conflict immediately) rather than by a later review round. **Fixed by + removing the grant entirely and making `WithSeedInMessage` itself + `public` instead of `internal`** - it already lives inside + `CompositionException`, so it already had full access to the private + constructor and `DiagnosingContextIdentity` it needs; the only reason + it ever required `InternalsVisibleTo` was its own access modifier, not + anything about where it's implemented. `Compono.XunitV3` now gets + exactly the one operation it needs via an ordinary public API call, + `CompositionContext` and every other internal type stay exactly as + inaccessible as ADR-0021 intended, and the only `InternalsVisibleTo` + entry remaining in `Compono.csproj` is `Compono.Tests`, unchanged from + before this PR. See ADR-0022 Amendment 5's own "amendment to this + amendment" for the full account. + 7. A sixth review round (P2) caught that `pack-to-local-feed.sh`'s + `until mkdir "$lock_dir"; do sleep 1; done` wait loop had no bound - + if a prior holder were killed before its own `EXIT` trap could run + (`SIGKILL`, a canceled build, a machine restart), the lock directory + is never cleaned up, and every later restore of this project would + wait on it forever with no way to recover short of knowing to delete + it by hand. Fixed with a bounded wait (~120 attempts, matching the + ~1 pack/second polling interval already in place) that fails with a + clear, actionable message (naming the exact `rm -rf` to run) instead + of hanging indefinitely. Verified by manually creating a stale lock + directory and confirming the script now fails fast with that message + (using a temporarily-reduced attempt count for the test) rather than + blocking forever, then confirming a normal run still succeeds once + the stale lock is gone. + 8. A seventh review round (P2) caught that `CompositionException + .WithSeedInMessage` - a genuinely new public `Compono` core API as + of round 5 - was documented only in ADR-0022, with its + `docs/public-api.md` coverage deferred to Phase 4's docs pass. Per + this repo's own rule (`AGENTS.md`: update the relevant `docs/*.md` in + the same PR that changes the behavior it describes, not a + follow-up), added a `Diagnostics API` section entry showing the + method's signature, its exception-chain contract (`Diagnostic` + copied through unchanged, `original` becomes `InnerException`), and + when to reach for it (a plain-message `CompositionException` has no + `Diagnostic` to render its own `Seed:` line). + - Also folded into this round, at the user's request (not a posted PR + comment): the sample project had no `[Compose<TProfile>]` theory at + all - every existing theory used the profile-less `[Compose]` form. + Added `ProfileTests.ComposesTheProfileConfiguredValue`, using a new + `ApplicationTestProfile` that registers a fixed `NotificationSettings` + value, proving `TProfile.Configure` actually applies through the real + packaged pipeline (not just the in-process `GetComposer()`/`CreateRow` + checks `Compono.XunitV3.Tests` already had). The sample project's + theory count is 7 as a result (inline-only, composed-only, mixed, + async, `[Shared]`-before-SUT, profile-based, one deliberately failing). +- **Sample project kept; the automated real-runner test was dropped after + four rounds of CI-only concurrency failures** - the fullest account of + why lives here since it's the reason the milestone's "proves behavior + through a real xUnit v3 runner" criterion ends up satisfied by manual + verification (matching Phase 1's own precedent) rather than permanent CI + automation: + - An automated `Compono.XunitV3.Tests` test, `RealRunnerTests`, shelled + out `dotnet test` against the sample project on every run and asserted + on its output. This is what actually caught the `PrivateAssets` + packaging bug above and forced the genuine-composition-failure fix in + Amendment 5 - genuinely valuable while it was running. + - CI's `dotnet test --solution Compono.slnx` runs every test host for + every TFM concurrently, so `Compono.XunitV3.Tests`' net10.0 and net11.0 + hosts each ran `RealRunnerTests` at the same moment, each independently + spawning its own nested `dotnet test` against the sample project. Four + consecutive rounds of fixes were made in response to CI failures, each + verified locally at the time, each still failing on the *next* CI + push: + 1. A `pack-to-local-feed.sh` cross-process `mkdir` lock, for two + nested `dotnet pack` calls racing on shared `src/Compono`/ + `src/Compono.Generators` bin/obj output + (`Could not find a part of the path '.../Compono.Generators/bin + /Debug/netstandard2.0'`). + 2. A `pack-to-local-feed.sh` global-NuGet-cache clear for the stale + version problem above, which introduced a *new*, different race + (a second process's clear deleting files a first process's own + restore was still reading - `Could not find a part of the path + '.../packages/compono/1.0.0'`). + 3. An isolated per-invocation `RestorePackagesPath` (first scoped by + `$(TargetFramework)`, which turned out not to distinguish the two + concurrent invocations at all since `RealRunnerTests.cs` hardcoded + `"-f net10.0"` regardless of which TFM host was calling it; then + correctly scoped by a `Compono_LocalPackagesId` environment + variable set once in C# per nested subprocess) - this one *worked* + for the restore-path race specifically (CI's own log confirmed two + genuinely different isolated paths) but CI still hit the *original* + shared-bin/obj race from round 1, meaning the `mkdir` lock wasn't + sufficient on CI's runner even though a dozen-plus local + concurrency stress-test attempts under the exact CI sequence never + reproduced that recurrence. + 4. A machine-wide named `System.Threading.Mutex` in `RealRunnerTests.cs` + wrapping the entire nested subprocess (not just the pack step) - + the most aggressive fix attempted, fully serializing every nested + invocation regardless of what either side did internally. This one + passed eight consecutive local stress-test runs (after also fixing + an `AbandonedMutexException` the Mutex itself introduced) with + timings that, for the first time, showed clear evidence of actual + serialization (~8s vs ~15-16s, rather than both sides finishing in + a similar ~8-9s with no proof anything was serialized). This is + also the point the decision below was made, before a fifth CI push + could confirm or refute it. + - **Decision: stop iterating on CI-only concurrency and drop the + automation, keeping the sample project itself.** The thing + `RealRunnerTests` existed to prove - that `Compono.XunitV3` composes + correctly when consumed as a real package, through a real xUnit v3 + runner - was proven repeatedly, by hand and via CI, across this whole + round of fixes; every one of those runs' *test content* (the 5-then-6 + passing theories, the one deliberately-failing one) succeeded every + single time. Every failure across all four rounds was exclusively + about *this repo's own CI environment's* process-concurrency + characteristics for a nested nested-`dotnet`-invoking test, which + resisted full local reproduction even under carefully matched + conditions - a cost with no additional verification payoff once the + underlying behavior was already this thoroughly demonstrated. Removed + `RealRunnerTests.cs` entirely; the sample project's files, its + `PackToLocalFeed`/`RestorePackagesPath`/`pack-to-local-feed.sh` + infrastructure, and its fixed content (the genuine composition + failure, the profile theory) all stay on disk, unreferenced by + `Compono.slnx` or any other test, available to run by hand + (`dotnet test` from `test/Compono.XunitV3.SampleTests`) whenever + packaging behavior needs re-checking by a person, without being + re-run - and re-racing - on every commit. +- Full suite green: `Compono.Tests` 392/392 (196 × 2 TFMs - 384 unaffected + + 2 new `CompositionException.WithSeedInMessage` tests, each × 2 TFMs - + one of which was rewritten in place during the third review round, net + count unchanged), `Compono.Generators.Tests` 166/166 (unchanged), + `Compono.XunitV3.Tests` 94/94 (47 × 2 TFMs - 46 from the second review + round (unchanged from the first round's count, since `RealRunnerTests` + was added and then removed within this same PR) + 1 new plain-message + `WithSeedInMessage` test from the third round; + `ComposeAttributeConcurrencyTests` rewritten in place to genuinely + exercise concurrency, same test count). + `Compono.XunitV3.SampleTests` itself last verified by hand reporting + 6 passed/1 deliberately failed (a genuine composition failure) on both + net10.0 and net11.0. + ## Open Items - **`ComposeMethodDiscovery` reports CMP0003 for an interface/abstract/ diff --git a/docs/public-api.md b/docs/public-api.md index c7ffd52..dad363c 100644 --- a/docs/public-api.md +++ b/docs/public-api.md @@ -447,6 +447,33 @@ catch (CompositionException exception) } ``` +`exception.Diagnostic` (when present) already renders its own `Seed: {value}` +line via `ToString()`, but not every `CompositionException` has a +`Diagnostic` - a plain-message one (e.g. a generated `HashSet<T>`/ +`Dictionary` collection plan's unique-value-exhaustion failure) does not. +`CompositionException.WithSeedInMessage(original, seed)` is a static +factory for exactly this case - it returns a copy of `original` whose +`Message` has the seed appended directly, regardless of whether +`Diagnostic` is present: + +```csharp +try +{ + composer.Create<Order>(); +} +catch (CompositionException exception) +{ + throw CompositionException.WithSeedInMessage(exception, mySeed); +} +``` + +The returned exception's `Diagnostic` is copied through from `original` +unchanged (`null` stays `null`), and `original` itself becomes its +`InnerException` - never discarded. `Compono.XunitV3`'s own `[Compose]` +binding algorithm (ADR-0022) uses this to guarantee every composition +failure's `Message` carries a pasteable seed, not only ones that happen to +have a `Diagnostic`. + Potential debugging API: ```csharp diff --git a/src/Compono.XunitV3/Compono.XunitV3.csproj b/src/Compono.XunitV3/Compono.XunitV3.csproj index 4179fbf..699e03b 100644 --- a/src/Compono.XunitV3/Compono.XunitV3.csproj +++ b/src/Compono.XunitV3/Compono.XunitV3.csproj @@ -9,7 +9,18 @@ </PropertyGroup> <ItemGroup> - <ProjectReference Include="..\Compono\Compono.csproj" /> + <!-- PrivateAssets="none" (PLAN-0004 Phase 3 real-packaged-consumer verification) - an ordinary + PackageReference/ProjectReference defaults PrivateAssets to "contentfiles;build;analyzers", + meaning those asset kinds stop at this package and never flow to a consumer of + Compono.XunitV3 - dotnet pack renders that default as exclude="Build,Analyzers" on the + generated <dependency> for Compono in this package's own nuspec. Left at its default, a + project that references only the Compono.XunitV3 package (never Compono directly - the + whole point of this package existing) never gets Compono.Generators transitively, and every + [Compose]-attributed type silently falls back to "no generated plan" - a real regression a + ProjectReference-based build can't surface. Caught by + test/Compono.XunitV3.SampleTests failing to compose a type reached only via a + [Compose]-attributed method until this was added. --> + <ProjectReference Include="..\Compono\Compono.csproj" PrivateAssets="none" /> <!-- Ordinary reference, not PrivateAssets="all" - ComposeAttribute/ComposeAttribute{TProfile} derive from and return types this package owns publicly, so a consumer needs them transitively assignable. See ADR-0022's Package Boundaries and Dependencies section. --> diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 18347cf..34360da 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -120,8 +120,14 @@ public int Seed /// parameter, more than one Compose-family attribute, or more than one <c>[Shared]</c> parameter /// of the same type); too many inline values were supplied; a supplied inline value is /// <see langword="null"/> for a non-nullable parameter or has a type not assignable to its - /// parameter; or composition itself fails for a parameter (propagated un-wrapped from the - /// pipeline). + /// parameter; or composition itself fails for a parameter - a new <see cref="CompositionException"/> + /// propagates whose <see cref="Exception.Message"/> is the pipeline's original message with the + /// row's seed appended and whose <see cref="CompositionException.Diagnostic"/> is copied through + /// from that original unchanged (<see langword="null"/> if the original had none). This new + /// exception's own <see cref="Exception.InnerException"/> is the pipeline's original + /// <see cref="CompositionException"/> itself, not that original's <see cref="Exception.InnerException"/> + /// - so a provider failure's chain is wrapper → original composition exception → the provider's + /// own thrown exception, one level deeper than the original throw. /// </exception> /// <remarks> /// <paramref name="disposalTracker"/> is deliberately never used to register a composed value. @@ -224,12 +230,12 @@ public override ValueTask<IReadOnlyCollection<ITheoryDataRow>> GetData(MethodInf if (i < _inlineValues.Length) { var value = _inlineValues[i]; - parameter.ShareExplicitInvoker(row, parameter.Descriptor, value); + InvokeWithSeedOnFailure(() => parameter.ShareExplicitInvoker(row, parameter.Descriptor, value), row.Seed); values[i] = value; } else { - values[i] = parameter.ResolveSharedInvoker(row, parameter.Descriptor); + values[i] = InvokeWithSeedOnFailure(() => parameter.ResolveSharedInvoker(row, parameter.Descriptor), row.Seed); } } @@ -243,7 +249,7 @@ public override ValueTask<IReadOnlyCollection<ITheoryDataRow>> GetData(MethodInf values[i] = i < _inlineValues.Length ? _inlineValues[i] - : parameter.ResolveInvoker(row, parameter.Descriptor); + : InvokeWithSeedOnFailure(() => parameter.ResolveInvoker(row, parameter.Descriptor), row.Seed); } // Assembled in method declaration order - binding order (shared first, then the rest) and @@ -291,4 +297,39 @@ private Composer BuildComposer() => Composer.Create(builder => // (ADR-0022's Seed Policy and Reporting) - so every failure category ends the same way, whether // Compono.XunitV3 constructed the message or the pipeline did. private static string AppendSeed(string message, int seed) => $"{message}\n\nSeed: {seed}"; + + // A genuine composition failure (PR #26 review; ADR-0022 Amendment 5) propagates un-wrapped from + // the pipeline otherwise, and CompositionException.Message alone never carries the seed for that + // case (only exception.Diagnostic does, via its own ToString(), when Diagnostic is even present - + // a plain-message CompositionException, e.g. a generated HashSet<T>/Dictionary collection plan's + // unique-value-exhaustion path, has no Diagnostic at all and would otherwise escape with no seed + // whatsoever - PR #26 review, third round) - a real test runner's failure display shows Message, + // not Diagnostic.ToString(), so the pasteable-seed promise wasn't actually visible there. + // CompositionException.WithSeedInMessage rewrites Message to include it, handling both shapes + // uniformly, while preserving Diagnostic (null or not) and the original exception as + // InnerException unchanged - so every CompositionException is caught here, not just diagnosed + // ones. + private static TResult InvokeWithSeedOnFailure<TResult>(Func<TResult> compose, int seed) + { + try + { + return compose(); + } + catch (CompositionException exception) + { + throw CompositionException.WithSeedInMessage(exception, seed); + } + } + + private static void InvokeWithSeedOnFailure(Action compose, int seed) + { + try + { + compose(); + } + catch (CompositionException exception) + { + throw CompositionException.WithSeedInMessage(exception, seed); + } + } } diff --git a/src/Compono/CompositionException.cs b/src/Compono/CompositionException.cs index cd70f18..c9a6aa5 100644 --- a/src/Compono/CompositionException.cs +++ b/src/Compono/CompositionException.cs @@ -21,18 +21,6 @@ public sealed class CompositionException : Exception /// </summary> public CompositionDiagnostic? Diagnostic { get; } - // Identifies which CompositionContext instance's own BuildException produced this exception - an - // opaque identity token (CompositionContext.Identity), not the context itself (PR #17 review): - // storing the actual CompositionContext would keep its whole object graph alive for as long as a - // consumer or test runner retains this exception - its configured IServiceProvider, registration - // factory closures, scope-held shared values, and trace buffer, none of which this comparison - // needs. A bare bool couldn't distinguish "some context produced this" from "this context's active - // nested resolution produced it" either (a registration factory can capture and invoke an entirely - // different Composer and let that composer's own pipeline-diagnosed exception escape, or rethrow - // one it captured earlier) - InvokeFactory's catch guard (docs/adr/0010) needs identity, just not - // by holding the whole context to get it. - internal object? DiagnosingContextIdentity { get; private init; } - /// <summary> /// Creates a <see cref="CompositionException"/> with no structured <see cref="Diagnostic"/>. /// </summary> @@ -73,6 +61,56 @@ public CompositionException(CompositionDiagnostic diagnostic, Exception innerExc Diagnostic = diagnostic; } + /// <summary> + /// Creates a copy of <paramref name="original"/> whose <see cref="Exception.Message"/> has a + /// <c>"Seed: <value>"</c> line appended, so a consumer building custom composition-failure + /// tooling (e.g. a test-framework integration reporting a reproducible seed) can surface it + /// without needing <see cref="Diagnostic"/> to be present - <see cref="Diagnostic"/> already + /// renders its own <c>"Seed:"</c> line via <see cref="CompositionDiagnostic.ToString"/> when it's + /// there, but not every <see cref="CompositionException"/> has one (a plain + /// <see cref="CompositionException(string)"/>, e.g. a generated collection plan's + /// unique-value-exhaustion failure, has none). + /// </summary> + /// <param name="original">The exception to copy and append the seed to.</param> + /// <param name="seed">The seed value to append.</param> + /// <returns> + /// A new <see cref="CompositionException"/> whose <see cref="Diagnostic"/> is + /// <paramref name="original"/>'s, unchanged (<see langword="null"/> stays <see langword="null"/>), + /// and whose <see cref="Exception.InnerException"/> is <paramref name="original"/> itself - so a + /// provider failure's full chain becomes this new exception, then <paramref name="original"/>, + /// then whatever <paramref name="original"/>'s own <see cref="Exception.InnerException"/> was (if + /// any), one level deeper than the original throw. + /// </returns> + /// <exception cref="ArgumentNullException"><paramref name="original"/> is <see langword="null"/>.</exception> + public static CompositionException WithSeedInMessage(CompositionException original, int seed) + { + ArgumentNullException.ThrowIfNull(original); + + var message = $"{original.Message}\n\nSeed: {seed}"; + return new CompositionException(message, original.Diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; + } + + // Identifies which CompositionContext instance's own BuildException produced this exception - an + // opaque identity token (CompositionContext.Identity), not the context itself (PR #17 review): + // storing the actual CompositionContext would keep its whole object graph alive for as long as a + // consumer or test runner retains this exception - its configured IServiceProvider, registration + // factory closures, scope-held shared values, and trace buffer, none of which this comparison + // needs. A bare bool couldn't distinguish "some context produced this" from "this context's active + // nested resolution produced it" either (a registration factory can capture and invoke an entirely + // different Composer and let that composer's own pipeline-diagnosed exception escape, or rethrow + // one it captured earlier) - InvokeFactory's catch guard (docs/adr/0010) needs identity, just not + // by holding the whole context to get it. + internal object? DiagnosingContextIdentity { get; private init; } + + // The only construction path that sets DiagnosingContextIdentity - used exclusively by + // CompositionContext.BuildException (passing its own Identity token), never by a factory/rule that + // constructs a CompositionException itself. + internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, object diagnosingContextIdentity) => + new(diagnostic) { DiagnosingContextIdentity = diagnosingContextIdentity }; + + internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, Exception innerException, object diagnosingContextIdentity) => + new(diagnostic, innerException) { DiagnosingContextIdentity = diagnosingContextIdentity }; + // The base(...) initializer runs before this constructor's own body, so a guard clause in the // body would already be too late - diagnostic.Message is dereferenced in the initializer itself. // Routing the null check through this helper is what lets a null argument surface as @@ -89,12 +127,9 @@ private static Exception RequireInnerException(Exception innerException) return innerException; } - // The only construction path that sets DiagnosingContextIdentity - used exclusively by - // CompositionContext.BuildException (passing its own Identity token), never by a factory/rule that - // constructs a CompositionException itself. - internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, object diagnosingContextIdentity) => - new(diagnostic) { DiagnosingContextIdentity = diagnosingContextIdentity }; - - internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, Exception innerException, object diagnosingContextIdentity) => - new(diagnostic, innerException) { DiagnosingContextIdentity = diagnosingContextIdentity }; + private CompositionException(string message, CompositionDiagnostic? diagnostic, Exception innerException) + : base(message, innerException) + { + Diagnostic = diagnostic; + } } diff --git a/test/Compono.Tests/CompositionExceptionTests.cs b/test/Compono.Tests/CompositionExceptionTests.cs index 90db5f2..be379cb 100644 --- a/test/Compono.Tests/CompositionExceptionTests.cs +++ b/test/Compono.Tests/CompositionExceptionTests.cs @@ -16,6 +16,47 @@ public void Constructor_ThrowsArgumentNullException_WhenDiagnosticIsNull() act.Should().Throw<ArgumentNullException>(); } + [Fact] + public void WithSeedInMessage_RewritesMessage_ButPreservesDiagnosticAndSetsInnerException() + { + // Internal seam for Compono.XunitV3 (PR #26 review; ADR-0022 Amendment 5) - a pipeline-thrown + // CompositionException's own Message never carried the seed on its own (only Diagnostic did, + // via its own ToString()), so Compono.XunitV3's GetData rewrites it before propagating. + var diagnostic = new CompositionDiagnostic + { + RootType = typeof(CompositionExceptionTests), + FailedType = typeof(CompositionExceptionTests), + Path = "unrelated-path", + Trace = [], + Seed = 0, + Message = "original pipeline failure message", + }; + var original = CompositionException.CreatePipelineDiagnosed(diagnostic, diagnosingContextIdentity: new object()); + + var wrapped = CompositionException.WithSeedInMessage(original, seed: 492173); + + wrapped.Message.Should().Be("original pipeline failure message\n\nSeed: 492173"); + wrapped.Diagnostic.Should().BeSameAs(diagnostic); + wrapped.Diagnostic!.Message.Should().Be("original pipeline failure message", "Diagnostic's own Message is left untouched - only the outer exception's Message is rewritten"); + wrapped.InnerException.Should().BeSameAs(original); + } + + [Fact] + public void WithSeedInMessage_RewritesMessage_AndLeavesDiagnosticNull_ForAPlainMessageOriginal() + { + // PR #26 review, third round - a generated HashSet<T>/Dictionary collection plan's + // unique-value-exhaustion path (CollectionPlan.scriban) throws exactly this shape: a plain + // CompositionException(string), no Diagnostic at all. That failure deserves the same + // pasteable seed as a diagnosed one, so WithSeedInMessage must handle it too, not throw. + var original = new CompositionException("a plain, non-pipeline-diagnosed message"); + + var wrapped = CompositionException.WithSeedInMessage(original, seed: 1); + + wrapped.Message.Should().Be("a plain, non-pipeline-diagnosed message\n\nSeed: 1"); + wrapped.Diagnostic.Should().BeNull(); + wrapped.InnerException.Should().BeSameAs(original); + } + [Fact] public void DoesNotDeclareAnyMemberOfTypeCompositionContext() { diff --git a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj new file mode 100644 index 0000000..c1471a0 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj @@ -0,0 +1,74 @@ +<Project Sdk="Microsoft.NET.Sdk"> + + <PropertyGroup> + <ImplicitUsings>enable</ImplicitUsings> + <Nullable>enable</Nullable> + <OutputType>Exe</OutputType> + <RootNamespace>Compono.XunitV3.SampleTests</RootNamespace> + <IsPackable>false</IsPackable> + <IsTestProject>true</IsTestProject> + <!-- Microsoft Testing Platform (MTP) runner, not VSTest --> + <TestingPlatformDotnetTestSupport>true</TestingPlatformDotnetTestSupport> + <UseMicrosoftTestingPlatformRunner>true</UseMicrosoftTestingPlatformRunner> + <!-- Deliberately no ProjectReference to Compono/Compono.XunitV3 anywhere in this project - the + whole point is consuming Compono.XunitV3 exactly the way an external consumer would, via + the PackageReference below, so packaging mistakes (a missing dependency, wrong + PrivateAssets, the generator analyzer not flowing transitively) surface here instead of + being masked by a ProjectReference build. See PLAN-0004 Phase 3 and ADR-0022's Testing + Strategy. --> + <LocalFeedPath>$(MSBuildThisFileDirectory)../../.local-nuget-feed</LocalFeedPath> + <!-- Isolates this restore from NuGet's shared global packages folder entirely (PR #26 review, + second/third rounds), scoped per Compono_LocalPackagesId - an environment variable + RealRunnerTests.cs sets to a fresh GUID before spawning its nested `dotnet test`, so its two + concurrent invocations (from the net10.0 and net11.0 Compono.XunitV3.Tests hosts CI runs + side by side) never share this path with each other. A fixed version ("1.0.0") would + otherwise let a stale entry silently satisfy a later restore even after the local feed's own + .nupkg contents changed. Two earlier fixes were tried and reverted first (see + pack-to-local-feed.sh's own comment and PLAN-0004 Phase 3 Notes for the full account): + clearing the *shared global* cache before every pack raced against a sibling process still + reading from it; scoping this path by $(TargetFramework) instead of an env var seemed + reasonable but never actually worked - RealRunnerTests.cs hardcodes "-f net10.0" for its + nested command regardless of which TFM the *calling* test host itself runs as, so both + concurrent invocations resolved the identical "net10.0" value, isolating nothing (still + raced, just on a different shared path than before - also reproduced locally). $([System + .Guid]::NewGuid()) computed directly as an MSBuild property (rather than in the environment) + was tried before that and was unstable for an unrelated reason: NuGet's restore-graph + evaluation pass and the actual build evaluation pass each re-evaluate that property function + independently, producing two different GUIDs for the same nominal build (a real NU1102 + version mismatch). An environment variable has none of these problems: it's set once, in the + C# process that spawns the nested `dotnet test`, and inherited stably by that whole child + process tree - every internal MSBuild re-evaluation reads the identical value, and a second, + independently-spawned nested process gets its own distinct one. Falls back to a fixed, + non-random literal (not another live function call) for a developer running this project + directly, without going through RealRunnerTests, where there is no concurrent sibling to + isolate from in the first place. --> + <Compono_LocalPackagesId Condition="'$(Compono_LocalPackagesId)' == ''">manual</Compono_LocalPackagesId> + <RestorePackagesPath>$(MSBuildThisFileDirectory)obj/.nuget-packages/$(Compono_LocalPackagesId)/</RestorePackagesPath> + </PropertyGroup> + + <ItemGroup> + <Content Include="xunit.runner.json" CopyToOutputDirectory="PreserveNewest" /> + </ItemGroup> + + <ItemGroup> + <PackageReference Include="Compono.XunitV3" /> + </ItemGroup> + + <!-- Populates the local feed nuget.config (in this directory) points restore at, by packing the + current source tree - not a stale checked-in .nupkg - every time this project restores. + Self-healing on a clean checkout and always tests today's Compono/Compono.XunitV3 source, + which is the point of a "real packaged consumer" test: a regression in packaging (a missing + dependency, wrong PrivateAssets, the generator analyzer not flowing transitively) fails this + project's own restore/build/run rather than being masked by a stale package. + + Delegates to pack-to-local-feed.sh, which serializes the two `dotnet pack` calls behind a + cross-process lock - CI (and RealRunnerTests) run this project's net10.0/net11.0 test hosts + concurrently, each independently triggering this target via its own nested `dotnet test`, and + two unsynchronized `dotnet pack` invocations racing on the same shared .local-nuget-feed/ and + src/Compono*/bin/obj output corrupts both (caught in CI - see PLAN-0004 Phase 3 Notes). --> + <Target Name="PackToLocalFeed" BeforeTargets="Restore"> + <MakeDir Directories="$(LocalFeedPath)" Condition="!Exists('$(LocalFeedPath)')" /> + <Exec Command=""$(MSBuildThisFileDirectory)pack-to-local-feed.sh" "$(MSBuildThisFileDirectory)../../src/Compono/Compono.csproj" "$(MSBuildThisFileDirectory)../../src/Compono.XunitV3/Compono.XunitV3.csproj" "$(LocalFeedPath)" $(Configuration) "$(RestorePackagesPath)"" /> + </Target> + +</Project> diff --git a/test/Compono.XunitV3.SampleTests/Domain.cs b/test/Compono.XunitV3.SampleTests/Domain.cs new file mode 100644 index 0000000..bae4df1 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/Domain.cs @@ -0,0 +1,42 @@ +namespace Compono.XunitV3.SampleTests; + +// Reached only through SharedTests.SharedRepositoryIsReusedByTheService's own [Compose]-attributed +// theory parameters - no [Composable], no Create<T>()/CreateMany<T>(), no direct CompositionRow call +// site anywhere else in this project. Proves Compono.Generators.ComposeMethodDiscovery (Phase 1) +// generates a real plan through the packaged Compono.XunitV3 -> Compono dependency, not just an +// isolated Compono.Generators.Tests snapshot test. +public sealed class Repository; + +public sealed class OrderService +{ + public OrderService(Repository repository) + { + Repository = repository; + } + + public Repository Repository { get; } +} + +public sealed record CreateOrder(string CustomerName, int Quantity); + +// No provider, registration, [Composable], or generated plan can ever satisfy this - reserved for +// FailingCompositionTests, which needs a genuine (pipeline-propagated) composition failure rather +// than one of Compono.XunitV3's own pre-composition validation failures (PR #26 review). +// +// Deliberately never used as a [Compose]-attributed parameter's own type directly - an +// interface/abstract/delegate root always reports CMP0003 at compile time (a documented, deliberate +// PLAN-0004 Open Item: ComposedTypeAnalyzer's root-level check is stricter than its nested-member +// check), so an abstract root here would be a compile error, not the runtime composition failure this +// fixture needs. GatewayConsumer below wraps it as a nested constructor parameter instead, which uses +// the more lenient member-level check and is left as a runtime Resolve<T> call. +public interface IUnregisteredGateway; + +public sealed class GatewayConsumer +{ + public GatewayConsumer(IUnregisteredGateway gateway) + { + Gateway = gateway; + } + + public IUnregisteredGateway Gateway { get; } +} diff --git a/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs new file mode 100644 index 0000000..3b3edca --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs @@ -0,0 +1,21 @@ +namespace Compono.XunitV3.SampleTests; + +// Deliberately fails, on every run, via a genuine (pipeline-propagated) composition failure - not one +// of Compono.XunitV3's own pre-composition validation failures (a negative seed, a signature error, an +// inline-value mismatch), which don't exercise real composition at all (PR #26 review). Uses an +// explicit seed so Compono.XunitV3.Tests' RealRunnerTests can assert on a deterministic value: it +// shells out `dotnet test` against this project and asserts the captured output contains this exact +// seed, proving the milestone's "a composition failure's message contains a seed that reproduces the +// same failure" promise reaches a real xUnit v3 runner's actual output, not just an in-process GetData +// call. +public sealed class FailingCompositionTests +{ + public const int Seed = 24601; + + [Theory] + [Compose(Seed = Seed)] + public void DeliberatelyFailingComposition_NoProviderCanSatisfyTheNestedInterfaceDependency(GatewayConsumer consumer) + { + consumer.Should().BeNull("GetData throws before this body ever runs - this line never executes"); + } +} diff --git a/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs b/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs new file mode 100644 index 0000000..690f1d7 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs @@ -0,0 +1,37 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed class InlineAndComposedTests +{ + [Theory] + [Compose(42, "widget")] + public void InlineValuesAreUsedDirectly(int quantity, string productName) + { + quantity.Should().Be(42); + productName.Should().Be("widget"); + } + + [Theory] + [Compose] + public void ComposedValuesAreProducedForEveryParameter(int quantity, string productName) + { + ((object)quantity).Should().BeOfType<int>(); + productName.Should().NotBeNull(); + } + + [Theory] + [Compose(42)] + public void MixesInlineAndComposedValues(int quantity, string productName) + { + quantity.Should().Be(42); + productName.Should().NotBeNull(); + } + + [Theory] + [Compose] + public async Task ComposesForAnAsyncTheory(string value) + { + await Task.Yield(); + + value.Should().NotBeNull(); + } +} diff --git a/test/Compono.XunitV3.SampleTests/ProfileTests.cs b/test/Compono.XunitV3.SampleTests/ProfileTests.cs new file mode 100644 index 0000000..729323d --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/ProfileTests.cs @@ -0,0 +1,22 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed record NotificationSettings(string SenderAddress); + +// Reached only through ProfileTests.ComposesTheProfileConfiguredValue's own [Compose<TProfile>] +// theory parameter - proves ComposeAttribute<TProfile> actually applies TProfile.Configure through the +// real packaged pipeline, not just Compono.XunitV3.Tests' in-process GetComposer()/CreateRow checks. +public sealed class ApplicationTestProfile : ICompositionProfile +{ + public void Configure(CompositionBuilder builder) => + builder.Register(() => new NotificationSettings("sample-test-sender@example.com")); +} + +public sealed class ProfileTests +{ + [Theory] + [Compose<ApplicationTestProfile>] + public void ComposesTheProfileConfiguredValue(NotificationSettings settings) + { + settings.SenderAddress.Should().Be("sample-test-sender@example.com"); + } +} diff --git a/test/Compono.XunitV3.SampleTests/SharedTests.cs b/test/Compono.XunitV3.SampleTests/SharedTests.cs new file mode 100644 index 0000000..9cbf9a1 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/SharedTests.cs @@ -0,0 +1,12 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed class SharedTests +{ + [Theory] + [Compose] + public void SharedRepositoryIsReusedByTheService([Shared] Repository repository, OrderService service, CreateOrder command) + { + service.Repository.Should().BeSameAs(repository); + command.Should().NotBeNull(); + } +} diff --git a/test/Compono.XunitV3.SampleTests/nuget.config b/test/Compono.XunitV3.SampleTests/nuget.config new file mode 100644 index 0000000..5d060c7 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/nuget.config @@ -0,0 +1,10 @@ +<?xml version="1.0" encoding="utf-8"?> +<configuration> + <packageSources> + <clear /> + <!-- Populated by this project's own PackToLocalFeed pre-restore target (see the .csproj) - + never committed, regenerated from current source on every restore. --> + <add key="compono-local" value="../../.local-nuget-feed" /> + <add key="nuget.org" value="https://api.nuget.org/v3/index.json" /> + </packageSources> +</configuration> diff --git a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh new file mode 100755 index 0000000..3bf6f8b --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Packs Compono and Compono.XunitV3 into the local NuGet feed this project restores against +# (test/Compono.XunitV3.SampleTests/nuget.config), serialized behind a cross-process lock, and clears +# this restore's own isolated packages path before every pack. +# +# Why the lock: this script is invoked from a PackToLocalFeed MSBuild target +# (BeforeTargets="Restore") on every restore of this project. CI (and RealRunnerTests, which shells +# out `dotnet test` against this project) runs Compono.XunitV3.Tests for multiple TFMs concurrently, +# so two independent nested `dotnet test` processes can each trigger this script at the same moment, +# both packing the same src/Compono and src/Compono.Generators projects into the same +# .local-nuget-feed/ directory - a real race (reproduced locally: "The process cannot access the +# file '.../Compono.1.0.0.nupkg' because it is being used by another process", and in CI: "Could not +# find a part of the path '.../Compono.Generators/bin/Debug/netstandard2.0'"). `mkdir` is atomic even +# across processes and platforms, so it works as a portable mutex without a custom MSBuild task. +# +# Why $restore_packages_path is cleared here (PR #26 review, second round): NuGet treats a package +# id+version already present in a packages folder as immutable and never re-extracts it, so this +# script repacking different content under the same fixed "1.0.0" version would otherwise let a stale +# entry silently satisfy a later restore. Two earlier fixes were tried and reverted first (see +# PLAN-0004 Phase 3 Notes for the full account): clearing NuGet's *shared global* packages folder here +# raced against a sibling process's own restore still reading from it after this script's lock +# released ("Could not find a part of the path '.../packages/compono/1.0.0'", reproduced locally and +# caught in CI); a per-evaluation-random package version (via MSBuild's $([System.Guid]::NewGuid())) +# turned out unstable, because NuGet's restore-graph evaluation pass and the actual build evaluation +# pass each re-evaluate that property function independently, producing two different GUIDs for the +# same nominal build (a real NU1102 version mismatch, reproduced locally). $restore_packages_path +# (scoped per $(TargetFramework) by the caller) has neither problem: it's private to this one +# project+TFM's own restore, so clearing it can never affect a concurrent sibling process running a +# different TFM, and there is nothing left to make "the same value across evaluation passes" in the +# first place - it's a plain path, not a value that needs to agree with anything else. +set -euo pipefail + +compono_csproj="$1" +xunitv3_csproj="$2" +feed_dir="$3" +configuration="$4" +restore_packages_path="$5" + +lock_dir="$feed_dir/.pack.lock" + +# Bounded wait, not an unconditional retry loop (PR #26 review, sixth round) - if a prior holder was +# killed before its own EXIT trap could run (SIGKILL, a canceled build, a machine restart), the lock +# directory is never cleaned up, and every subsequent restore of this project would otherwise wait on +# it forever. A real pack of both projects normally takes well under a minute, so 120 attempts (~2 +# minutes) is generous headroom for a genuinely concurrent sibling while still failing fast, with +# actionable remediation, on a truly abandoned lock rather than hanging indefinitely. +max_wait_attempts=120 +attempt=0 + +until mkdir "$lock_dir" 2>/dev/null; do + attempt=$((attempt + 1)) + if [ "$attempt" -ge "$max_wait_attempts" ]; then + echo "pack-to-local-feed.sh: timed out after ${max_wait_attempts}s waiting for the lock at '$lock_dir'." >&2 + echo "This usually means a previous run was killed before it could release the lock. If no other" >&2 + echo "'dotnet pack'/'dotnet test' against this project is currently running, remove it manually and" >&2 + echo "retry: rm -rf '$lock_dir'" >&2 + exit 1 + fi + sleep 1 +done +trap 'rmdir "$lock_dir"' EXIT + +rm -rf "$restore_packages_path" + +dotnet pack "$compono_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo +dotnet pack "$xunitv3_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo diff --git a/test/Compono.XunitV3.SampleTests/xunit.runner.json b/test/Compono.XunitV3.SampleTests/xunit.runner.json new file mode 100644 index 0000000..86c7ea0 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/xunit.runner.json @@ -0,0 +1,3 @@ +{ + "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json" +} diff --git a/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs new file mode 100644 index 0000000..58dd7e5 --- /dev/null +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs @@ -0,0 +1,32 @@ +using System.Collections.Concurrent; +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +public sealed class ComposeAttributeConcurrencyTests +{ + [Fact] + public async Task GetData_ProducesNoExceptionsOrDataRaces_WhenCalledConcurrently_OnOneSharedAttributeInstance() + { + // GetData runs fully synchronously and returns an already-completed ValueTask, so awaiting + // Task.WhenAll over a lazily-Select-projected sequence of .AsTask() calls does NOT exercise + // overlapping execution at all - WhenAll enumerates that deferred sequence one element at a + // time, and since each GetData call has already finished by the time .AsTask() returns, call + // N+1 is never even created until call N is done (PR #26 review, third round). + // Parallel.ForEachAsync genuinely dispatches all 200 calls across the thread pool + // concurrently, so this actually exercises concurrent first-touch access to the shared + // Lazy<Composer>/binding-plan initialization ADR-0022's Caching section describes. + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.Simple))!; + var lengths = new ConcurrentBag<int>(); + + await Parallel.ForEachAsync(Enumerable.Range(0, 200), async (_, _) => + { + var rows = await attribute.GetData(method, new DisposalTracker()); + lengths.Add(rows.Single().GetData().Length); + }); + + lengths.Should().HaveCount(200).And.OnlyContain(length => length == 2); + } +} diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index 23c5d3b..24c37c9 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -20,6 +20,59 @@ public static void WithNullableParameters(string? nullableReference, string notN { } + public static void WithNullableReferenceParameter(string? value) + { + } + + public static void WithNonNullableReferenceParameter(string value) + { + } + + public static void WithNullableValueParameter(int? value) + { + } + + public static void WithNonNullableValueParameter(int value) + { + } + + // Deliberately single-parameter - a second same-typed parameter would read this shared value + // back from scope too (Phase 0's stage-2 read gate applies to every same-typed request in the + // row, not just ones marked [Shared]) and re-validate it against that parameter's own + // nullability, which isn't what these fixtures exist to test. + public static void WithSharedNullableReferenceParameter([Shared] string? value) + { + } + + public static void WithSharedNonNullableReferenceParameter([Shared] string value) + { + } + + public static void WithSharedNullableValueParameter([Shared] int? value) + { + } + + public static void WithSharedNonNullableValueParameter([Shared] int value) + { + } + + // No provider and no generated plan can ever satisfy this interface (this test project doesn't + // reference Compono.Generators as an analyzer, and nothing registers it) - composing it always + // fails, deterministically, which is exactly what the seed-message-content proof needs. + public static void WithUnregisteredInterfaceParameter(IUnregisteredDependency value) + { + } + + // Reaches CollectionExhaustionPlan (registered via CollectionPlanCache<HashSet<bool>>.Instance, + // since this test project doesn't reference Compono.Generators as an analyzer) - a plain-message + // CompositionException, no Diagnostic at all, exactly matching what the real generated + // HashSet<T>/Dictionary collection plan throws on unique-value exhaustion + // (CollectionPlan.scriban). Proves GetData appends the seed to this shape too, not only a + // pipeline-diagnosed one (PR #26 review, third round). + public static void WithExhaustedHashSetParameter(HashSet<bool> values) + { + } + public static void WithDisposableParameter(DisposableValue disposable) { } @@ -66,6 +119,30 @@ public sealed class TestProfile : ICompositionProfile public void Configure(CompositionBuilder builder) => builder.Register(() => "from-profile"); } + // Mirrors CollectionPlan.scriban's own HashSet<T> shape exactly (same UniqueValueResolver call, + // same plain-message CompositionException on exhaustion) rather than just throwing directly, + // since this test project doesn't reference Compono.Generators as an analyzer and can't get the + // real generated plan (testing.md's hand-fake convention). Registered by the test itself (not a + // static constructor here - typeof(...)/GetMethod(...) don't trigger a type's static constructor, + // only an actual member access does), matching Compono.Tests' CollectionPlanCacheDispatchTests + // register/try-finally-unregister pattern. + public sealed class CollectionExhaustionPlan : ICompositionPlan<HashSet<bool>> + { + public HashSet<bool> Compose(ICompositionContext context) + { + var size = context.ResolveCollectionSize(); + var result = new HashSet<bool>(size); + + for (var i = 0; i < size; i++) + { + if (!UniqueValueResolver.TryResolve<bool>(context, CompositionRequestKind.CollectionElement, i, Nullability.NotNullable, result, out _)) + throw new CompositionException($"Could not generate {size} unique values of type 'bool' for 'HashSet<bool>' after {UniqueValueResolver.MaxAttempts} attempts per element - the element type's value space is likely too small for the requested collection size."); + } + + return result; + } + } + // Composed via a registration, not a generated plan - this test project doesn't reference // Compono.Generators as an analyzer (testing.md), so a registration is the only way to get a // real (non-fake) composed value for a custom type here. @@ -83,3 +160,5 @@ public sealed class DisposableValue : IDisposable public void Dispose() => DisposeCount++; } + +internal interface IUnregisteredDependency; diff --git a/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs b/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs new file mode 100644 index 0000000..84d8753 --- /dev/null +++ b/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs @@ -0,0 +1,117 @@ +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +// The four null/non-null-parameter combinations Phase 3's plan requires, each covered for both an +// ordinary parameter and an inline-[Shared] parameter - proving rejection happens before +// ShareExplicit's invoker is ever reached (a rejected inline value fails GetData's own +// pre-composition validation loop, which runs before either binding loop touches a parameter). +public sealed class InlineNullHandlingTests +{ + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForANullableReferenceParameter() + { + var attribute = new ComposeAttribute((string?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableReferenceParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForANullableValueParameter() + { + var attribute = new ComposeAttribute((int?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForANonNullableReferenceParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync<CompositionException>() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForANonNullableValueParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableValueParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync<CompositionException>() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForASharedNullableReferenceParameter() + { + var attribute = new ComposeAttribute((string?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNullableReferenceParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForASharedNullableValueParameter() + { + var attribute = new ComposeAttribute((int?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForASharedNonNullableReferenceParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNonNullableReferenceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync<CompositionException>() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForASharedNonNullableValueParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNonNullableValueParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync<CompositionException>() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_AcceptsANonNullInlineValue_ForANullableValueParameter() + { + // The regression test for the boxed-T-vs-boxed-Nullable<T> CLR bug: a non-null int arrives + // boxed as a boxed int, not a boxed int? - without the Nullable.GetUnderlyingType unwrap this + // is wrongly rejected against the raw int? parameter type. + var attribute = new ComposeAttribute(42); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().Be(42); + } +} diff --git a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs new file mode 100644 index 0000000..5b701fb --- /dev/null +++ b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs @@ -0,0 +1,29 @@ +namespace Compono.XunitV3.Tests; + +// Cheap insurance against accidental public-API drift (Phase 3's plan task) - locks the exact set of +// public types Compono.XunitV3 exposes, matching this milestone's "keep public APIs minimal" +// constraint (docs/public-api.md). A hand-rolled exact-set assertion, not a Verify snapshot: the +// expected shape is a fixed, short list, so a snapshot file would add ceremony without adding +// coverage testing.md doesn't already ask for. +public sealed class PublicApiSurfaceTests +{ + [Fact] + public void Assembly_ExposesExactlyTheDocumentedPublicTypes() + { + // IsPublic alone misses an accidentally-added nested public type (PR #26 review, fourth + // round): a nested type is never IsPublic (only a top-level type can be), it's IsNestedPublic + // instead - reflection's own naming split, not a synonym. Checking only IsPublic would let an + // unapproved `public class Foo { public class Bar { } }` addition slip through this exact-set + // assertion undetected. + var publicTypeNames = typeof(ComposeAttribute).Assembly.GetTypes() + .Where(static type => type.IsPublic || type.IsNestedPublic) + .Select(static type => type.FullName); + + publicTypeNames.Should().BeEquivalentTo( + [ + "Compono.XunitV3.ComposeAttribute", + "Compono.XunitV3.ComposeAttribute`1", + "Compono.XunitV3.SharedAttribute", + ]); + } +} diff --git a/test/Compono.XunitV3.Tests/SeedReportingTests.cs b/test/Compono.XunitV3.Tests/SeedReportingTests.cs new file mode 100644 index 0000000..92207da --- /dev/null +++ b/test/Compono.XunitV3.Tests/SeedReportingTests.cs @@ -0,0 +1,96 @@ +using System.Globalization; +using System.Text.RegularExpressions; +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +// Proves the pasteable-seed promise itself, not just a "Seed:" label's presence, for a genuine +// (pipeline-propagated) composition failure - as opposed to one of Compono.XunitV3's own +// pre-composition exceptions (negative seed, signature errors, inline validation), which already +// append "Seed: <value>" straight into Message via AppendSeed. A pipeline-thrown CompositionException's +// own Message never carried the seed on its own (only exception.Diagnostic did, via its own +// ToString()) - a real test runner's failure display shows Message, not Diagnostic.ToString(), so +// GetData now rewrites it via CompositionException.WithSeedInMessage (PR #26 review; ADR-0022 +// Amendment 5) before it propagates. Diagnostic itself (and its own correct Seed/ToString() rendering) +// is untouched by this - these tests check both. +public sealed partial class SeedReportingTests +{ + [Fact] + public async Task GetData_FailingComposition_MessageContainsTheAutoGeneratedRowSeed_AndReproducesTheSameFailure_WhenPastedBack() + { + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var firstAct = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var firstException = (await firstAct.Should().ThrowAsync<CompositionException>()).Which; + + var match = SeedLine().Match(firstException.Message); + match.Success.Should().BeTrue("GetData rewrites a propagating composition failure's Message to include a 'Seed: <value>' line"); + var pastedSeed = int.Parse(match.Groups["seed"].Value, CultureInfo.InvariantCulture); + + var pastedAttribute = new ComposeAttribute { Seed = pastedSeed }; + var secondAct = () => pastedAttribute.GetData(method, new DisposalTracker()).AsTask(); + var secondException = (await secondAct.Should().ThrowAsync<CompositionException>()).Which; + + secondException.Message.Should().Contain($"Seed: {pastedSeed}"); + } + + [Fact] + public async Task GetData_FailingComposition_MessageContainsTheExplicitRowSeed() + { + const int seed = 7654321; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync<CompositionException>()).Which; + + exception.Message.Should().Contain(seed.ToString(CultureInfo.InvariantCulture)); + } + + [Fact] + public async Task GetData_FailingComposition_DiagnosticIsPreservedUnchanged_AlongsideTheRewrittenMessage() + { + const int seed = 987654; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync<CompositionException>()).Which; + + exception.Diagnostic!.Seed.Should().Be((ulong)seed); + exception.Diagnostic.Message.Should().NotContain("Seed:", "the wrapped Message carries the seed - Diagnostic's own Message field is left as the pipeline's original plain text"); + exception.InnerException.Should().NotBeNull("the original pipeline exception is preserved, never discarded"); + } + + [Fact] + public async Task GetData_FailingComposition_MessageContainsTheSeed_ForAPlainMessageExceptionWithNoDiagnostic() + { + // CollectionPlan.scriban's generated HashSet<T>/Dictionary unique-value-exhaustion path throws + // a plain CompositionException(string) - no Diagnostic at all - so this proves the earlier + // catch-filter gap (PR #26 review, third round) is actually closed: every CompositionException + // gets the seed appended, not only a pipeline-diagnosed one. + const int seed = 246810; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithExhaustedHashSetParameter))!; + + CollectionPlanCache<HashSet<bool>>.Instance = new SampleTestMethods.CollectionExhaustionPlan(); + + try + { + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync<CompositionException>()).Which; + + exception.Diagnostic.Should().BeNull(); + exception.Message.Should().Contain($"Seed: {seed}"); + } + finally + { + CollectionPlanCache<HashSet<bool>>.Instance = null; + } + } + + [GeneratedRegex(@"Seed: (?<seed>\d+)")] + private static partial Regex SeedLine(); +}