diff --git a/.github/workflows/package-validation.yaml b/.github/workflows/package-validation.yaml index f963629..cb98677 100644 --- a/.github/workflows/package-validation.yaml +++ b/.github/workflows/package-validation.yaml @@ -118,11 +118,23 @@ jobs: # Reuses test/Compono.XunitV3.SampleTests' own PackToLocalFeed restore # (packs current source, not this job's Release artifacts above, into # .local-nuget-feed and restores the four publishable packages from - # there as a real consumer would) - filtered to exclude - # FailingCompositionTests, which fails by design per ADR-0022's + # there as a real consumer would) - filtered to exclude every class + # whose name starts with "Failing" (FailingCompositionTests, + # FailingConfigProfileTests, and any future one following the same + # naming convention), each of which fails by design per ADR-0022's # Testing Strategy and is why this project is deliberately not in - # Compono.slnx (docs/plans/0004-milestone-4-xunit-integration.md). + # Compono.slnx (docs/plans/0004-milestone-4-xunit-integration.md). A + # single wildcarded --filter-not-class covers the whole naming + # convention rather than needing a new literal class name added here + # every time a new deliberately-failing proof test is added (PR #65 + # review: a non-wildcarded, single-class filter here is exactly what + # broke this job when FailingConfigProfileTests' predecessor test was + # first added inside an otherwise-green class instead). The wildcard + # must be trailing-only ("Failing*", not "Failing*Tests") - the MTP + # CLI rejects a wildcard placed in the middle of a filter expression + # ("wildcards may only be at the beginning and/or end"), confirmed + # directly against the built test host. run: | dotnet test test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj \ -c Release \ - -- --filter-not-class "Compono.XunitV3.SampleTests.FailingCompositionTests" + -- --filter-not-class "Compono.XunitV3.SampleTests.Failing*" diff --git a/docs/adr/0036-parameterized-composition-profile-selection.md b/docs/adr/0036-parameterized-composition-profile-selection.md new file mode 100644 index 0000000..847f3e9 --- /dev/null +++ b/docs/adr/0036-parameterized-composition-profile-selection.md @@ -0,0 +1,530 @@ +# [ADR-0036] Call-Site Values Influencing Nested Composition + +**Status:** Accepted + +**Date:** 2026-08-08 + +**Decision Makers:** Nick Cipollina, Claude (design review) + +**Naming note:** this ADR keeps the identifier `ADR-0036` and its original +filename (`0036-parameterized-composition-profile-selection.md`) for +link-stability. Its title and Context were revised, before any design work +started, to avoid presupposing the eventual mechanism — the actual gap was +stated solution-neutrally, then a deep-design pass (below) evaluated four +genuinely different mechanisms before this ADR settled on one. Treat +"parameterized... profile selection" in the filename as a historical label +only, not a full description of the accepted shape — the accepted shape is +narrower and more specific than that filename suggests (a **typed +configuration object paired with a profile**, not a profile constructed +directly from an argument list); see Decision Outcome. + +**Terminology — two distinct concepts, not one.** This ADR introduces +**profile configuration arguments**, and deliberately does not reuse +**inline values** to describe them — they are different concepts governed +by different code paths, and conflating the terms in documentation or +error messages would make both harder to reason about: + +- **Inline values** (existing, [ADR-0022](0022-compono-xunit-package-design.md)) — + `[Compose(42, "widget")]`'s constructor arguments, bound positionally to + the **test method's own parameters**, partially or fully replacing + composition for that row. +- **Profile configuration arguments** (new, this ADR) — bound positionally + to a **`TConfig` type's constructor**, used only to construct the + profile that then configures the `Composer` for the whole test method; + never seen by, or bound to, the test method's own parameters at all. + +## Context + +[RESEARCH-0002](../research/0002-trivia-platform-comparison.md) — a +pre-migration capability survey of `ncipollina/trivia-platform`'s +AutoFixture-based test kit, run using +[ADR-0029](0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md)'s +rubric — surfaced one finding with no clean answer in Compono's current +model, stated at the level the gap actually exists, not at the level of +any one candidate fix: + +> Compono currently has no clean mechanism for compile-time-constant +> values supplied at a test call site to influence nested composition +> configuration for that specific test invocation. + +`trivia-platform`'s ~16 custom `AutoDataAttribute` subclasses are the real +evidence for this — most take runtime constructor arguments that change +what the underlying fixture customization actually produces somewhere +*inside* the composed graph, not just which top-level type gets composed. +For example (real call-site shapes, not invented): + +- `PersistenceAutoData(repositoryName)` — ~45 call sites, each supplying a + different repository name that the attribute's customization logic + switches on to configure a different DynamoDB table/persistence setup. +- `AnnouncementsAutoData(validConfig, gameOverEnabled, audienceEnabled, audienceItemEnabled, startOffsetDays, endOffsetDays, messageLocale, defaultLocale)` — + 8 constructor parameters, 18 call sites, each a distinct + boolean/locale combination driving which `AnnouncementsOptions` gets + built. +- `HandlerAutoData(requestType, aplSupported, locale, ...)`/ + `InterceptorAutoData(...)`/`PresenterAutoData(...)` — hundreds of call + sites across the Alexa-handler test suites, each configuring the + composed `IHandlerInput`/request shape differently per test. +- `InfraStackAutoData(region, account)`. + +`cosmere-tracker`'s Milestone 7 dogfooding pass ([RESEARCH-0001](../research/0001-autofixture-comparison.md)) +never surfaced this pattern — its custom attributes took no meaningful +runtime arguments, so this is new evidence, not a recurrence of an +already-decided question. + +Compono's `[Compose]` ([ADR-0022](0022-compono-xunit-package-design.md)) +selects a fixed, compile-time profile *type* — `TProfile` is a generic +type parameter, not a runtime value, and `ICompositionProfile.Configure(CompositionBuilder)` +takes no arguments of its own. `[Compose(42, "widget")]`'s inline-value +binding binds *test method parameters* positionally (per the migration +guide's "Migrate `[AutoData]` and `[InlineAutoData]`" section); it does +not thread a literal into configuration logic that runs somewhere *inside* +the composed graph. There is today no documented way for a value known at +the test call site to reach a `Register`/`.For()` decision made deeper in +composition — every such decision is either fully generic (the same for +every caller) or committed to one hard-coded configuration. + +**What this gap is not.** Two adjacent capabilities that might look +related are already solved and are explicitly out of scope for whatever +closes this gap: + +- **Requested type + resolution-site name.** `CompositionProviderRequest.Name` + already lets a custom `ICompositionValueProvider` match on the + requesting parameter/member's own name (`docs/concepts/providers.md`) — + this is how, e.g., `trivia-platform`'s `SlotSpecimenBuilder`/ + `ProductSpecimenBuilder`-shaped patterns already have a clean Compono + answer per RESEARCH-0002's Finding 2. Nothing about this ADR should + re-solve that with a second request-descriptor abstraction. +- **Fixed member-specific override.** `.For().Member(...)` already + covers "this one member of this one type always gets this value/rule." +- **The actual gap** is a third, distinct case neither of the above + reaches: a compile-time-constant value supplied at a specific test's + call site needs to influence a composition decision made for *that test + invocation only* — not a global provider rule, not a fixed member + override, but a per-invocation input to otherwise-static configuration + logic. + +## Decision Drivers + +- `docs/manifesto.md`'s explicit non-goal of AutoFixture feature parity — + this finding still needs to survive the same "is this a real gap or an + acceptable Compono-native alternative" question every other finding in + RESEARCH-0002 was put through, not be assumed onto the roadmap because + AutoFixture happens to support it. +- [ADR-0029](0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md)'s + evidence-driven restraint: a roadmap-candidate finding gets a + `Proposed` ADR stating the problem only — the actual API design belongs + to a later deep-design pass, not to this ADR. +- [ADR-0001](0001-source-generation-first.md)'s no-reflection-by-default + posture and this repo's explicit-over-implicit bias — any eventual + solution has to survive these, not just be convenient. +- [ADR-0017](0017-immutable-composer-configuration-and-builder-model.md)'s + immutable-builder model — a profile's `Configure` method runs once, + declaratively, before composition; whatever closes this gap can't + require mutating an already-built `Composer` mid-test. +- The evidence is high-frequency and structurally costly, not marginal: + per RESEARCH-0002's Finding 1, none of Compono's current mechanisms — + writing a distinct profile per configuration variant, or falling back to + inline `Composer.Create(builder => ...)` in each affected test — let a + call site keep the concise, declarative attribute-based idiom + (`[Compose]` on the method, real composed values in the + signature) without substantial duplication or hand-written boilerplate + once the number of real variants grows past a couple. That cost, not any + specific workaround's mechanics, is the actual evidence. + +## The reframing finding + +Before evaluating mechanisms, the deep-design pass established a fact that +changes the shape of this entire ADR: **core Compono already solves the +underlying problem.** `CompositionBuilder.AddProfile(ICompositionProfile +profile)` ([ADR-0018](0018-composition-profiles.md)) already exists, and +its own XML doc already states its purpose: *"for a profile that needs +constructor arguments or is otherwise not default-constructible."* +Programmatically, `Composer.Create(b => b.AddProfile(new +PersistenceTestProfile(repositoryName)))` already works today, with no new +code. The gap this ADR closes is entirely one layer up: `Compono.XunitV3`'s +`[Compose]` only supports `TProfile : ICompositionProfile, new()` +— there is no attribute-level path from a compile-time-constant literal to +a non-default-constructed profile instance. **This means the fix belongs +entirely in `Compono.XunitV3` — core `Compono` needs zero new capability.** +See "Considered Options" below for why this rules out treating the +underlying idea as generally useful outside xUnit attributes: it already +is, today, via `AddProfile(ICompositionProfile)`. + +## Considered Options + +Four genuinely different mechanisms were generated and compared, per +`design-decisions.md`'s deep-dive requirement (not a strawman plus a +preferred option): + +### 1. Attribute arguments bind directly to `TProfile`'s own constructor + +`[Compose(args...)]`, reflection-matching `args` against +`TProfile`'s constructor directly — no separate config type. + +**Rejected — collides with shipped API, not on merit.** +`ComposeAttribute(params object?[] inlineValues)` already ships, +and its constructor arguments already mean "bind to the test method's +leading parameters" ([ADR-0022](0022-compono-xunit-package-design.md)). +Reusing that same argument slot to instead mean "construct `TProfile` +with these" is a silent, ambiguous, breaking redefinition of shipped +behavior — `[Compose(42, "widget")]` today binds `42`/`"widget"` +to test parameters; this option cannot reuse that syntax without breaking +it. + +### 2. Ambient scenario/invocation values, resolved via `ICompositionContext` + +`Configure(CompositionBuilder)`'s signature stays untouched; a new +per-row value bag is attached to `CompositionRow`, and a factory +registered inside `Configure` pulls a value from it at *resolve* time +(`context.ResolveScenarioValue("name")`), rather than at +profile-construction time. + +**Rejected — no evidence justifies the size of this option.** It is the +most general shape (it would also cover values varying *per row*, not +just per method), but nothing in RESEARCH-0002's evidence needs +per-row variation — every real `trivia-platform` call site is one +attribute instance, fixed arguments, applied once per method. It scores +worst on **API clarity/discoverability**: a profile's scenario +dependencies aren't visible in its `Configure` signature at all, only +discoverable by reading every factory body inside it. It is also by far +the largest new surface — a new context API, a new resolution-order +interaction, new diagnostics naming, and new determinism/seed interaction +to design from scratch, none of which RESEARCH-0002's evidence justifies +today. Per [ADR-0029](0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md)'s +evidence-driven restraint, this is named and shelved, not designed +further, until a real per-row-varying call site actually surfaces. + +### 3. Source-generated specialization per call site + +The generator recognizes `[Compose(literalArgs)]` and emits a +closed, literal-baked construction path per call site — zero runtime +dispatch of the arguments at all. + +**Rejected — disproportionate to what it would save.** Profile +construction already happens once per test *method* (cached across every +row that method produces, per ADR-0022's Caching section), not once per +composed object — nowhere near the hot path [ADR-0001](0001-source-generation-first.md)'s +no-reflection rule actually targets (repeated per-object construction +reflection). Buying that already-cheap, already-bounded cost out entirely +would require real new generator complexity (understanding +attribute-literal semantics, routing them into type construction, new +snapshot-test surface) for a marginal runtime saving. + +### 4. A typed configuration object paired with the profile (chosen) + +A new, distinct attribute — `ComposeAttribute` — binds +profile configuration arguments positionally to `TConfig`'s constructor +(reusing [ADR-0022](0022-compono-xunit-package-design.md)'s existing +inline-value positional-binding validation, retargeted rather than +reinvented), constructs `TConfig`, then constructs `TProfile` from that +`TConfig`, then hands the fully-built instance to the **already-existing, +unchanged** `AddProfile(ICompositionProfile)`. See Decision Outcome for +the full shape. + +**Chosen** — smallest true addition to the system (one new attribute type +in `Compono.XunitV3`, zero core changes), reuses proven binding-validation +code instead of inventing new logic, and its one real cost (losing +`[Compose]`'s compile-time `new()` enforcement) is a narrow, +nameable tradeoff rather than a structural one. Full evaluation below. + +## Decision Outcome + +Chosen option: **4 — a typed configuration object paired with the +profile**, implemented entirely in `Compono.XunitV3`, with **zero changes +to core `Compono`** (`ICompositionProfile`, `AddProfile()`, +`AddProfile(ICompositionProfile)`, `ComposeAttribute`, +`ComposeAttribute`, and the existing inline-value binding +algorithm are all unchanged). + +### Shape + +```csharp +public sealed record PersistenceTestConfig(RepositoryKind Repository); + +public sealed class PersistenceTestProfile : ICompositionProfile +{ + private readonly PersistenceTestConfig _config; + + public PersistenceTestProfile(PersistenceTestConfig config) => _config = config; + + public void Configure(CompositionBuilder builder) => + builder.Register(_ => new PlayerRepository(_config.Repository)); +} +``` + +```csharp +[Theory] +[Compose(RepositoryKind.Player)] +public void Repository_Works(PlayerRepository sut) { } +``` + +A new attribute type, distinct from (not a subclass sharing a +constructor-argument slot with) the existing `ComposeAttribute`: + +```csharp +namespace Compono.XunitV3; + +[AttributeUsage(AttributeTargets.Method, AllowMultiple = false)] +public sealed class ComposeAttribute : ComposeAttribute + where TProfile : ICompositionProfile +{ + // Base gets zero inline values - this attribute form composes every + // test-method parameter in full; profile configuration arguments are + // a completely separate binding target (see Terminology, above). + public ComposeAttribute(params object?[] profileConfigurationArguments) : base() + { + // stored for use by the cached construction delegates described below + } +} +``` + +`TProfile`'s `new()` constraint is dropped (it doesn't apply to this +form — see "What this form deliberately gives up," below); `TConfig` is +unconstrained beyond being a type profile configuration arguments can +bind to. + +### Design principle: profile configuration arguments must not encourage stringly typed configuration + +`params object?[]` is a **binding mechanism**, forced by C#'s +attribute-argument-must-be-a-compile-time-constant rule — it is not a +license to design `TConfig` types around loosely-typed primitives. +Documentation, samples, and this ADR's own examples use the strongest +meaningful attribute-legal C# type available for each value: + +- A value that represents a finite, named choice → an `enum` + (`RepositoryKind.Player`, not `"Player"` or `"PlayerRepository"`). +- A value that represents a CLR type → `typeof(...)` + (`typeof(IntentRequest)`, not a type's string name). +- A value that's naturally boolean, numeric, or a genuinely free-form + string (a locale tag, say) → the corresponding attribute-legal type + directly, with no artificial enum/type wrapper forced onto it. + +`TConfig` itself should be a `record` (per `coding-standards.md`'s +"DTOs... must be immutable" rule) whose constructor parameters are named +and typed to carry real domain meaning — the same discipline any other +strongly-typed configuration object in this codebase already follows, not +a special case for this feature. + +### Constructor contracts — deliberately narrow, deterministic, no "best match" + +Per the explicit requirement that this stay narrow and predictable rather +than reintroducing AutoFixture-style implicit resolution: + +- **`TConfig` must have exactly one public constructor.** Zero or more + than one is a binding-plan-cache-time failure (see Diagnostics below) — + never a "pick the best/greediest one" heuristic. Profile configuration + arguments bind to that one constructor's parameters positionally, using + the identical validation ADR-0022 already built for inline values + (count check, `Nullable.GetUnderlyingType`-unwrap-before-assignability + check, clear per-parameter failure messages) — retargeted at `TConfig`'s + constructor instead of the test method's parameters, not reimplemented. +- **`TProfile` must have exactly one public constructor accepting exactly + one parameter of type `TConfig`.** Not "some constructor that could + accept a `TConfig`," not the greediest overload — an exact, + single-parameter, exact-type match. Zero or more than one qualifying + constructor is a binding-plan-cache-time failure, same category as + `TConfig`'s check. +- **No "best constructor match" algorithm exists anywhere in this + design**, for either type. Ambiguity is always a hard, named failure, + never a resolved-by-guessing outcome. + +### Diagnostics + +All three new checks are **pre-composition, computed once (per attribute +instance, at first `GetData` call), and cached** — the same place and +timing ADR-0022's existing signature-validation checks already run, never +re-checked per row: + +| Failure | When | Mechanism | +|---|---|---| +| `TConfig` has zero or >1 public constructors | Binding-plan-cache construction | Plain-message `CompositionException`, naming `TConfig` and the exact-one-constructor rule | +| `TProfile` has no (or >1) public constructor with exactly one `TConfig`-typed parameter | Binding-plan-cache construction | Plain-message `CompositionException`, naming `TProfile`, `TConfig`, and the exact-shape rule | +| Profile configuration argument count/type/nullability mismatch against `TConfig`'s constructor | Binding-plan-cache construction | Same validation and message shape as today's inline-value mismatch diagnostics ([ADR-0022](0022-compono-xunit-package-design.md)), retargeted at `TConfig`'s parameters | + +No new exception type — every case reuses the existing +`CompositionException` convention, consistent with "prefer existing +structured diagnostics" (`coding-standards.md`). + +### Reflection is bounded and cached, never on the hot path + +Building the `TConfig` constructor invoker and the `TProfile(TConfig)` +constructor invoker each happens **exactly once per attribute instance**, +at binding-plan-cache-construction time — the identical +close-once-cache-a-delegate shape ADR-0022 already uses for +`MakeGenericMethod`/`Delegate.CreateDelegate` in "Runtime-Typed +`CompositionRow` Invocation." Every subsequent `GetData` call for that +attribute instance reuses the cached delegates; nothing reflective runs on +the per-row composition path, consistent with +[ADR-0001](0001-source-generation-first.md)'s no-reflection-on-the-hot-path +rule. + +### What this form deliberately gives up + +**`[Compose]`'s compile-time `new()` enforcement does not carry +over.** Today, `[Compose]` for a type without a public +parameterless constructor is a **compile error** — nothing left to +validate at runtime. `ComposeAttribute` cannot offer +that: "does `TProfile` have a constructor accepting exactly one `TConfig`" +is not expressible as a C# generic constraint, so it becomes a +**deterministic runtime check** instead (see Diagnostics above) — still +computed once, cached, and failing clearly before any test executes, but +a real, honest regression from a compile error to a pre-composition +runtime error. This is stated explicitly here per the requirement that it +not be glossed over: it is an accepted cost of the chosen shape, not an +oversight. + +### Scope: `Compono.XunitV3` only, zero core changes + +`ICompositionProfile`, `CompositionBuilder.AddProfile()`, +`CompositionBuilder.AddProfile(ICompositionProfile)`, `ComposeAttribute`, +`ComposeAttribute`, and the existing inline-value binding +algorithm are **all unchanged** by this ADR. The new +`ComposeAttribute` type is additive, in +`Compono.XunitV3` only. This directly answers "does the underlying feature +belong in core or only `Compono.XunitV3`": the underlying capability +(building a profile from call-site-known values) is **already** a core +capability, reachable today from any C# call site via +`AddProfile(new Profile(...))` — what's missing is specifically an +attribute-to-instance bridge, which is inherently a problem of +attribute-based test-framework integration, not of the composition engine +itself. A future NUnit/MSTest integration would face the identical +bridging problem and solve it the identical way inside its own package — +not a reason to hoist this into core speculatively before a second +consumer exists. + +### Positive Consequences + +- Closes RESEARCH-0002's Finding 1 with the smallest true addition to the + system evaluated — one new `Compono.XunitV3` attribute type, zero core + changes. +- Reuses ADR-0022's proven positional-binding validation rather than + inventing a second binding algorithm. +- Strong typing at the point that matters most — inside `Configure`, a + profile author writes ordinary typed C# against `TConfig`, never + `object[]` unpacking. +- The "no stringly typed configuration" principle, and the "inline + values" vs. "profile configuration arguments" terminology split, are + now first-class, documented parts of the design — not left implicit for + a future doc pass to get wrong. +- Existing `[Compose]`, `[Compose]`, inline-value binding, and + `AddProfile()` are completely unaffected — no migration, no behavior + change, for any test that doesn't opt into the new attribute form. + +### Negative Consequences + +- Loses `[Compose]`'s compile-time `new()` enforcement for this + form specifically, replaced by a deterministic but runtime check — see + "What this form deliberately gives up" above. +- Two attribute forms now exist for profile selection + (`ComposeAttribute` and `ComposeAttribute`) + instead of one — an accepted, small increase in public-surface area for + a capability real evidence demonstrates is needed. +- Options 2's more general "any call-site value, including per-row + variation" capability remains unbuilt — accepted per ADR-0029's + evidence-driven restraint; revisit only if a real per-row-varying call + site surfaces. + +## Pros and Cons of the Options + +### Leave it as-is (not chosen) + +- Good, because it requires no further Compono work. +- Bad, because it leaves a high-frequency, high-cost gap unaddressed for + any real project (not just `trivia-platform`) whose tests need a + call-site value to shape nested composition. + +### Option 1 — args bind directly to `TProfile`'s constructor (rejected) + +- Good, because it needs no separate `TConfig` type. +- Bad, because it collides with `ComposeAttribute`'s already-shipped + inline-value constructor-argument meaning — not resolvable without + breaking existing behavior. + +### Option 2 — ambient scenario values via context (rejected, shelved) + +- Good, because it's the most general shape, covering per-row variation + option 4 doesn't. +- Bad, because nothing in the evidence needs per-row variation, and it's + the largest new surface of any option considered, with the weakest + API-discoverability story (a profile's dependencies aren't visible in + its own signature). + +### Option 3 — source-generated specialization (rejected) + +- Good, because it's the most "true to source-gen-first" mechanism, with + zero runtime argument dispatch. +- Bad, because profile construction is already a cheap, one-time-per-method + cost, not a hot path — the generator complexity this option would add + isn't proportionate to what it saves. + +### Option 4 — typed configuration object paired with the profile (chosen) + +- Good, because it reuses proven binding-validation code instead of + inventing new logic. +- Good, because it requires zero core `Compono` changes. +- Good, because it keeps strong typing at the `Configure` boundary. +- Bad, because it loses `[Compose]`'s compile-time `new()` + enforcement, replaced by a deterministic runtime check — an accepted, + explicitly-stated cost. + +## Amendment 1 (2026-08-09): direct `ConstructorInfo.Invoke`, not a cached delegate + +"Reflection is bounded and cached, never on the hot path" (above) +specified the identical close-once-cache-a-delegate shape +[ADR-0022](0022-compono-xunit-package-design.md) uses for +`MakeGenericMethod`/`Delegate.CreateDelegate` — building a cached invoker +delegate for `TConfig`'s and `TProfile`'s constructors once, at +binding-plan-cache-construction time. PLAN-0036's implementation does not +do this: `ConfigProfileBinder` calls `ConstructorInfo.Invoke` directly +(via a small shared `Invoke` helper that also unwraps +`TargetInvocationException`, per PR #65 review round 3), with no +separate delegate-caching layer of its own. + +This is a correction to that section's implementation detail, not a +reversal of the section's actual guarantee. The guarantee — reflection +bounded to once per attribute instance, never on the repeated per-row +`GetData` path — still holds, for a different reason than originally +assumed: `ComposeAttribute.ApplyProfile` (the only +caller of `ConfigProfileBinder`'s methods) is itself only ever invoked +once per attribute instance, from inside the base `ComposeAttribute`'s +existing `Lazy`-backed caching (`ComposeAttribute.cs`'s +`_composer` field) — a caching layer this ADR's original design already +relied on for the *composer* as a whole, but didn't originally credit +with also bounding the *constructor-resolution* reflection specifically. +`RowInvokers`' `MakeGenericMethod`/`Delegate.CreateDelegate` shape exists +for a genuinely different reason: it closes a generic method over a +parameter type known only at runtime (a test method's own +`ParameterInfo.ParameterType`, discovered per-parameter across +potentially many parameters), which needs a delegate cache to avoid +`MakeGenericMethod`/`MethodInfo.Invoke` cost repeating per row. +`TConfig`/`TProfile` need no equivalent: they are already +compile-time-closed generic arguments on +`ComposeAttribute` itself, so there is no per-runtime- +discovered-type generic closure to cache in the first place — a direct +`ConstructorInfo.Invoke`, called once (per the `Lazy` guarantee +above), already satisfies the no-reflection-on-the-hot-path requirement +without needing the heavier delegate-caching mechanism. + +This does not change the ADR's Decision Outcome (Option 4 remains +chosen) or any of its stated tradeoffs — it corrects one implementation +detail this ADR specified more precisely than turned out necessary, per +`design-decisions.md`'s Amendment mechanic for a correction discovered +during implementation. + +## Links + +- [RESEARCH-0002](../research/0002-trivia-platform-comparison.md) — + Finding 1, the evidence this ADR records +- [ADR-0029](0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md) — + the rubric/classification framework and evidence-driven-restraint rule + this ADR follows +- [ADR-0022](0022-compono-xunit-package-design.md) — governs + `[Compose]`'s current fixed-type-only selection +- [ADR-0018](0018-composition-profiles.md) — governs `ICompositionProfile`'s + current no-argument `Configure` shape +- [ADR-0017](0017-immutable-composer-configuration-and-builder-model.md) — + the immutable-builder constraint a future solution must respect +- [ADR-0001](0001-source-generation-first.md) — the no-reflection-by-default + constraint a future solution must respect +- `ncipollina/trivia-platform` — the repo whose real call sites motivate + this ADR; not part of this monorepo diff --git a/docs/adr/README.md b/docs/adr/README.md index 7391722..9375a3c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -97,3 +97,4 @@ the mechanics: numbering, status, and the index. | [0033](0033-public-preview-samples-strategy.md) | Public Preview Samples Strategy | Accepted | | [0034](0034-benchmark-suite-strategy-and-redesign.md) | Benchmark Suite Strategy and Redesign | Accepted | | [0035](0035-compono-agent-skill-pack.md) | Compono Agent Skill Pack | Accepted | +| [0036](0036-parameterized-composition-profile-selection.md) | Call-Site Values Influencing Nested Composition | Accepted | diff --git a/docs/how-to/use-profiles.md b/docs/how-to/use-profiles.md index 76743dd..828069a 100644 --- a/docs/how-to/use-profiles.md +++ b/docs/how-to/use-profiles.md @@ -31,6 +31,18 @@ public void ComposesTheProfileConfiguredValue(NotificationSettings settings) { } parameterless constructor — `[Compose]` enforces this at compile time via a generic constraint. +**A profile that needs a value known only at a specific test's call +site** — not a fixed, default-constructed one — can't use +`[Compose]` at all, since it has no way to receive that value. +`[Compose]` covers this: `TConfig` is a small, +strongly-typed configuration object, bound positionally from the +attribute's own constructor arguments and passed to `TProfile`'s +constructor. See +[`Compono.XunitV3`'s Package Guide](../packages/compono-xunitv3.md#profile-configuration-arguments) +for the full shape and +[Migrating from AutoFixture](../migrating-from-autofixture.md#migrate-a-parameterized-custom-autodataattribute) +for the AutoFixture pattern this replaces. + ## Combining more than one profile ```csharp diff --git a/docs/migrating-from-autofixture.md b/docs/migrating-from-autofixture.md index 7e7925c..4259027 100644 --- a/docs/migrating-from-autofixture.md +++ b/docs/migrating-from-autofixture.md @@ -89,6 +89,7 @@ each row is expanded into its own section below. | `fixture.Create()` | `composer.Create()` | | `[AutoData]` | `[Compose]` | | Custom `AutoDataAttribute` subclass | `[Compose]` | +| **Parameterized** custom `AutoDataAttribute` subclass (constructor args driving customization logic) | `[Compose]` | | `ICustomization` | `ICompositionProfile` | | Exact-type specimen customization | `Register()` | | Exact-type `ISpecimenBuilder` | `Register()` | @@ -180,6 +181,93 @@ cover the rest with a separate `[Theory]`/`[InlineData]` method instead. See [`Compono.XunitV3`'s Package Guide](packages/compono-xunitv3.md#what-it-deliberately-doesnt-do) for the full mechanics of why stacking isn't supported. +## Migrate a parameterized custom `AutoDataAttribute` + +A common, larger pattern than the previous section's simple wrapper: a +custom `AutoDataAttribute` subclass whose own **constructor** takes +arguments that change what the underlying fixture customization produces +— not just which type gets composed, but a value read *inside* the +customization logic itself. Real, frequent examples found migrating a +much larger AutoFixture test suite than this guide's other examples are +drawn from (`ncipollina/trivia-platform`'s `PersistenceAutoData(repositoryName)` — +around 45 call sites, each a different repository name driving a +different persistence setup — and an 8-parameter +`AnnouncementsAutoData(validConfig, gameOverEnabled, ...)`): + +```csharp +// Before +public sealed class PersistenceAutoDataAttribute(string repositoryName) + : AutoDataAttribute(() => CreateFixture(repositoryName)) +{ + private static IFixture CreateFixture(string repositoryName) + { + var fixture = new Fixture(); + fixture.Customize(new PersistenceCustomization(repositoryName)); + return fixture; + } +} + +[Theory] +[PersistenceAutoData("PlayerRepository")] +public void Repository_Works(PlayerRepository sut) { } +``` + +Neither of the migration paths the previous sections cover fits cleanly +here: a plain `[Compose]` has no way to receive +`"PlayerRepository"` at all, and writing one profile subclass per +repository name doesn't scale to `AnnouncementsAutoData`'s combinatorial +8-flag argument space — nor does falling back to a hand-built +`Composer.Create(...)` per test, which reintroduces exactly the per-test +setup code the attribute-based idiom exists to eliminate. This is what +`[Compose]` ([ADR-0036](adr/0036-parameterized-composition-profile-selection.md)) +exists for — a **typed configuration object** paired with the profile, +bound from this attribute's own constructor arguments: + +```csharp +// After +public enum RepositoryKind +{ + Player, + Leaderboard, +} + +public sealed record PersistenceConfig(RepositoryKind Repository); + +public sealed class PersistenceProfile : ICompositionProfile +{ + public PersistenceProfile(PersistenceConfig config) => Config = config; + + public PersistenceConfig Config { get; } + + public void Configure(CompositionBuilder builder) => + builder.Register(_ => RepositoryOptionsFactory.Create(Config.Repository)); +} + +[Theory] +[Compose(RepositoryKind.Player)] +public void Repository_Works(PlayerRepository sut) { } +``` + +Note the enum, not a string — the original AutoFixture attribute took a +raw `string repositoryName`, but that string only ever had a handful of +valid values in practice (a finite, named choice). `params object?[]` is +a binding mechanism forced by C#'s attribute-argument-must-be-a- +compile-time-constant rule, not a license to carry the original +stringly-typed shape forward — see +[`Compono.XunitV3`'s Package Guide](packages/compono-xunitv3.md#profile-configuration-arguments) +for the full "prefer the strongest attribute-legal type" guidance +(`typeof(...)` for a CLR type, `bool`/numeric values where those already +carry the real meaning). + +**Don't reach for `[Compose]` for every parameterized +attribute, though.** If the "parameter" is really just a `[Frozen]`-style +substitute or a single fixed value that's the same for every call site in +practice, the simpler existing forms (`[Compose]`, an inline +value, a member rule) already cover it — reserve this form for the case +this section actually describes: a value that's genuinely different per +call site and needs to reach configuration logic running *inside* the +profile, not at the test method's own parameter list. + ## Migrate `ICustomization` ```csharp @@ -346,6 +434,57 @@ case. Reach for a custom `ICompositionValueProvider` only for the rarer case that genuinely needs to match on request shape rather than a fixed type — see [Providers](concepts/providers.md). +**A specimen builder that dispatches on the requesting *parameter/member +name*, not just its type** — several distinct values of the same +declared type, chosen by which parameter is asking — is the other real +case a custom `ICompositionValueProvider` covers cleanly. +`CompositionProviderRequest.Name` carries the requesting constructor +parameter/required member/test-method-parameter's own name for exactly +this: + +```csharp +// Before +public sealed class UpsellPayloadSpecimenBuilder : ISpecimenBuilder +{ + public object Create(object request, ISpecimenContext context) => request switch + { + ParameterInfo { Name: "newGamePayload" } => new UpsellPayload("new-game"), + ParameterInfo { Name: "lockedPackPayload" } => new UpsellPayload("locked-pack"), + _ => new NoSpecimen(), + }; +} +``` + +```csharp +// After +public sealed class UpsellPayloadProvider : ICompositionValueProvider +{ + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (request.RequestedType != typeof(UpsellPayload)) + return CompositionProviderResult.NotHandled; + + return request.Name switch + { + "newGamePayload" => CompositionProviderResult.Handled(new UpsellPayload("new-game")), + "lockedPackPayload" => CompositionProviderResult.Handled(new UpsellPayload("locked-pack")), + _ => CompositionProviderResult.NotHandled, + }; + } +} +``` + +Registered via `builder.AddSemanticProvider(new UpsellPayloadProvider())` +(or `AddTestDoubleProvider`, depending on what it's producing — see +[Providers](concepts/providers.md)). This is a different question from +[Profile configuration arguments](packages/compono-xunitv3.md#profile-configuration-arguments) — +a `Name`-based provider is a **global rule** ("whenever anything asks for +`UpsellPayload` named `newGamePayload`, produce this"), evaluated for +every matching request across every test; a profile configuration +argument is a **per-invocation value** known only at one specific test's +`[Compose(...)]` call site. Don't reach for one to +solve the other. + ## Handle recursion behavior **Intentional difference:** AutoFixture's default `ThrowingRecursionBehavior` @@ -469,7 +608,9 @@ in [Troubleshooting](troubleshooting/index.md#known-limitations). - [ ] Remove AutoFixture package references. - [ ] Add the required Compono packages at matching versions. -- [ ] Replace custom AutoData attributes with `[Compose]` or `[Compose]`. +- [ ] Replace custom AutoData attributes with `[Compose]` or + `[Compose]` — or, for one whose constructor arguments + drive customization logic, `[Compose]`. - [ ] Convert real customizations into profiles. - [ ] Delete empty or obsolete fixture abstractions. - [ ] Audit every `[Frozen]` usage to determine whether identity is diff --git a/docs/packages/compono-xunitv3.md b/docs/packages/compono-xunitv3.md index 08e7bc0..8cc93d0 100644 --- a/docs/packages/compono-xunitv3.md +++ b/docs/packages/compono-xunitv3.md @@ -26,6 +26,10 @@ composer's own `Create()`. [Your First Composed Theory](../getting-started/first-test.md). - **`[Compose]`** — same, with a specific [`ICompositionProfile`](../concepts/profiles.md) applied. +- **`[Compose]`** — same, with a profile built from + call-site-known profile configuration arguments — see + [Profile configuration arguments](#profile-configuration-arguments) + below. - **Inline + composed mixing** — `[Compose(42, "widget")]` binds inline values left-to-right; anything left over is composed. See [How Do I Write a Composed Theory?](../how-to/write-a-composed-theory.md). @@ -40,6 +44,71 @@ composer's own `Create()`. instead. See [Determinism and Seeding](../concepts/determinism-and-seeding.md). +## Profile configuration arguments + +`[Compose]` selects a fixed, default-constructed profile type — +the same profile, configured the same way, for every caller. When a +profile needs to be built differently per test call site (drawn from real +migration evidence — see +[Migrating from AutoFixture](../migrating-from-autofixture.md#migrate-a-parameterized-custom-autodataattribute)), +`[Compose]` binds this attribute's own constructor +arguments — **profile configuration arguments**, a distinct concept from +this package's inline values above — positionally to `TConfig`'s single +public constructor, then constructs `TProfile` from that `TConfig`: + +```csharp +public enum RepositoryKind +{ + Player, + Game, +} + +public sealed record RepositoryConfig(RepositoryKind Repository); + +public sealed class RepositoryProfile : ICompositionProfile +{ + public RepositoryProfile(RepositoryConfig config) => Config = config; + + public RepositoryConfig Config { get; } + + public void Configure(CompositionBuilder builder) => + builder.Register(_ => RepositoryFactory.Create(Config.Repository)); +} + +[Theory] +[Compose(RepositoryKind.Player)] +public void Handles_PlayerRepository(IRepository repository) { } +``` + +**Inline values vs. profile configuration arguments — never the same +thing.** Inline values (`[Compose(42, "widget")]`) bind to the **test +method's own parameters**. Profile configuration arguments +(`[Compose(...)]`) bind to **`TConfig`'s +constructor**, which builds the profile applied to the whole row — they +never bind to the test method's parameters, all of which are composed in +full under this attribute form. + +**Prefer the strongest attribute-legal type for each argument.** +`params object?[]` is a binding mechanism forced by C#'s +attribute-argument-must-be-a-compile-time-constant rule, not a license to +design `TConfig` around magic strings — use an `enum` for a finite choice +(`RepositoryKind.Player`, not `"Player"`), `typeof(...)` for a CLR type, a +`bool`/numeric value where that's already the real meaning. + +**Constructor contracts are narrow and deterministic, not "best match."** +`TConfig` must have exactly one public constructor; `TProfile` must have +exactly one public constructor accepting exactly one `TConfig`-typed +parameter. Either shape being missing or ambiguous is a clear, cached +`CompositionException` — computed once per attribute instance, never on +the per-row path. See +[Troubleshooting: Common Errors](../troubleshooting/common-errors.md) for +each specific message. This is a deliberate tradeoff: +`[Compose]`'s `TProfile : ICompositionProfile, new()` constraint +rejects an invalid profile type at **compile time**; this form's +constructor-shape checks can only happen at runtime, since "has a +constructor accepting exactly this type" isn't expressible as a C# generic +constraint. + ## What it deliberately doesn't do - **No stacking distinct Compose-family attributes on one method.** A test diff --git a/docs/packages/index.md b/docs/packages/index.md index 68ee876..bf3a9b3 100644 --- a/docs/packages/index.md +++ b/docs/packages/index.md @@ -7,7 +7,7 @@ need the first two. | Package | What it adds | Install if... | |---|---|---| | [`Compono`](compono.md) | The core composition engine: `Composer`, the resolution pipeline, and the source generator (embedded, no separate install). | Always — every other package depends on it. | -| [`Compono.XunitV3`](compono-xunitv3.md) | `[Compose]`/`[Compose]` theory data attributes and `[Shared]` parameter sharing for xUnit v3. | You write xUnit v3 tests and want composed theory parameters instead of hand-built test data. | +| [`Compono.XunitV3`](compono-xunitv3.md) | `[Compose]`/`[Compose]`/`[Compose]` theory data attributes and `[Shared]` parameter sharing for xUnit v3. | You write xUnit v3 tests and want composed theory parameters instead of hand-built test data. | | [`Compono.NSubstitute`](compono-nsubstitute.md) | Automatic substitute composition for interface, delegate, and (optionally) abstract-class parameters. | Your composed types depend on interfaces you'd otherwise stub by hand with NSubstitute. | | [`Compono.Bogus`](compono-bogus.md) | Realistic fake data — member-name-convention matching plus explicit `Faker` sugar. | You want `FullName`/`Email`/`StreetAddress`-shaped fields to look like real data instead of anonymous strings. | diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md new file mode 100644 index 0000000..7feb0bf --- /dev/null +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -0,0 +1,667 @@ +# [PLAN-0036] Call-Site Values Influencing Nested Composition + +**Status:** Done + +**Implements:** [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) + +## Goal + +A `Compono.XunitV3` test can select a profile that needs call-site-known +configuration — `[Compose(RepositoryKind.Player)]` +— without writing a dedicated profile subclass per configuration variant +or falling back to inline `Composer.Create(...)` per test. Done when: +`ComposeAttribute` exists in `Compono.XunitV3`, binds +profile configuration arguments positionally to `TConfig`'s single public +constructor (reusing ADR-0022's existing inline-value validation), builds +`TProfile` from that `TConfig` via `TProfile`'s single qualifying +constructor, applies it through the existing `AddProfile(ICompositionProfile)` +core API unchanged, and every constructor-shape/argument-mismatch failure +reported by ADR-0036 is a clear, pre-composition, cached-once diagnostic — +with `trivia-platform`'s `PersistenceAutoData`-shaped pattern working +end-to-end against a packaged `Compono.XunitV3` build as proof. + +## Scope + +**In scope**, per ADR-0036's Decision Outcome: + +- `ComposeAttribute` in `Compono.XunitV3`, with + `TProfile : ICompositionProfile` (no `new()` constraint). +- Positional binding of profile configuration arguments to `TConfig`'s + constructor, reusing ADR-0022's existing count/nullability/assignability + validation, retargeted. +- The two new constructor-shape diagnostics (`TConfig` not exactly one + public constructor; `TProfile` not exactly one public constructor taking + exactly one `TConfig`-typed parameter) plus the retargeted + argument-mismatch diagnostic. +- Cached, bounded reflection for both constructor invocations (closed once + per attribute instance, at binding-plan-cache-construction time), + mirroring ADR-0022's existing `MakeGenericMethod`/`Delegate.CreateDelegate` + pattern. +- Documentation across every surface listed in "Documentation tasks" + below — treated as part of the feature, not a closeout afterthought. +- The published `skills/compono` agent skill, reviewed and updated per + "Published skill tasks" below — a runtime change isolated to + `Compono.XunitV3` still changes what an agent should recommend, so the + skill is in scope even though no skill *code* changes. +- A benchmark-policy evaluation against ADR-0034 — see "Benchmark + evaluation" below. Benchmarks are added **only if** that evaluation + finds a real boundary crossed; otherwise the evaluation and its + reasoning are recorded here, not silently skipped. + +**Explicitly deferred** (per ADR-0036's Decision Outcome / "Considered +Options"): + +- Option 2's ambient scenario-value/per-row-varying mechanism — shelved, + no real call site needs it yet. +- Option 3's source-generated per-call-site specialization — rejected, + disproportionate to the one-time-per-method cost it would save. +- Combining profile configuration arguments with inline test-parameter + values on the same attribute — `ComposeAttribute` + composes every test-method parameter in full; no evidence yet needs both + in one row. +- Any actual `trivia-platform` migration work — this plan delivers the + Compono-side capability only; migrating `trivia-platform` itself is + separate, future work in that repo. +- Extending `benchmarks/Compono.Benchmarks` to cover `Compono.XunitV3`'s + attribute-binding cost *in general* (see "Benchmark evaluation" below — + this is a real, pre-existing gap the evaluation surfaces, but fixing it + is a separate, appropriately-scoped follow-up, not something to fold + into a single-feature plan). + +## Benchmark evaluation + +Per [ADR-0034](../adr/0034-benchmark-suite-strategy-and-redesign.md), +evaluated before deciding whether this plan adds any benchmark: + +**Finding: `benchmarks/Compono.Benchmarks` has no `Compono.XunitV3` +coverage at all today.** The project has no `ProjectReference` to +`Compono.XunitV3`, and none of its six categories +(`ImplementationStrategies`/`ConsumerScenarios`/`ExternalComparison`/ +`FeatureOverhead`/`Scalability`/`SourceGeneration`) measure attribute +binding-plan construction. This means the *already-shipped* +`[Compose]`'s own bounded, cached `MakeGenericMethod`/ +`Delegate.CreateDelegate` construction cost ([ADR-0022](../adr/0022-compono-xunit-package-design.md)'s +"Runtime-Typed `CompositionRow` Invocation") has never been benchmarked +either — this isn't a gap specific to the new attribute. + +**Decision: no new benchmark added by this plan.** Two reasons, not one: + +1. **No category fits.** ADR-0034's six categories answer questions about + the composition *engine* (core `Compono` + `Compono.Generators`); none + is scoped to test-framework-integration attribute-binding cost. Adding + a benchmark for only the new `ComposeAttribute` + path, with no comparable benchmark for the structurally-identical, + already-shipped `ComposeAttribute` path, would produce a + number with nothing to compare it against — not a real "did the + reflection stay bounded" answer, just an isolated figure. +2. **The property that actually matters is a correctness property, not a + performance one, and is already covered.** "Does the reflection stay + bounded to binding-plan construction and never run on the repeated + `GetData`/composition path" is proven by the Test Plan's + invoker-delegate-caching assertion (reflection runs exactly once per + attribute instance across many repeated `GetData` calls) — a unit test + proves *boundedness*; a microbenchmark would only add a relative-cost + number on top of a property the test already guarantees. + +**Recorded, not silently omitted, per the requirement:** extending +`benchmarks/Compono.Benchmarks` to cover `Compono.XunitV3` attribute +binding-plan construction — covering `[Compose]` and +`[Compose]` together, so any future comparison has a +baseline — is a real, legitimate future benchmark-suite gap. It's called +out here and in "Explicitly deferred" above rather than folded into this +plan, because closing it properly means extending ADR-0034's own category +structure (a new category, or a case for why an existing one covers it), +which is a decision for that ADR's own maintainers/a dedicated follow-up, +not something to decide as a side effect of one feature's plan. + +## Tasks + +**New files** + +- [x] `src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs` — the + new attribute type. +- [x] `src/Compono.XunitV3/Binding/ConfigProfileBinder.cs` (final name; + not `ConfigBindingPlan.cs` as originally sketched — see Notes) — + resolves/validates `TConfig`'s and `TProfile`'s constructors and + performs the actual binding/construction. + +**Changes to existing files** + +- [x] `src/Compono.XunitV3/ComposeAttribute.cs` — extracted the existing + inline-value `params object?[]` normalization (the single-null/ + single-array edge cases) into an `internal static + NormalizeParamsArguments` helper, reused by the new attribute's + constructor for its own, separate `configArguments` parameter — a + behavior-preserving refactor (see Notes), not a change to existing + binding semantics. +- [x] `src/Compono.XunitV3/Binding/BindingPlan.cs` — no change needed for + the core binding mechanism (see Notes for why the originally-sketched + approach of extending `BindingPlan` for construction turned out + unnecessary); a later review round did update this file's + Compose-family-stacking diagnostic message to name the new attribute + form (see Notes' round 5 entry) — a message-text fix, not a + reopening of the "no core change needed" finding. + +**Documentation tasks** (part of the feature, verified at closeout — see +"Verification and closeout" below, not left implicit): + +- [x] `docs/packages/compono-xunitv3.md` — new "Profile configuration + arguments" section under "What it gives you," cross-linking + ADR-0036, with the enum/`typeof(...)`/attribute-legal-type guidance + (no stringly typed examples), and an explicit one-line contrast + against inline values. +- [x] `docs/migrating-from-autofixture.md` — new "Migrate a parameterized + custom `AutoDataAttribute`" subsection (before/after, drawn from + `trivia-platform`'s real `PersistenceAutoData(repositoryName)` shape + per RESEARCH-0002 Finding 1, enum-based example), plus a "Quick + concept map" row and a "Migration checklist" line. +- [x] `docs/migrating-from-autofixture.md` — RESEARCH-0002 Finding 2's + documentation gap closed: `CompositionProviderRequest.Name`-based + `ICompositionValueProvider` matching added as a documented pattern + under "Migrate specimen builders," with an explicit note + distinguishing it from profile configuration arguments. +- [x] `docs/troubleshooting/common-errors.md`, not + `docs/reference/diagnostics.md` — corrected during implementation + (see Notes): `diagnostics.md` is scoped exclusively to the + generator's compile-time `CMP` codes; these three failures are + runtime, plain-message `CompositionException`s, exactly like + today's existing inline-value diagnostics, which already live in + `common-errors.md`'s "By symptom (runtime)" section, not + `diagnostics.md`. Documented there instead, as a new + `### "ComposeAttribute throws before my test even + runs"` subsection. +- [x] API reference — regenerated via + `.github/scripts/generate-api-reference.sh` (DefaultDocumentation, + per ADR-0032) against a Release build; produced the two new pages + for `ComposeAttribute` plus updates to the two + existing pages that list/link it, deterministically, with no other + diff. +- [x] README / package-table / sample surfaces — grepped; `README.md` + doesn't enumerate individual Compose-family forms, so left + unchanged; `docs/how-to/use-profiles.md` did enumerate the + `[Compose]` constraint specifically and got a new + paragraph pointing to the config form. `docs/packages/index.md`'s + package-table cell was initially left unchanged by the same + "would clutter a summary cell" reasoning, but a later review round + (see Notes' round 6 entry) corrected that call — the cell's job is + specifically to enumerate what a package gives you, so omitting a + shipped form from it was a real gap, not acceptable brevity; fixed + there, along with two equivalent stale rows in + `skills/compono/SKILL.md`. + +**Published skill tasks** (`skills/compono/`, reviewed even though the +runtime change is `Compono.XunitV3`-only — per ADR-0035, one skill, not a +new one per package): + +- [x] `skills/compono/references/xunit-v3.md` — `[Compose]` section added, enum example, explicit distinction from + `[Compose]` and from `Name`-based provider matching. +- [x] `skills/compono/references/patterns-and-antipatterns.md` — mapping + table row added; three new antipattern entries added (wrong + migration moves; stringly typed config args; confusing this feature + with `Name`-based provider matching). +- [x] `skills/compono/references/registrations-profiles-and-scopes.md` — + new "Custom providers — matching on request shape, including name" + section added, closing the pre-existing gap. +- [x] `skills/compono/SKILL.md` — workflow-step bullet added. +- [x] `skills/compono-evals/evals.json` — eval id 19 added (validated as + well-formed JSON; 19 evals total). +- [x] Confirmed no new skill created — all changes landed inside the + existing single `skills/compono/` skill. + +## Critical Files + +- `src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs` — new, the + public attribute surface this plan adds. +- `src/Compono.XunitV3/Binding/ConfigProfileBinder.cs` — new, the + constructor-resolution/validation/construction logic. +- `src/Compono.XunitV3/ComposeAttribute.cs` — modified (inline-value + normalization/validation extracted for reuse; behavior unchanged); left + `ComposeAttribute{TProfile}.cs` untouched. +- `src/Compono.XunitV3/Binding/PositionalArgumentBinder.cs` — new (added + in review round 5), the shared null/`Nullable`-unwrap/assignability + validator both `ComposeAttribute.GetData` and `ConfigProfileBinder` use. +- `src/Compono.XunitV3/Binding/BindingPlan.cs` — modified (review round 5: + the Compose-family-stacking diagnostic message now names all three + attribute forms). +- `src/Compono.Generators/ComponoIncrementalGenerator.cs`, + `src/Compono.Generators/Discovery/ComposeMethodDiscovery.cs` — modified + (review round 1: a third `ForAttributeWithMetadataName` registration for + `ComposeAttribute\`2`'s metadata name, without which a concrete type + reached only through `[Compose]` got no generated + plan at all). +- `test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs` — new, + 19 cases (grew from an initial 10 across review rounds 2-5: seed + reporting, abstract-type rejection, `TargetInvocationException` + unwrapping, the `Nullable`-boxing case). +- `test/Compono.XunitV3.Tests/BindingPlanTests.cs` — modified (review + round 5: one new stacking-diagnostic test for the new attribute form). +- `test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs`, + `test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs` — modified (new + fixtures; exact-public-type-set assertion updated for `ComposeAttribute\`2`). +- `test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs` — new, + packaged-consumer proof (real `dotnet test` run against the packed + NuGet, per its own csproj's existing pattern); composes a real concrete + `RepositoryConsumer` class as of review round 1's fix, not a bare + provider-resolved `string`, so it actually exercises generated-plan + discovery. +- `test/Compono.XunitV3.SampleTests/FailingConfigProfileTests.cs` — new + (review round 5), the one deliberately-failing packaged-consumer proof, + split into its own class to match `package-validation.yaml`'s + `Failing*`-named-class CI filter (see Notes' round 5 entry for the live + CI regression this fixed). +- `.github/workflows/package-validation.yaml` — modified (review round 5: + the "Local-feed packed-consumer smoke test" filter widened to a + trailing wildcard, `Failing*`, covering both `FailingCompositionTests` + and the new `FailingConfigProfileTests`). +- `docs/packages/compono-xunitv3.md`, `docs/migrating-from-autofixture.md`, + `docs/troubleshooting/common-errors.md`, `docs/how-to/use-profiles.md`, + `docs/packages/index.md` (review round 6), `docs/adr/0036-*.md`'s + Amendment 1 (review round 4), + `docs/reference/api/Compono.XunitV3/*` (regenerated) — documentation + updates, per `documentation.md`'s "update the subsystem doc in the same + PR" rule. +- `skills/compono/references/xunit-v3.md`, + `skills/compono/references/patterns-and-antipatterns.md`, + `skills/compono/references/registrations-profiles-and-scopes.md`, + `skills/compono/SKILL.md` (also touched again in review round 6), + `skills/compono-evals/evals.json` — + published-skill updates, in scope per ADR-0035. + +## Test Plan + +**Final state, after all six review rounds (see Notes below for the +round-by-round history): full solution `dotnet test` (`Compono.slnx`, +Debug): 913 passed, 0 failed, 0 skipped** — +`test/Compono.XunitV3.Tests` alone carries 134 of those (67 per TFM), +including 19 cases in `ComposeAttributeConfigBindingTests.cs` (grew from +an initial 10) and one new case in `BindingPlanTests.cs`. +`test/Compono.XunitV3.SampleTests` is excluded from `Compono.slnx` itself +(per its own csproj comment) but **is** run directly by the +`package-validation` CI workflow (a fact this plan originally got wrong — +see Notes' round 5 entry), not run "manually" as this section first +claimed: 20 passed (10 per TFM), 2 pre-existing `FailingCompositionTests` +failures (expected, unrelated), and 2 deliberate +`FailingConfigProfileTests` failures (expected, its own class as of round +5 — see below), all excluded from the CI gate by +`--filter-not-class "Compono.XunitV3.SampleTests.Failing*"`. + +Per `testing.md`'s existing `Compono.XunitV3.Tests` (fast, +direct-`GetData`) / `Compono.XunitV3.SampleTests` (real xUnit v3 runner) +split, matching how ADR-0022's own binding algorithm was verified: + +**`test/Compono.XunitV3.Tests`** (direct `GetData` calls, no real runner): + +- `TConfig` with exactly one public constructor, valid arguments → + `TProfile` constructed and applied correctly (assert the registration it + makes is actually in effect on the resulting `Composer`). +- `TConfig` with zero public constructors → clear, named + `CompositionException`, cached (not re-thrown with a different message + on a second `GetData` call on the same attribute instance). +- `TConfig` with more than one public constructor → same, distinct + message naming the ambiguity. +- `TProfile` with no constructor taking exactly one `TConfig` → clear, + named `CompositionException`. +- ~~`TProfile` with more than one qualifying constructor~~ — confirmed + during implementation this is unreachable via ordinary C#: two + constructors with an identical single-`TConfig`-parameter signature is a + compiler error (duplicate signature), so no test double can exercise + this branch; the check itself stays in `ConfigProfileBinder` as + defensive belt-and-suspenders (see Notes). +- Profile configuration argument count mismatch (too few/too many against + `TConfig`'s constructor) → reuses the existing pre-composition + "wrong argument count" message shape, retargeted. +- Profile configuration argument type mismatch (including the existing + `Nullable`-boxing-unwrap case, proving the reused validation still + handles it correctly against `TConfig`'s parameters) → reuses the + existing message shape. +- `null` profile configuration argument for a non-nullable `TConfig` + parameter → rejected, same as the existing inline-value rule. +- Invoker-delegate caching: `MakeGenericMethod`/constructor-invocation + reflection runs exactly once per attribute instance across many repeated + `GetData` calls (same assertion shape ADR-0022's own caching tests use). +- Existing `ComposeAttribute`/`ComposeAttribute` behavior is + unaffected — a regression check, not new coverage, confirming the new + type didn't touch the existing binding path. + +**`test/Compono.XunitV3.SampleTests`** (real xUnit v3 runner): + +- A representative `[Compose(...)]` theory, modeled on + `trivia-platform`'s `PersistenceAutoData(repositoryName)` shape (enum + argument, per ADR-0036's "no stringly typed configuration" principle), + run end-to-end against a packaged (not project-referenced) + `Compono.XunitV3` build — proving generated-plan discovery still reaches + every type composed inside the resulting profile's `Configure` method, + the same packaged-consumer verification ADR-0022's own Amendment + (2026-07-30) required for `[Compose]`-attributed parameters. +- A deliberately-failing case (a `TProfile` with no constructor accepting + the `TConfig`) asserted to fail before the test method ever executes, + with the expected diagnostic text — its own `FailingConfigProfileTests` + class as of round 5 (see Notes), matching `FailingCompositionTests`' + established pattern. + +**`skills/compono-evals`** — a new eval, shaped exactly as required: + +```json +{ + "id": 19, + "category": "migration", + "prompt": "Convert this parameterized AutoFixture custom AutoDataAttribute to Compono. It takes a RepositoryKind-shaped argument (currently a string constant) at each call site and configures a different repository customization per call.", + "expected_output": "Recognizes this as the profile-configuration-arguments pattern: a TConfig record (using an enum, not the original string) paired with a profile via [Compose(...)], not a combinatorial set of profile subclasses, a per-test Composer.Create(...) escape hatch, invented ambient/global scenario state, or a recommendation to keep the AutoFixture attribute.", + "files": [], + "expectations": [ + "Proposes [Compose] specifically, not [Compose] with no way to pass the value, and not a new attribute-per-argument-combination subclass", + "Uses an enum (or other attribute-legal typed value) for the finite-choice argument, not a magic string, per the no-stringly-typed-configuration principle", + "Does not suggest ambient/global mutable scenario state, a per-test hand-built Composer.Create(...) as the primary recommendation, or retaining the AutoFixture attribute", + "Correctly distinguishes this from inline values (which bind to the test method's own parameters) and does not conflate the two" + ] +} +``` + +Added to `skills/compono-evals/evals.json`'s existing `evals` array, +following the file's established id/category/prompt/expected_output/ +files/expectations shape. + +## Verification and closeout + +Explicit exit checklist — every item confirmed before this plan moves to +`Done`, not assumed from the Tasks list alone: + +- [x] Every new-API example compiles against the **packaged** + `Compono.XunitV3` surface — `test/Compono.XunitV3.SampleTests` + references `Compono.XunitV3` via `PackageReference` only (no + `ProjectReference` anywhere in that project, by design), packed + fresh from current source via `pack-to-local-feed.sh` on every + restore; `ConfigProfileTests.cs`'s two passing theories and + `FailingConfigProfileTests.cs`'s one deliberately-failing theory (its + own class as of review round 5) all ran successfully against that + real packaged build (see Notes for the exact `dotnet test` output + confirming the failure's stack trace originates in the packaged + `Compono.XunitV3.dll`, not a project reference). +- [x] Existing `[Compose]`/`[Compose]` semantics unchanged — + the full solution suite (913 tests, final count after all review + rounds) passes; no existing test file's assertions were modified, + only `PublicApiSurfaceTests.cs`'s exact-set list extended (expected, + additive) and `ComposeAttribute.cs`'s normalization/validation logic + extracted for reuse with no behavioral change (verified by + every pre-existing inline-value test still passing unmodified). +- [x] `ConfigArguments_AreNeverBoundAsInlineValues` (new test) proves + profile configuration arguments never populate the base class's + `InlineValues` — structurally impossible for them to be + misinterpreted as inline values, not just untested. +- [x] Benchmark evaluation satisfied by the "no new benchmark, here's why" + reasoning above — nothing during implementation contradicted it (no + new hot-path reflection was introduced; the invoker-delegate-caching + test proves boundedness directly). +- [x] Every documentation file updated (see Tasks above); README/package-table + grep completed with a documented "left unchanged by design" outcome + where appropriate. +- [x] Every skill file updated; the new eval (id 19) is well-formed JSON + in the existing array — running it against the now-updated skill is + a human/CI eval-harness action outside this coding session's own + tool access (no `run_eval.py`-equivalent invoked here); the skill + content itself was written to satisfy every one of the eval's + stated expectations directly. +- [x] `docs/roadmap/post-mvp.md`, `docs/adr/README.md`, + `docs/plans/README.md` all reflect final status — reconfirmed below. +- [x] This plan's `Status` set to `Done` — every box above checked. + +## Notes + +Implementation deviated from this plan's original file-level sketch in +two ways, neither changing the ADR's decision, both narrowing scope in a +good direction: + +1. **No `BindingPlan.cs` changes needed for construction itself** (a later + review round did touch this file for an unrelated reason — a + diagnostic-message-text fix, not a reopening of this finding; see + round 5 below). The plan originally assumed the new attribute would + need to hook into `BindingPlan`'s cache-construction pass the way + test-method-parameter binding does. It doesn't for the actual + construction/binding work: `ComposeAttribute.ApplyProfile` is + already called exactly once per attribute instance, for free, by the + *existing* `Lazy`-backed `_composer` field the base + `ComposeAttribute` class already has — `TConfig`/`TProfile` are + compile-time-closed generic arguments on the attribute class itself, + not a runtime-discovered `Type` requiring `MakeGenericMethod` the way + an arbitrary test-method parameter type does. `ConfigProfileBinder` + uses plain `ConstructorInfo`/`Type.GetConstructors()` reflection + directly (no `MakeGenericMethod`/`Delegate.CreateDelegate` dance), + documented as a deliberate, narrower reflection shape in its own XML + remarks — still bounded to once per attribute instance, just via the + existing caching mechanism rather than a new one. +2. **`docs/reference/diagnostics.md` was the wrong target** — corrected to + `docs/troubleshooting/common-errors.md` once the file's actual scope + (compile-time `CMP` codes only) was checked directly rather than + assumed from the plan's original hedge. + +**Packaged-consumer verification, actual output** (from `dotnet test +test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj -c +Debug`, both TFMs): `ConfigProfileTests.ComposesTheProfileBuiltFromConfigArguments` +and `.DifferentConfigArguments_ProduceADifferentlyConfiguredProfile` both +pass, proving `RepositoryKind.Player`/`RepositoryKind.Game` produce +differently-configured profiles through the real packaged pipeline. +`ConfigProfileTests.MismatchedProfileConstructorShape_FailsBeforeTheTestExecutes` +fails exactly as designed, with message `'Compono.XunitV3.SampleTests.ProfileWithNoMatchingConstructor' +must have exactly one public constructor accepting a single +'Compono.XunitV3.SampleTests.RepositoryTestConfig' parameter, but has 0.`, +stack-traced through `ConfigProfileBinder.ResolveSingleProfileConstructor` +→ `BuildProfile` → `ComposeAttribute\`2.ApplyProfile` → `BuildComposer` → +`Lazy.CreateValue()` → `GetData` — confirming the failure +happens before the test body runs, from inside the packaged assembly. +Full solution `dotnet build`/`dotnet test` (`Compono.slnx`): 0 warnings, +0 errors, 893/893 passed. + +**PR #65 review (Codex) caught a real blocking gap this plan's own +verification missed:** `Compono.Generators`' `ComposeMethodDiscovery` was +registered against the non-generic and one-type-parameter +`ComposeAttribute` metadata names only (`ComponoIncrementalGenerator.cs`) +— `ComposeAttribute`'s own arity-suffixed metadata name +(`Compono.XunitV3.ComposeAttribute\`2`) was never registered, so a +concrete parameter type reached *only* through +`[Compose]` (no other `Create()`/`[Composable]` call +site) got no generated `ICompositionPlan` at all and would fail at +`GetData` time in real usage. This plan's own packaged-consumer sample +(`ConfigProfileTests.cs`) didn't catch it because its only composed +parameter type was a `string` — provider-resolved, never needs a +generated plan — masking the gap exactly the way `testing.md`'s +"verifying a new public entry point" rule warns against. Fixed: a third +`ForAttributeWithMetadataName` registration added for +`ComposeMethodDiscovery.TwoTypeParameterAttributeMetadataName`, merged +into the same `composeMethodResultsAll` pipeline as the other two arities +(`ComponoIncrementalGenerator.cs`); a new isolated +`Compono.Generators.Tests` snapshot test +(`ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan`) +proves a concrete type reached only this way now gets a plan; and +`ConfigProfileTests.cs` was changed to compose a real concrete +`RepositoryConsumer` class (with its own nested `string` dependency +satisfied by the profile's registration) instead of a bare `string`, so +the packaged sample now actually exercises the fixed path instead of +masking it. `docs/roadmap/post-mvp.md` was also corrected in the same +review round — the page's own stated purpose +(`docs/roadmap/index.md`: "not fully available") doesn't allow a shipped, +`Accepted`+`Done` capability to stay listed as an outstanding candidate; +returned to a no-current-candidates state with the historical trail +preserved via ADR-0036/RESEARCH-0002/PLAN-0036 links instead of inline +restatement. + +**PR #65's second review round caught two more real gaps, both fixed and +pushed:** + +1. **Missing seed on config/profile binder failures.** `ApplyProfile` + runs while the base class's `Lazy` is still being built — + before `GetData` ever calls `Composer.CreateRow` — so a + `ConfigProfileBinder` failure had no `CompositionRow`/`row.Seed` to + read from and escaped without the `"\n\nSeed: ..."` suffix every other + `Compono.XunitV3`-owned pre-composition failure carries (ADR-0022). + Fixed: `ApplyProfile` now catches `CompositionException` and rethrows + via the existing `CompositionException.WithSeedInMessage` helper, using + `SeedAsNullable` (the attribute's own configured seed) or a freshly + generated one otherwise — reproducibility isn't actually meaningful for + this failure category (a constructor-shape mismatch fails identically + regardless of seed), this is purely about applying the established + convention consistently. +2. **Abstract `TConfig`/`TProfile` threw the wrong exception type.** An + abstract class can still declare a public constructor (only a derived + type can call it) — `ResolveSingleConstructor`/ + `ResolveSingleProfileConstructor` would find it, pass the "exactly one + constructor" check, and then `ConstructorInfo.Invoke` would throw + `MemberAccessException` instead of the documented `CompositionException`. + Fixed: both methods now explicitly reject an abstract type with a named + `CompositionException`, checked before the constructor-count logic. + +Both fixes have dedicated `Compono.XunitV3.Tests` regression coverage +(`GetData_AppendsTheConfiguredSeed_WhenProfileConstructionFailsBeforeARowExists`, +`GetData_AppendsAGeneratedSeed_WhenProfileConstructionFailsWithNoSeedConfigured`, +`GetData_Throws_WhenConfigTypeIsAbstract`, +`GetData_Throws_WhenProfileTypeIsAbstract`) and are documented in +`docs/troubleshooting/common-errors.md`. Full solution: 903/903 passed. + +**PR #65's third review round caught two more, both edge cases of the +round-2 fixes rather than newly independent gaps — fixed and pushed:** + +1. **A negative configured seed lost to a binder failure.** Round 2's + catch-block fix used `SeedAsNullable ?? ` unconditionally — + if `SeedAsNullable` itself was negative (`Seed = -1`) *and* the + config/profile shape was also invalid, the binder failure reported + `Seed: -1` instead of the documented negative-seed diagnostic the base + `GetData` enforces for every other case. Fixed: `ApplyProfile` now + checks for a negative `SeedAsNullable` first, before attempting any + config/profile binding, throwing the identical negative-seed message + the base class uses (`AppendSeed` promoted from `private` to + `private protected` so both share the exact convention). +2. **`ConstructorInfo.Invoke` wrapping constructor-thrown exceptions.** If + `TConfig`'s or `TProfile`'s own constructor throws (e.g. custom + validation logic), reflection wraps that in `TargetInvocationException` + — `ApplyProfile`'s `catch (CompositionException)` never saw it, so a + constructor's own actionable exception was replaced by an opaque + reflection failure with no seed reporting. Fixed: + `ConfigProfileBinder`'s shared `Invoke` helper unwraps + `TargetInvocationException` via `ExceptionDispatchInfo.Capture(...).Throw()` + (preserving the original stack trace), for both the `TConfig` and + `TProfile` construction call sites. + +Regression coverage: +`GetData_ReportsTheNegativeSeedDiagnostic_NotTheBinderFailure_WhenBothApply`, +`GetData_UnwrapsAndReportsTheOriginalException_WhenTheConfigConstructorThrows`, +`GetData_UnwrapsAndReportsTheOriginalException_WhenTheProfileConstructorThrows`. +Full solution: 909/909 passed. + +**PR #65's fourth review round — three findings, two fixed, one +deliberately not actioned:** + +1. **ADR-0036 needed a dated Amendment, not a silent plan-note + correction.** This plan's own Notes already recorded that + `ConfigProfileBinder` uses direct `ConstructorInfo.Invoke`, not the + cached-delegate (`MakeGenericMethod`/`Delegate.CreateDelegate`) shape + ADR-0036's "Reflection is bounded and cached" section specified — but + per this repo's own rule (`design-decisions.md`'s Amendment mechanic), + a correction to an *already-`Accepted`* ADR's decision detail belongs + as a dated Amendment on that ADR itself, not only a plan-side note. + Fixed: added ADR-0036's Amendment 1 (2026-08-09), explaining why the + simpler direct-invocation shape still satisfies the ADR's actual + guarantee (bounded to once per attribute instance — via the base + class's existing `Lazy` caching, not a new delegate cache). +2. **Base `ComposeAttribute`'s XML docs and the NuGet package description + were stale.** `ComposeAttribute`'s remarks still said + `ComposeAttribute` was "the one designed extension point," + and `Compono.XunitV3.csproj`'s `` listed only + `[Compose]`/`[Compose]`. Fixed: updated both to describe both + extension points, and regenerated the API reference + (`docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md`) + from the corrected XML docs. +3. **Not actioned: "move the `CompositionProviderRequest.Name` migration + section to a separate PR."** A legitimate scope observation in the + abstract, but this was a deliberate, explicit instruction from the + user who commissioned this plan (not an oversight) — the original + request said, verbatim in spirit, to include RESEARCH-0002 Finding 2's + documentation gap in this same work "unless there is a strong reason + to keep it separate," and this plan's own "Documentation tasks" + section already recorded that no such reason was found. Replied on the + thread explaining this and resolved it without a code change - the + scope decision stands as the user directed it, not as this review + round would have made it unilaterally. + +Full solution after round 4: 909/909 passed, 0 warnings, 0 errors. + +**PR #65's fifth review round — three findings, all fixed, plus a +live-CI-caught regression this round's own fix introduced:** + +1. **ADR-0036's "retargeted rather than reimplemented" promise wasn't + actually kept.** `ConfigProfileBinder.BindConfig` independently + reimplemented the exact null/`Nullable`-unwrap/assignability check + `ComposeAttribute.GetData`'s own inline-value loop already had — a + correction to one would never have reached the other. Fixed: extracted + both into a new shared `Binding/PositionalArgumentBinder.cs` + (`PositionalArgumentBinder.Validate`, returning a + `PositionalArgumentValidation` enum), used by both call sites, with + message text ownership staying local to each (the two describe the + value differently — "Inline value... on..." vs. "Profile configuration + argument... of..."). +2. **No regression coverage for a non-null value-typed config argument + against a `Nullable` constructor parameter** (e.g. `42` for an + `int?`) — the plan promised this exact case per ADR-0022's own + precedent, but every existing config-binding test used a reference + type. Fixed: added `NullableIntTestConfig`/ + `NullableIntParameterizedTestProfile` fixtures and + `GetData_AcceptsANonNullValueTypeArgument_ForANullableValueTypeParameter`. +3. **The Compose-family-stacking diagnostic still named only the original + two forms.** Detection already worked correctly for the new attribute + (`GetCustomAttributes()` matches any derived type), + but `BindingPlan.ValidateSignature`'s message text hadn't been updated. + Fixed: message and its surrounding comment now name all three forms; + added `WithComposeAndTwoTypeParameterComposeAttributes` fixture and + `Build_ReportsASignatureError_ForComposeStackedWithTheTwoTypeParameterForm`. + +**Live CI regression, caught by the user, not by Codex:** round 5's own +push broke the `package-validation` workflow's "Local-feed +packed-consumer smoke test" step. That step runs +`Compono.XunitV3.SampleTests` directly (it's excluded from `Compono.slnx` +only, not from CI as a whole — an incorrect assumption baked into this +project's own code comments since round 1, now corrected everywhere it +appeared) with `--filter-not-class +"Compono.XunitV3.SampleTests.FailingCompositionTests"` to skip exactly +one known-always-failing class. `ConfigProfileTests.cs`'s own +deliberately-failing `MismatchedProfileConstructorShape_...` test lived +inside the otherwise-green `ConfigProfileTests` class, so it wasn't +excluded and failed the gate. Fixed: moved that test into its own +`FailingConfigProfileTests` class (mirroring `FailingCompositionTests`' +pattern exactly), and changed the workflow's filter to a trailing +wildcard, `--filter-not-class "Compono.XunitV3.SampleTests.Failing*"`, +covering the whole naming convention instead of one hardcoded class name +— verified directly against the built test host that the wildcard must +be trailing-only (a mid-string wildcard like `Failing*Tests` is rejected +by the MTP CLI). Verified locally with the exact corrected command: +20/20 passed. CS1591-as-error build and `docs/reference/api` freshness +(the other two `package-validation`/`docs` gates) also reverified clean. + +Full solution after round 5 + the CI fix: 913/913 passed, 0 warnings, 0 +errors. + +**PR #65's sixth review round** — one finding, a continuation of round +4's stale-docs theme: `docs/packages/index.md`'s top-level package +catalog table and `skills/compono/SKILL.md`'s Detection table/reference +table (two separate rows) still enumerated only the original two +Compose-family forms. Round 4 fixed the base attribute's XML docs and the +NuGet package description but missed these two additional discovery +surfaces. Fixed all three, then ran a repo-wide +`grep -rn '\[Compose\]/\[Compose\]'` across every `.md` file to +confirm no further stale mentions remained (none found). Full solution: +913/913 passed, unchanged (markdown-only change). + +**PR #65's seventh review round** — one finding, about this plan +document itself: as a `Done` plan, the Test Plan summary/Critical +Files/Verification-checklist sections above are the record maintainers +rely on, and they still described the pre-review-round state (stale test +counts, a "left unchanged by design" claim round 6 reversed, an "excluded +from CI" claim round 5 already corrected in code comments but not here, +and a "no `BindingPlan.cs` changes" claim round 5's own diagnostic-message +fix quietly contradicted). Fixed by editing those sections directly to +the final, true state (913/913, `PositionalArgumentBinder.cs`, +`FailingConfigProfileTests.cs`, `package-validation.yaml`, and every +other file this loop actually touched) rather than adding a seventh +dated note on top of six already-accumulated ones — this plan is a living +document, not an ADR, so a correction here is a direct edit, not an +Amendment. The round-by-round Notes entries above stay as the historical +"what changed and why, in order" trail; only the summary sections a +reader would check first for final state were stale. diff --git a/docs/plans/README.md b/docs/plans/README.md index a088157..a807769 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -52,3 +52,4 @@ one. This file is just the mechanics: numbering, status, and the index. | [0007](0007-milestone-7-dogfooding.md) | Milestone 7: Dogfooding | Done | | [0008](0008-milestone-8-public-preview.md) | Milestone 8: Public Preview | Done | | [0035](0035-compono-agent-skill-pack.md) | Compono Agent Skill Pack | Done | +| [0036](0036-call-site-values-influencing-nested-composition.md) | Call-Site Values Influencing Nested Composition | Done | diff --git a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md index 41cc30e..6cb9e41 100644 --- a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md @@ -16,15 +16,22 @@ public class ComposeAttribute : Xunit.v3.DataAttribute Inheritance [System\.Object](https://learn.microsoft.com/en-us/dotnet/api/system.object 'System\.Object') → [System\.Attribute](https://learn.microsoft.com/en-us/dotnet/api/system.attribute 'System\.Attribute') → `Xunit.v3.DataAttribute` → ComposeAttribute Derived +↳ [ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\') ↳ [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\') ### Remarks -Deliberately unsealed \- [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\') is the one designed extension -point, mirroring [AddProfile<TProfile>\(\)](../Compono/Compono.CompositionBuilder.AddProfile.md#Compono.CompositionBuilder.AddProfile_TProfile_() 'Compono\.CompositionBuilder\.AddProfile\`\`1')'s own -`TProfile : ICompositionProfile, new()` constraint\. [SupportsDiscoveryEnumeration\(\)](Compono.XunitV3.ComposeAttribute.SupportsDiscoveryEnumeration().md 'Compono\.XunitV3\.ComposeAttribute\.SupportsDiscoveryEnumeration\(\)') -returns [false](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/bool 'https://docs\.microsoft\.com/en\-us/dotnet/csharp/language\-reference/builtin\-types/bool'): composition is deferred entirely to execution time, so -[GetData\(MethodInfo, DisposalTracker\)](Compono.XunitV3.ComposeAttribute.GetData(System.Reflection.MethodInfo,Xunit.Sdk.DisposalTracker).md 'Compono\.XunitV3\.ComposeAttribute\.GetData\(System\.Reflection\.MethodInfo, Xunit\.Sdk\.DisposalTracker\)') runs for real exactly once per test execution \- there is no separate -discovery\-time composition pass to keep synchronized with it\. +Deliberately unsealed \- [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\') and +[ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\') are the two designed extension points\. +[ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\') mirrors [AddProfile<TProfile>\(\)](../Compono/Compono.CompositionBuilder.AddProfile.md#Compono.CompositionBuilder.AddProfile_TProfile_() 'Compono\.CompositionBuilder\.AddProfile\`\`1')'s +own `TProfile : ICompositionProfile, new()` constraint \(a fixed, default\-constructed +profile\); [ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\') mirrors +[AddProfile\(ICompositionProfile\)](../Compono/Compono.CompositionBuilder.AddProfile.md#Compono.CompositionBuilder.AddProfile(Compono.ICompositionProfile) 'Compono\.CompositionBuilder\.AddProfile\(Compono\.ICompositionProfile\)')'s instance\-based overload +instead \(a profile built from call\-site\-known \profile configuration arguments\ \- see +`docs/adr/0036-parameterized-composition-profile-selection.md`\)\. +[SupportsDiscoveryEnumeration\(\)](Compono.XunitV3.ComposeAttribute.SupportsDiscoveryEnumeration().md 'Compono\.XunitV3\.ComposeAttribute\.SupportsDiscoveryEnumeration\(\)') returns [false](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/bool 'https://docs\.microsoft\.com/en\-us/dotnet/csharp/language\-reference/builtin\-types/bool'): composition is +deferred entirely to execution time, so [GetData\(MethodInfo, DisposalTracker\)](Compono.XunitV3.ComposeAttribute.GetData(System.Reflection.MethodInfo,Xunit.Sdk.DisposalTracker).md 'Compono\.XunitV3\.ComposeAttribute\.GetData\(System\.Reflection\.MethodInfo, Xunit\.Sdk\.DisposalTracker\)') runs for real exactly once per +test execution \- there is no separate discovery\-time composition pass to keep synchronized with +it\. | Constructors | | | :--- | :--- | diff --git a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.ComposeAttribute(object[]).md b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.ComposeAttribute(object[]).md new file mode 100644 index 0000000..e844a6f --- /dev/null +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.ComposeAttribute(object[]).md @@ -0,0 +1,21 @@ +#### [Compono\.XunitV3](index.md 'index') +### [Compono\.XunitV3](Compono.XunitV3.md 'Compono\.XunitV3').[ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\') + +## ComposeAttribute\(object\[\]\) Constructor + +Creates a [ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\')\. + +```csharp +public ComposeAttribute(params object?[] configArguments); +``` +#### Parameters + + + +`configArguments` [System\.Object](https://learn.microsoft.com/en-us/dotnet/api/system.object 'System\.Object')[\[\]](https://learn.microsoft.com/en-us/dotnet/api/system.array 'System\.Array') + +Profile configuration arguments, bound positionally to [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig')'s single +public constructor \- an entirely separate binding target from this attribute family's ordinary +inline values; every test method parameter is composed in full regardless of what's supplied +here\. See the type\-level remarks for why each argument should use the strongest attribute\-legal +type available rather than a bare string\. \ No newline at end of file diff --git a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md new file mode 100644 index 0000000..9e59689 --- /dev/null +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md @@ -0,0 +1,62 @@ +#### [Compono\.XunitV3](index.md 'index') +### [Compono\.XunitV3](Compono.XunitV3.md 'Compono\.XunitV3') + +## ComposeAttribute\ Class + +Composes an xUnit v3 theory row's parameters through Compono, applying a profile built from +\profile configuration arguments\ known at this attribute's call site \- a distinct concept +from this attribute family's ordinary inline values \([ComposeAttribute\(object\[\]\)](Compono.XunitV3.ComposeAttribute.ComposeAttribute(object[]).md 'Compono\.XunitV3\.ComposeAttribute\.ComposeAttribute\(object\[\]\)')\), +which bind to the test method's own parameters instead\. This constructor never binds to the test +method's parameters at all; every one of them is composed in full\. [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig') is +constructed positionally from this attribute's own constructor arguments, then +[TProfile](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TProfile 'Compono\.XunitV3\.ComposeAttribute\\.TProfile') is constructed from that [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig') instance and +applied via [AddProfile\(ICompositionProfile\)](../Compono/Compono.CompositionBuilder.AddProfile.md#Compono.CompositionBuilder.AddProfile(Compono.ICompositionProfile) 'Compono\.CompositionBuilder\.AddProfile\(Compono\.ICompositionProfile\)') \- equivalent to +`Composer.Create(builder => builder.AddProfile(new TProfile(new TConfig(...))))`\. See +`docs/adr/0036-parameterized-composition-profile-selection.md` for the full design, including +why this exists as a separate attribute rather than overloading +[ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\')'s own inline\-value constructor argument\. + +```csharp +public sealed class ComposeAttribute : Compono.XunitV3.ComposeAttribute + where TProfile : Compono.ICompositionProfile +``` +#### Type parameters + + + +`TProfile` + +The profile to construct and apply\. Must have exactly one public constructor accepting exactly one +[TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig')\-typed parameter \- no `new()` constraint, unlike +[ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\'), since this form is never default\-constructed\. + + + +`TConfig` + +The type this attribute's constructor arguments bind to, positionally, against its own single +public constructor\. Prefer strongly\-typed, attribute\-legal values for its constructor parameters \- +an [enum](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/enum 'https://docs\.microsoft\.com/en\-us/dotnet/csharp/language\-reference/keywords/enum') for a finite choice, [System\.Type](https://learn.microsoft.com/en-us/dotnet/api/system.type 'System\.Type') via `typeof(...)` for a CLR +type, a plain [bool](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/bool 'https://docs\.microsoft\.com/en\-us/dotnet/csharp/language\-reference/keywords/bool')/numeric/string value where that already carries the real +meaning \- over loosely\-typed primitives standing in for something more specific\. +`params object?[]` is a binding mechanism forced by C\#'s attribute\-argument\-must\-be\-a\- +compile\-time\-constant rule, not a license to design [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig') around magic +strings\. + +Inheritance [System\.Object](https://learn.microsoft.com/en-us/dotnet/api/system.object 'System\.Object') → [System\.Attribute](https://learn.microsoft.com/en-us/dotnet/api/system.attribute 'System\.Attribute') → `Xunit.v3.DataAttribute` → [ComposeAttribute](Compono.XunitV3.ComposeAttribute.md 'Compono\.XunitV3\.ComposeAttribute') → ComposeAttribute\ + +### Remarks +Unlike [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\')'s compile\-time\-enforced `new()` constraint, an +unsupported [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig')/[TProfile](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TProfile 'Compono\.XunitV3\.ComposeAttribute\\.TProfile') constructor shape \(not +exactly one public constructor on [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig'); no exactly\-one\- +[TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig')\-parameter public constructor on [TProfile](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TProfile 'Compono\.XunitV3\.ComposeAttribute\\.TProfile')\) is a +deterministic runtime [CompositionException](../Compono/Compono.CompositionException.md 'Compono\.CompositionException'), not a compile error \- there is no C\# +generic constraint that expresses "has a constructor accepting exactly this type\." Both constructor +lookups, and the actual construction, are reflection \(`Compono.XunitV3.Binding.ConfigProfileBinder`\) \- bounded +and cached to once per attribute instance by this attribute family's existing +[System\.Lazy<>](https://learn.microsoft.com/en-us/dotnet/api/system.lazy-1 'System\.Lazy\`1')\-backed [Composer](../Compono/Compono.Composer.md 'Compono\.Composer') caching \(`Compono.XunitV3.ComposeAttribute<>.ApplyProfile(Compono.CompositionBuilder)` is only ever +invoked from inside that lazy initializer\), never on the repeated per\-row `GetData` path\. + +| Constructors | | +| :--- | :--- | +| [ComposeAttribute\(object\[\]\)](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.ComposeAttribute(object[]).md 'Compono\.XunitV3\.ComposeAttribute\\.ComposeAttribute\(object\[\]\)') | Creates a [ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\')\. | diff --git a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.md b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.md index eb071c3..0ac87da 100644 --- a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.md +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.md @@ -5,5 +5,6 @@ | Classes | | | :--- | :--- | | [ComposeAttribute](Compono.XunitV3.ComposeAttribute.md 'Compono\.XunitV3\.ComposeAttribute') | Composes an xUnit v3 theory row's parameters through Compono \- the default \(no explicit profile\) entry point\. Every parameter not supplied inline is composed; a parameter targeted by a supplied inline value takes that value instead, taking precedence over composition\. See `docs/adr/0022-compono-xunit-package-design.md` for the full binding algorithm, seed policy, and diagnostics\. | +| [ComposeAttribute<TProfile,TConfig>](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md 'Compono\.XunitV3\.ComposeAttribute\') | Composes an xUnit v3 theory row's parameters through Compono, applying a profile built from \profile configuration arguments\ known at this attribute's call site \- a distinct concept from this attribute family's ordinary inline values \([ComposeAttribute\(object\[\]\)](Compono.XunitV3.ComposeAttribute.ComposeAttribute(object[]).md 'Compono\.XunitV3\.ComposeAttribute\.ComposeAttribute\(object\[\]\)')\), which bind to the test method's own parameters instead\. This constructor never binds to the test method's parameters at all; every one of them is composed in full\. [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig') is constructed positionally from this attribute's own constructor arguments, then [TProfile](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TProfile 'Compono\.XunitV3\.ComposeAttribute\\.TProfile') is constructed from that [TConfig](Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md#Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.TConfig 'Compono\.XunitV3\.ComposeAttribute\\.TConfig') instance and applied via [AddProfile\(ICompositionProfile\)](../Compono/Compono.CompositionBuilder.AddProfile.md#Compono.CompositionBuilder.AddProfile(Compono.ICompositionProfile) 'Compono\.CompositionBuilder\.AddProfile\(Compono\.ICompositionProfile\)') \- equivalent to `Composer.Create(builder => builder.AddProfile(new TProfile(new TConfig(...))))`\. See `docs/adr/0036-parameterized-composition-profile-selection.md` for the full design, including why this exists as a separate attribute rather than overloading [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\')'s own inline\-value constructor argument\. | | [ComposeAttribute<TProfile>](Compono.XunitV3.ComposeAttribute_TProfile_.md 'Compono\.XunitV3\.ComposeAttribute\') | Composes an xUnit v3 theory row's parameters through Compono, with [TProfile](Compono.XunitV3.ComposeAttribute_TProfile_.md#Compono.XunitV3.ComposeAttribute_TProfile_.TProfile 'Compono\.XunitV3\.ComposeAttribute\\.TProfile') applied to the underlying [Composer](../Compono/Compono.Composer.md 'Compono\.Composer') \- equivalent to `Composer.Create(builder => builder.AddProfile())`\. See [ComposeAttribute](Compono.XunitV3.ComposeAttribute.md 'Compono\.XunitV3\.ComposeAttribute') for the full binding algorithm\. | | [SharedAttribute](Compono.XunitV3.SharedAttribute.md 'Compono\.XunitV3\.SharedAttribute') | Marks a `[Compose]`\-attributed test method parameter as shared: its composed \(or inline\-supplied\) value is stored in the row's [CompositionRow](../Compono/Compono.CompositionRow.md 'Compono\.CompositionRow') scope, so any other composed parameter or nested generated dependency that structurally requests the same type in the same row reuses this exact value instead of composing its own independent one\. | diff --git a/docs/research/0002-trivia-platform-comparison.md b/docs/research/0002-trivia-platform-comparison.md new file mode 100644 index 0000000..506e06b --- /dev/null +++ b/docs/research/0002-trivia-platform-comparison.md @@ -0,0 +1,283 @@ +# [RESEARCH-0002] AutoFixture vs. Compono: `trivia-platform` Pre-Migration Capability Survey + +**Status:** Done (survey complete; no migration performed — see "Scope" below) + +**Feeds:** [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) + +This document is the evidence record for a pre-migration capability-gap +survey of `ncipollina/trivia-platform`'s AutoFixture-based test kit, +following `design-decisions.md`'s `docs/research/` convention and reusing +[ADR-0029](../adr/0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md)'s +rubric/five-way classification framework. Unlike +[RESEARCH-0001](0001-autofixture-comparison.md) (a full Milestone-7 +dogfooding migration of `cosmere-tracker`), this is a lighter-weight +**survey, not a migration** — no `Compono` package reference exists +anywhere in `trivia-platform` (confirmed via `grep -rl "Compono" +--include="*.csproj" .` returning nothing at the time of this survey), and +no code in either repo changed as part of it. The rubric's four questions +are therefore answered qualitatively, from Compono's documented model and +`trivia-platform`'s existing AutoFixture call sites, rather than from an +actual before/after migration diff. + +## Scope + +`trivia-platform` is a much larger and more elaborate AutoFixture test kit +than `cosmere-tracker`'s — seven layered `TestKit` projects (one base kit, +one per platform integration — Alexa, APL, DynamoDB — and one per gameplay +module — Announcements, Commerce, Gameplay, Leaderboard — plus an infra +test kit), roughly sixteen custom `AutoDataAttribute` subclasses, and +several actively-used (not zero-call-site, unlike `cosmere-tracker`'s +`HttpClientSpecimenBuilder`) request-specification/specimen-builder pairs. +The survey read every file under each kit's +`Attributes/`/`Customizations/`/`SpecimenBuilders/`/ +`RequestSpecifications/`/`Extensions/` folder, then sampled 2-3 real test +files per consuming module to confirm frequency claims against actual call +sites. + +## Inventory + +| Mechanism | Module(s) | What it does | +|---|---|---| +| `BaseFixtureFactory` | core | `Fixture()` + `OmitOnRecursionBehavior` + `AutoNSubstituteCustomization{ConfigureMembers=true}` | +| `DisneyTriviaAutoDataAttribute`/`Inline...` | core | Thin `AutoDataAttribute` wrapper around `BaseFixtureFactory` | +| `LazySpecimenBuilder` | core/DynamoDb | Open-generic `Lazy` match, reflectively builds `Lazy` deferring to `context.Resolve(typeof(T))`; 1 real call site (`Lazy`) | +| `HandlerInputSpecification`/`ResponseBuilderSpecification`/`AttributesManagerSpecification` + matching `SpecimenBuilder`s | Platform.Alexa | Exact-`Type` dispatch on `IHandlerInput`/`IResponseBuilder`/`IAttributesManager`, heavily used (30–45+ call sites via `HandlerAutoData`/`PresenterAutoData`/`InterceptorAutoData`) | +| `DocumentBuilderSpecification`/`DocumentBuilderSpecimenBuilder` | Platform.Apl | Exact `typeof(IDocumentBuilder)` | +| `IntentNameSpecimenBuilder`, `JsonAttributeBagSpecimenBuilder`, `JsonElementSpecimenBuilder` | Platform.Alexa | Inline exact-`Type`-equality dispatch | +| `ResponseModelSpecification`/`ResponseModelSpecimenBuilder` | Commerce | Set-membership over 3 known types | +| `SlotSpecification`/`SlotSpecimenBuilder` | Gameplay | Matches on member **name + type** (`PropertyInfo{Name:"Slots"}`, type `Dictionary`), regardless of declaring type | +| `ProductSpecimenBuilder`, `UpsellPayloadSpecimenBuilder`, `LeaderboardEntrySpecimenBuilder` | Commerce/Leaderboard | Parameter-name-polymorphic dispatch (`pi.Name.Contains(...)`/`switch` on `pi.Name`) — several distinct values for the same declared type, chosen by parameter name | +| `GameplayIntentSpecimenBuilder`, `DynamoDbOptionsSpecimenBuilder`, `AnnouncementsOptionsSpecimenBuilder`, `RequestSpecimenBuilder` | various | Exact-type, constructor-configured | +| `GamePlayStateCustomization`, `SkillLocalizerCustomization`, `SkillThemeCustomization`, `TimeProviderCustomization`, `ConversationalContextCustomization` | various | Exact-type: build/inject one substitute or fixed value | +| `SlotCustomization`, `UserEventArgumentsCustomization`, `UserEventAnswerArgumentsCustomization`, `SkillRequestCustomization`, `InfraStackCustomization` | various | Member-level `.With(...)` override chains | +| `HandlerFixtureExtensions`, `CommerceFixtureExtensions`, `GameplayFixtureExtensions`, `LeaderboardFixtureExtensions`, `DynamoDbFixtureExtensions` | all | `IFixture` extension methods, called only from `AutoDataAttribute` constructors — declarative setup-time wiring, never mid-test | +| ~16 `*AutoDataAttribute` subclasses | all modules | Each wires the above together; **most take runtime constructor literals** that parameterize the underlying customization/specimen-builder logic per call site (see Finding 1) | + +## Findings + +Reusing ADR-0029's five-way classification (Bug / Roadmap candidate / +Acceptable Compono-native alternative / Intentional design difference / +Migration-only friction). "Bug" is not a reachable classification here — +no Compono code path was ever exercised against this repo, so there is +nothing to be a defect in. + +### Finding 1 — Parameterized custom AutoData attributes (roadmap candidate) + +The one genuinely new finding this survey surfaces, not exercised by +`cosmere-tracker`'s dogfooding pass. + +- **Frequency:** pervasive. `PersistenceAutoData(repositoryName)` — ~45 + call sites, each a different repository name; + `AnnouncementsAutoData(validConfig, gameOverEnabled, audienceEnabled, + audienceItemEnabled, startOffsetDays, endOffsetDays, messageLocale, + defaultLocale)` — 8 constructor parameters, 18 call sites, each a + different boolean/locale combination; `HandlerAutoData`/ + `InterceptorAutoData`/`PresenterAutoData(requestType, aplSupported, + locale, ...)` — hundreds of call sites; `InfraStackAutoData(region, + account)`. Nearly every one of the ~16 custom attribute subclasses takes + constructor arguments that change what the underlying customization or + specimen builder actually produces, not just which type gets composed. +- **Compono's documented answer today:** none, and the gap is narrower + than it first looks. Two adjacent capabilities are already solved and + are *not* this finding: requested-type-plus-resolution-site-name + matching (`CompositionProviderRequest.Name`, an `ICompositionValueProvider` — + see Finding 2 below) and fixed member-specific overrides + (`.For().Member(...)`). What's actually missing is a way for a + compile-time-constant value known at a specific test's call site to + reach a composition decision made deeper in that test's own graph — + `[Compose]` selects a fixed, compile-time profile *type* with + no documented way to carry a call-site value into it, and + `[Compose(42, "widget")]`'s inline-value binding binds *test method + parameters* positionally, not a value used inside nested configuration + logic. +- **Workaround cost:** real and structural, not cosmetic. Compono's + current model offers two workarounds — a dedicated `ICompositionProfile` + subclass per distinct configuration variant, or hand-building + `Composer.Create(builder => ...)` inline in the test body — and both + remain technically possible even at `AnnouncementsAutoData`'s 8-flag + argument space. The actual cost is that neither preserves the concise, + declarative attribute-based idiom (`[Compose]` on the method, + real composed values in the signature) without substantial duplication: + a profile-per-variant approach means a new subclass for every argument + combination a test actually needs, and the inline-`Composer.Create` + fallback reintroduces per-test setup code the profile idiom exists to + eliminate. Neither is a hard wall; both are a real, recurring tax on + every call site this pattern touches. +- **Principle-alignment note:** doesn't obviously require reflection or + hidden state — attribute constructor arguments are already compile-time + constants, so a shape that threads a call-site-known constant into + nested composition configuration wouldn't conflict with + [ADR-0001](../adr/0001-source-generation-first.md)'s no-reflection-by-default + posture. This is new territory for Compono (nothing today lets a + call-site value reach configuration logic that runs deeper than a + top-level `[Compose(...)]`-bound parameter), not a rejection of an + existing principle. +- **Classification: roadmap candidate.** High frequency, real material + cost, no identified principle conflict — recorded as + [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md), + now `Accepted` following a deep-design pass (a typed `TConfig` object + paired with the profile, implemented entirely in `Compono.XunitV3`; see + [PLAN-0036](../plans/0036-call-site-values-influencing-nested-composition.md)). + +### Finding 2 — Parameter/member-name-polymorphic specimen builders (acceptable alternative, documentation gap) + +`ProductSpecimenBuilder`, `UpsellPayloadSpecimenBuilder`, +`LeaderboardEntrySpecimenBuilder` (parameter-name `switch`/`Contains` +dispatch) and `SlotSpecification`/`SlotSpecimenBuilder` (member-name+type +match, declaring-type-agnostic). + +- **Frequency:** 4 mechanisms, moderate-to-heavy real call sites (e.g. + `NewGameUpsellHandlerTests`, `UpsellYesHandlerTests`, + `LockedPackUpsellHandlerTests` for `UpsellPayload`; `SlotSpecimenBuilder` + reachable through `GameplayHandlerAutoData`'s 127 call sites). +- **Compono's answer:** `CompositionProviderRequest.Name` exposes the + declaring constructor parameter/required member/test-method-parameter's + own name for exactly this purpose — a custom `ICompositionValueProvider` + checking `request.RequestedType == typeof(UpsellPayload) && + request.Name == "newGamePayload"` is the documented use case in + `docs/concepts/providers.md` for shape-based (not fixed-type) matching. +- **Workaround cost:** real but moderate — one hand-written provider per + polymorphic-by-name family, replacing an `ISpecimenBuilder` (plus, + sometimes, a separate `IRequestSpecification`) with a single + `ICompositionValueProvider` of similar shape. +- **Classification: acceptable Compono-native alternative.** No ADR/ + Amendment needed — there's no decision to make, just a pattern worth + adding to the migration guide, since it currently doesn't call out + `Name`-based provider matching as a first-class pattern and this is the + first real evidence it's needed. + +### Finding 3 — `Lazy` support (acceptable alternative) + +`LazySpecimenBuilder`, 1 real call site (`Lazy`). No +built-in Compono `Lazy` support exists, but +`Register>(context => new(() => +context.Resolve()))` is trivial — registration bypasses +constructor selection entirely, so `Lazy`'s multiple constructors never +risk `CMP0001`. **Classification: acceptable Compono-native alternative** — +single closed type, zero generalized `Lazy` need observed. + +### Finding 4 — SDK-interface specimen builders at scale (acceptable alternative) + +`HandlerInputSpecimenBuilder`/`ResponseBuilderSpecimenBuilder`/ +`AttributesManagerSpecimenBuilder`/`DocumentBuilderSpecimenBuilder` — all +exact-type dispatch on interfaces (`IHandlerInput`, `IResponseBuilder`, +`IAttributesManager`, `IDocumentBuilder`), heavily used. Interfaces are +always provider-resolved in Compono, never hit `CMP0001`; concrete SDK +construction happens inside an ordinary `Register` factory body, not +through compile-time constructor selection, so multi-constructor SDK types +carry no ambiguity risk there either. **Classification: acceptable +Compono-native alternative** — a stronger positive data point than +`cosmere-tracker`'s zero-call-site `HttpClientSpecimenBuilder`: this +pattern is real, heavily-exercised infrastructure that maps cleanly to +`Register` at scale. + +### Finding 5 — NSubstitute `ConfigureMembers` recurrence (no new evidence) + +`BaseFixtureFactory` applies the same `AutoNSubstituteCustomization +{ConfigureMembers=true}` `cosmere-tracker`'s did. No new evidence toward a +different verdict than +[ADR-0025](../adr/0025-compono-nsubstitute-package-design.md)'s existing +Amendment — same expected migration cost (explicit stubs where a test +silently relied on an auto-configured return value), at larger scale. +**Classification: intentional design difference**, recurring — not a new +finding, no new Amendment written from this survey alone. + +### Finding 6 — `OmitOnRecursionBehavior` recurrence (no new evidence) + +Same swap present in `BaseFixtureFactory`; no genuinely self-referencing +object graph identified (Gameplay's `GameState`↔`Question` linkage is +one-directional via `context.Create()`, not a cycle). +**Classification: intentional design difference**, still unexercised, +consistent with [ADR-0011](../adr/0011-composition-scope-shared-values-and-recursion-detection.md). + +### Finding 7 — Compose-family attribute stacking: still unexercised + +Every sampled test method uses exactly one `*AutoData` attribute; the +"stacking many customizations" pattern happens *inside* one attribute's +constructor logic (e.g. `GameplayHandlerAutoDataAttribute` wires +Alexa-handler-input + `SlotCustomization` + localizer + +`GamePlayStateCustomization` + `ConversationalContextCustomization`), +mapping cleanly to one Compono profile with many `Register`/`.For()`/ +`UseNSubstitute()` calls in one `Configure` method — not to stacking two +*different* Compose-family attributes on one test method. +**Classification: intentional design difference, still zero real call +sites** — consistent with `cosmere-tracker`'s own Finding 4; no new +evidence either direction. + +### Finding 8 — Exact-type/member-level customizations (the bulk of the kit) + +`GamePlayStateCustomization`, `SkillLocalizerCustomization`, +`SkillThemeCustomization`, `TimeProviderCustomization`, +`ConversationalContextCustomization`, `SlotCustomization`, +`UserEventArgumentsCustomization`, `UserEventAnswerArgumentsCustomization`, +`SkillRequestCustomization`, `InfraStackCustomization`, and +`ResponseModelSpecimenBuilder`'s 3-type set all map directly to +`Register` (exact-type) or `.For().Member(...)` (member override), +including ones with non-trivial construction logic inside the factory. +**Classification: acceptable Compono-native alternative**, already fully +covered by the existing migration guide — no new pattern. + +### Finding 9 — ADR-0017 immutable-builder concern: cleared + +Explicitly checked: grepped for `.Customize(`/`.Register(`/`.Inject(` +inside `[Fact]`/`[Theory]` bodies across every sampled module. Zero hits — +every customization happens declaratively at `AutoDataAttribute`- +construction time (the direct analog of a profile's `Configure` running +once before composition), never mid-test. `trivia-platform`'s kit is +disciplined the same way `cosmere-tracker`'s was; nothing here conflicts +with [ADR-0017](../adr/0017-immutable-composer-configuration-and-builder-model.md)'s +frozen-configuration decision. **No classification needed** — confirmed +non-conflict, not a finding. + +### Finding 10 — Duplicated bootstrap logic (project-local cleanup) + +`AnnouncementsAutoDataAttribute` and `InfraStackAutoDataAttribute` bypass +`DisneyTriviaAutoDataAttribute`/`BaseFixtureFactory`, duplicating the +`Fixture()` + behavior + `AutoNSubstituteCustomization` bootstrap inline +rather than reusing it; localizer-substitute setup is independently +copy-pasted across Commerce's and Leaderboard's fixture extensions rather +than shared. **Classification: migration-only friction** — a real +migration would naturally collapse these into one shared profile plus +`AddProfile()` composition, the same simplification `cosmere-tracker`'s +three-tier-stack finding already demonstrated. Not a Compono capability +question. + +### Finding 11 — Testcontainers-backed real client injection (non-finding) + +`DynamoDbFixtureExtensions.AddDynamoDbPersistence` wires a real +`IAmazonDynamoDB` from a running test container, not an AutoFixture-specific +concept at all — trivially `Register(_ => +DynamoContainerFixture.CurrentClient)`, orthogonal to the framework choice. + +## Decisions + +- **Finding 1** → [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) + (`Accepted`, implementation tracked in + [PLAN-0036](../plans/0036-call-site-values-influencing-nested-composition.md)) — + the only finding from this survey promoted to a new ADR. +- **Findings 2-4, 8** → no ADR/Amendment; each is an acceptable + Compono-native alternative already covered by existing documentation + (Finding 2 additionally flags a migration-guide documentation gap — + `Name`-based provider matching isn't currently called out as a pattern). +- **Findings 5-7** → no new Amendment; each recurs an already-`Accepted` + verdict ([ADR-0025](../adr/0025-compono-nsubstitute-package-design.md), + [ADR-0011](../adr/0011-composition-scope-shared-values-and-recursion-detection.md), + [ADR-0022](../adr/0022-compono-xunit-package-design.md)) with no new + evidence in either direction. +- **Finding 9** → confirms no conflict with + [ADR-0017](../adr/0017-immutable-composer-configuration-and-builder-model.md); + no action. +- **Findings 10-11** → project-local, not recorded against any Compono + ADR. + +## Links + +- [ADR-0029](../adr/0029-milestone-7-dogfooding-strategy-and-capability-gap-decision-framework.md) — + the rubric/classification framework this survey reuses +- [RESEARCH-0001](0001-autofixture-comparison.md) — the prior, full + dogfooding migration (`cosmere-tracker`) this survey's findings are + compared against +- [migrating-from-autofixture.md](../migrating-from-autofixture.md) — the + migration guide; Finding 2 flags a gap in its provider-matching coverage +- `ncipollina/trivia-platform` — the repo surveyed; not part of this + monorepo, no code there was changed by this survey diff --git a/docs/roadmap/post-mvp.md b/docs/roadmap/post-mvp.md index 9212e11..62a00b6 100644 --- a/docs/roadmap/post-mvp.md +++ b/docs/roadmap/post-mvp.md @@ -14,19 +14,37 @@ record and their governing ADR's Amendments, not here. ## Current state: no roadmap candidates -Milestone 7's dogfooding pass (migrating `ncipollina/cosmere-tracker`'s -AutoFixture-based test kit to Compono) surfaced ten findings. **None were -classified roadmap candidate** — every finding's evidence pointed toward -Compono's existing model already being the right answer, a project-local -fix, or an unexercised theoretical constraint, not a missing capability. +Per `docs/roadmap/index.md`, this page is a status-filtered index of +capability gaps that are **not yet available** — a shipped capability +doesn't stay listed here once it's implemented, even though the evidence +that motivated it remains a permanent part of the record elsewhere (the +ADR, the research doc, the plan). Two dogfooding passes have run so far: -A dogfooding pass that surfaces zero roadmap candidates is itself a real, -evidence-backed outcome, not a shortfall in the process — see -[RESEARCH-0001](../research/0001-autofixture-comparison.md)'s -"Classifications (Phase 3)" and "Decisions" sections for the full -per-finding reasoning and which ADR Amendment (if any) recorded each -verdict. That doesn't mean Compono is "done": a different real-world -project, or a future package, may surface findings this one didn't -(`cosmere-tracker`'s domain, scale, and test patterns are one data point, -not an exhaustive survey) — but there is nothing to list here as of this -milestone. +- Milestone 7's pass (migrating `ncipollina/cosmere-tracker`'s + AutoFixture-based test kit to Compono) surfaced ten findings, **none** + classified roadmap candidate — every finding's evidence pointed toward + Compono's existing model already being the right answer, a project-local + fix, or an unexercised theoretical constraint, not a missing capability. + See [RESEARCH-0001](../research/0001-autofixture-comparison.md)'s + "Classifications (Phase 3)" and "Decisions" sections for the full + per-finding reasoning. +- A subsequent pre-migration capability survey of + `ncipollina/trivia-platform`'s (much larger) AutoFixture test kit — see + [RESEARCH-0002](../research/0002-trivia-platform-comparison.md) — + surfaced one finding classified roadmap candidate: **call-site values + influencing nested composition**, motivated by `trivia-platform`'s + parameterized custom `AutoDataAttribute` subclasses (e.g. + `PersistenceAutoData(repositoryName)`, ~45 call sites). That finding is + no longer a candidate — it's been designed, `Accepted`, and shipped: + [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) + records the decision, [PLAN-0036](../plans/0036-call-site-values-influencing-nested-composition.md) + (`Done`) records the implementation, and + [`Compono.XunitV3`'s Package Guide](../packages/compono-xunitv3.md#profile-configuration-arguments) + is the current-state usage documentation — `ComposeAttribute` + is available today, not planned. + +That two dogfooding passes together produced zero *outstanding* roadmap +items is itself a real, evidence-backed outcome, not a shortfall in the +process — it doesn't mean Compono is "done": a different real-world +project, or a future package, may surface a finding neither of these two +did (each is one data point, not an exhaustive survey). diff --git a/docs/troubleshooting/common-errors.md b/docs/troubleshooting/common-errors.md index dc28c40..45f0ba7 100644 --- a/docs/troubleshooting/common-errors.md +++ b/docs/troubleshooting/common-errors.md @@ -86,6 +86,56 @@ conflict found is collected and reported together, not just the first one. Fix: remove the duplicate/contradictory configuration call — there is no last-write-wins fallback to rely on instead. +### "`ComposeAttribute` throws before my test even runs" + +Five distinct, deterministic, pre-composition failures — all plain-message +`CompositionException`s, computed once per attribute instance and cached, +never re-checked per theory row (see +[`Compono.XunitV3`'s Package Guide](../packages/compono-xunitv3.md#profile-configuration-arguments) +and [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) +for the full design). Every one of these ends with the same `"\n\nSeed: +{value}"` convention every other `Compono.XunitV3`-owned pre-composition +failure uses — even though a constructor-shape/argument mismatch fails +identically regardless of seed, the seed printed is either the one you +configured via `Seed = ...` or a freshly generated one, for consistency +with every other failure category: + +- **`'{TConfig}' must have exactly one public constructor...`** — the + config type you passed as `TConfig` has zero or more than one public + constructor. There's no "best match" heuristic here by design — reduce + `TConfig` to exactly one public constructor. +- **`'{TConfig}' is abstract and cannot be used as profile + configuration...`** — `TConfig` is an abstract class. Even if it has + exactly one public constructor (abstract types can declare one; only a + derived type can actually call it), it can't be instantiated directly — + use a concrete type. +- **`'{TProfile}' must have exactly one public constructor accepting a + single '{TConfig}' parameter...`** — the profile type has no + constructor accepting exactly one `TConfig`-typed parameter (or has more + than one, which normal C# overload resolution can't actually produce for + an identical single-parameter shape, so this case is effectively + unreachable in practice). Add a public constructor to your profile that + takes a single `TConfig` parameter. +- **`'{TProfile}' is abstract and cannot be used as a profile...`** — same + reasoning as `TConfig`'s abstract-rejection case above, applied to the + profile type. +- **`'{TConfig}' requires {N} profile configuration argument(s), but {M} + were supplied.`** / a null-for-non-nullable or type-mismatch message — + the attribute's own constructor arguments don't match `TConfig`'s + constructor positionally. Unlike this attribute family's ordinary inline + values (which may supply fewer than the test method has parameters, + leaving the rest composed), profile configuration arguments must match + `TConfig`'s constructor **exactly** — there's no "leave the rest to + composition" fallback for a config type's own constructor parameters. + +Note the compile-time-vs-runtime tradeoff this form makes deliberately: +`[Compose]` (no `TConfig`) rejects an invalid profile type at +**compile time**, via its `TProfile : ICompositionProfile, new()` +constraint — `[Compose]` can't offer that, since "has a +constructor accepting exactly this type" isn't expressible as a C# generic +constraint. All three failures above are deterministic runtime checks +instead, computed once and cached, but not compile errors. + ### "A composed value doesn't look realistic" (looks like an anonymous string) `Compono.Bogus` isn't installed, or `UseBogus()` wasn't called, or the diff --git a/skills/compono-evals/evals.json b/skills/compono-evals/evals.json index 09129ce..49ecd1a 100644 --- a/skills/compono-evals/evals.json +++ b/skills/compono-evals/evals.json @@ -201,6 +201,19 @@ "Does not use or recommend UseBogus()/BogusOptions/any Compono.Bogus API without that package being referenced or explicitly requested", "Correctly identifies that realistic-data generation requires Compono.Bogus specifically, not core Compono" ] + }, + { + "id": 19, + "category": "migration", + "prompt": "Convert this parameterized AutoFixture custom AutoDataAttribute to Compono. It takes a RepositoryKind-shaped argument (currently a string constant) at each call site and configures a different repository customization per call.", + "expected_output": "Recognizes this as the profile-configuration-arguments pattern: a TConfig record (using an enum, not the original string) paired with a profile via [Compose(...)], not a combinatorial set of profile subclasses, a per-test Composer.Create(...) escape hatch, invented ambient/global scenario state, or a recommendation to keep the AutoFixture attribute.", + "files": [], + "expectations": [ + "Proposes [Compose] specifically, not [Compose] with no way to pass the value, and not a new attribute-per-argument-combination subclass", + "Uses an enum (or other attribute-legal typed value) for the finite-choice argument, not a magic string, per the no-stringly-typed-configuration principle", + "Does not suggest ambient/global mutable scenario state, a per-test hand-built Composer.Create(...) as the primary recommendation, or retaining the AutoFixture attribute", + "Correctly distinguishes this from inline values (which bind to the test method's own parameters) and does not conflate the two" + ] } ] } \ No newline at end of file diff --git a/skills/compono/SKILL.md b/skills/compono/SKILL.md index d3eeed7..0f730a6 100644 --- a/skills/compono/SKILL.md +++ b/skills/compono/SKILL.md @@ -41,7 +41,7 @@ some packages and not others. | Signal | Where to look | Confidence | Meaning | |---|---|---|---| | `]`/`[Shared]` available — load `references/xunit-v3.md` | +| `]`/`[Compose]`/`[Shared]` available — load `references/xunit-v3.md` | | `()` available — load `references/bogus.md` | | `Composer.Create(`, `.Create<`, `.CreateMany<`, `CompositionBuilder` | `*.cs` | High | Core Compono API in active use | @@ -93,6 +93,15 @@ user to make test-by-test, not something to do as a drive-by. generated values are fine when realism doesn't matter to the test. - Cross-test/cross-project reusable setup → an `ICompositionProfile`, not a copy-pasted builder lambda in every test. + - A value only known at a *specific test's call site* that must + influence configuration logic running *inside* a profile (not a + top-level test parameter) → `Compono.XunitV3`'s + `[Compose]`, if that package is referenced — see + `references/xunit-v3.md`. Prefer an enum/`typeof(...)` over a bare + string for the argument. Don't confuse this with a + `CompositionProviderRequest.Name`-based custom provider + (`references/registrations-profiles-and-scopes.md`), which solves a + different (name-based, not call-site) selection problem. 4. **Check `[Composable]` necessity** — see `references/composition-model.md`'s Discovery section. Most types need nothing; only add it when the type has no local `Create()`/ @@ -189,7 +198,7 @@ Load only what the Detection table says is relevant to the current task. | `references/composition-model.md` | Composing a type, deciding on `[Composable]`, understanding generated-plan discovery, or anything about determinism/seeding | | `references/registrations-profiles-and-scopes.md` | Using `Register()`, `.For().Use()`/`.Member()`, `ICompositionProfile`, `[Shared]`, or debugging a recursion/registration-conflict error | | `references/diagnostics.md` | A `CMP0001`-`CMP0012` build error, or a runtime `CompositionException` needs diagnosing | -| `references/xunit-v3.md` | `Compono.XunitV3` is referenced — `[Compose]`/`[Compose]`/`[Shared]` theory work | +| `references/xunit-v3.md` | `Compono.XunitV3` is referenced — `[Compose]`/`[Compose]`/`[Compose]`/`[Shared]` theory work | | `references/nsubstitute.md` | `Compono.NSubstitute` is referenced — `UseNSubstitute()` work | | `references/bogus.md` | `Compono.Bogus` is referenced — `UseBogus()`/`UseBogus()` work | | `references/patterns-and-antipatterns.md` | Reviewing existing Compono usage for correctness, migrating from AutoFixture, or unsure whether an approach is idiomatic | diff --git a/skills/compono/references/patterns-and-antipatterns.md b/skills/compono/references/patterns-and-antipatterns.md index 4c82603..55075b2 100644 --- a/skills/compono/references/patterns-and-antipatterns.md +++ b/skills/compono/references/patterns-and-antipatterns.md @@ -52,6 +52,23 @@ drawn from real dogfooding evidence 14. **Reusing one member rule across unrelated types** hoping it applies broadly — use a type rule or `Register()` if it should really be global. +15. **Recommending a combinatorial set of profile subclasses, a per-test + `Composer.Create(...)` escape hatch, invented ambient/global scenario + state, or AutoFixture retention** for a parameterized custom + `AutoDataAttribute` (constructor args driving customization logic — + e.g. `PersistenceAutoData(repositoryName)`) — the correct answer is + `[Compose]`, see `xunit-v3.md` and the mapping + table below. Don't propose the workarounds `[Compose]` exists to eliminate. +16. **Passing a bare string to a `[Compose]` argument** + when the value is really a finite choice or a CLR type — prefer an + `enum`/`typeof(...)` instead; see `xunit-v3.md`'s "no stringly typed + configuration" guidance. +17. **Reaching for `[Compose]` to solve a name-based + value-selection problem, or reaching for a custom + `ICompositionValueProvider` to solve a call-site-configuration + problem** — these are two different questions (see `xunit-v3.md`), + not two names for the same mechanism. ## AutoFixture → Compono concept mapping @@ -63,6 +80,7 @@ drawn from real dogfooding evidence | `AutoNSubstituteCustomization` | `builder.UseNSubstitute()` | No `ConfigureMembers` equivalent — see antipattern 12 and `nsubstitute.md` | | `fixture.Customize(...)` | `builder.Register()` / `.For().Use()` | Re-customizing the same type is a build-time conflict, not override | | `[AutoData]`/`[InlineAutoData]` | `[Compose]` / inline args on `[Compose(...)]` | Only one Compose-family attribute per method — see `xunit-v3.md` | +| Parameterized custom `AutoDataAttribute` (constructor args driving customization logic) | `[Compose]` | `TConfig`'s constructor args, not the test method's parameters — see `xunit-v3.md`. Use an enum/`typeof(...)`, not a bare string | | `OmitOnRecursionBehavior` | *(none)* | Real cycles fail fast; break them with an explicit `Register()` | | `IFixture` | *(none)* | No fixture-holder object; configure via `[Compose]`/`ICompositionProfile` per test | | `IRequestSpecification`/`NamedRequest` | *(none)* | No equivalent request-matching abstraction | diff --git a/skills/compono/references/registrations-profiles-and-scopes.md b/skills/compono/references/registrations-profiles-and-scopes.md index 9da9b23..2b2796e 100644 --- a/skills/compono/references/registrations-profiles-and-scopes.md +++ b/skills/compono/references/registrations-profiles-and-scopes.md @@ -76,6 +76,51 @@ var composer = Composer.Create(b => b.AddProfile()); consumer/test class that happens to use them — don't grow one giant catch-all profile. +## Custom providers — matching on request shape, including name + +`Register()`/`.For()` are exact-type-keyed. When a value genuinely +needs to vary by the **requesting parameter/member's own name**, not just +its type — several distinct values of the same declared type, chosen by +which parameter is asking — write a custom `ICompositionValueProvider` +instead: + +```csharp +public sealed class UpsellPayloadProvider : ICompositionValueProvider +{ + public CompositionProviderResult TryProvide(in CompositionProviderRequest request, ICompositionContext context) + { + if (request.RequestedType != typeof(UpsellPayload)) + return CompositionProviderResult.NotHandled; + + return request.Name switch + { + "newGamePayload" => CompositionProviderResult.Handled(new UpsellPayload("new-game")), + "lockedPackPayload" => CompositionProviderResult.Handled(new UpsellPayload("locked-pack")), + _ => CompositionProviderResult.NotHandled, + }; + } +} + +builder.AddSemanticProvider(new UpsellPayloadProvider()); +// or AddTestDoubleProvider(...), depending on what it produces +``` + +`CompositionProviderRequest.Name` carries the requesting constructor +parameter/required member/test-method-parameter's own name — this is a +**global rule** ("whenever anything asks for `UpsellPayload` named +`newGamePayload`, produce this"), evaluated for every matching request +across every test. Reserve this for the case that genuinely needs to +match on request shape rather than a fixed type — most AutoFixture +specimen builders migrate to a plain `Register()` instead (see +`patterns-and-antipatterns.md`'s mapping table). + +**Don't confuse this with `[Compose]`'s profile +configuration arguments** (`xunit-v3.md`) — a `Name`-based provider is a +global rule keyed off the requesting parameter's name; a profile +configuration argument is a per-invocation value known only at one +specific test's call site. They solve different problems and aren't +interchangeable. + ## Scopes and recursion A type appearing twice in a graph is **not** automatically a cycle — a diff --git a/skills/compono/references/xunit-v3.md b/skills/compono/references/xunit-v3.md index fe7d346..f97129e 100644 --- a/skills/compono/references/xunit-v3.md +++ b/skills/compono/references/xunit-v3.md @@ -50,6 +50,60 @@ Same behavior as `[Compose]`, but applies `TProfile.Configure` to the row's builder first — this is how a theory picks up `UseNSubstitute()`/`UseBogus()`/registrations for that specific test. +## `[Compose]` + +```csharp +public enum RepositoryKind { Player, Game } + +public sealed record RepositoryConfig(RepositoryKind Repository); + +public sealed class RepositoryProfile : ICompositionProfile +{ + public RepositoryProfile(RepositoryConfig config) => Config = config; + public RepositoryConfig Config { get; } + public void Configure(CompositionBuilder builder) => + builder.Register(_ => RepositoryFactory.Create(Config.Repository)); +} + +[Theory] +[Compose(RepositoryKind.Player)] +public void Handles_PlayerRepository(IRepository repository) { } +``` + +Use this when a profile needs a value only known at **this specific +test's call site** - not a fixed, default-constructed profile the way +`[Compose]` always is. `TConfig`'s constructor arguments here +(**profile configuration arguments**) are a completely different binding +target from this file's inline values above - they never bind to the +test method's own parameters, all of which are still composed in full. + +- `TConfig` must have exactly one public constructor; `TProfile` must have + exactly one public constructor accepting exactly one `TConfig`-typed + parameter. Either shape being wrong is a clear, cached + `CompositionException` at binding-plan-construction time, not a compile + error (`[Compose]`'s `new()` constraint doesn't carry over to + this form - see `docs/adr/0036-parameterized-composition-profile-selection.md`). +- **Use the strongest attribute-legal type for each argument** - an + `enum` for a finite choice, `typeof(...)` for a CLR type, `bool`/numeric + where that's already the real meaning. `params object?[]` is a binding + mechanism C# attribute rules force, not a reason to design `TConfig` + around magic strings. Flag `[Compose("SomeString")]` + in review the same way you'd flag any other stringly-typed value + standing in for a finite choice. +- **This is not the same problem as name-based value selection.** A value + that varies by which parameter/member is *asking* (not by test call + site) is a `CompositionProviderRequest.Name`-matching custom + `ICompositionValueProvider` question - see + `registrations-profiles-and-scopes.md`. Don't reach for + `[Compose]` for that case, and don't reach for a + custom provider for this one. +- **Don't reach for this form by default.** If the "parameter" a + migrated AutoFixture attribute takes is really just obtaining a + substitute, or a single fixed value that never actually varies across + real call sites, the plain forms already cover it - reserve this one + for a value that's genuinely different per call site and needs to + reach configuration logic running *inside* the profile. + ## Hard constraint: one Compose-family attribute per method `[Compose]` and `[Compose]` are both `DataAttribute` subclasses. diff --git a/src/Compono.Generators/ComponoIncrementalGenerator.cs b/src/Compono.Generators/ComponoIncrementalGenerator.cs index 03e0e39..664d2ea 100644 --- a/src/Compono.Generators/ComponoIncrementalGenerator.cs +++ b/src/Compono.Generators/ComponoIncrementalGenerator.cs @@ -73,14 +73,27 @@ public void Initialize(IncrementalGeneratorInitializationContext context) ComposeMethodDiscovery.TransformMethod) .WithTrackingName(TrackingNames.ComposeGenericMethods); - // Both ComposeMethodDiscovery registrations above (non-generic and generic-metadata-name) - // feed the exact same discovery logic - merge them into one provider here so every consumer - // below treats "a [Compose]/[Compose]-attributed method" as a single source, same - // as CreateInvocations/Composable/AssemblyComposable already do for their own two-syntax-form - // splits. + // [Compose] specifically (ADR-0036) - same reasoning as the arity-1 + // registration immediately above: ForAttributeWithMetadataName matches only the exact, + // arity-suffixed attribute class metadata name ("ComposeAttribute`2"), invisible to either + // of the other two registrations. Same transform - TransformMethod only cares about the + // attributed method's own parameters, not which ComposeAttribute arity triggered it. + var composeTwoTypeParameterMethodResults = context.SyntaxProvider + .ForAttributeWithMetadataName( + ComposeMethodDiscovery.TwoTypeParameterAttributeMetadataName, + static (node, _) => node is MethodDeclarationSyntax, + ComposeMethodDiscovery.TransformMethod) + .WithTrackingName(TrackingNames.ComposeTwoTypeParameterMethods); + + // All three ComposeMethodDiscovery registrations above (non-generic, arity-1, arity-2) feed + // the exact same discovery logic - merge them into one provider here so every consumer below + // treats "a [Compose]/[Compose]/[Compose]-attributed method" as + // a single source, same as CreateInvocations/Composable/AssemblyComposable already do for + // their own multi-syntax-form splits. var composeMethodResultsAll = composeMethodResults.Collect() .Combine(composeGenericMethodResults.Collect()) - .SelectMany(static (results, _) => results.Left.Concat(results.Right)) + .Combine(composeTwoTypeParameterMethodResults.Collect()) + .SelectMany(static (results, _) => results.Left.Left.Concat(results.Left.Right).Concat(results.Right)) .WithTrackingName(TrackingNames.ComposeMethodsAll); // Each discovery result carries its own transitive closure (Types) alongside every closed @@ -266,6 +279,7 @@ internal static class TrackingNames public const string AssemblyComposablesTypes = "AssemblyComposables.Types"; public const string ComposeMethods = "ComposeMethods"; public const string ComposeGenericMethods = "ComposeMethods.Generic"; + public const string ComposeTwoTypeParameterMethods = "ComposeMethods.TwoTypeParameter"; public const string ComposeMethodsAll = "ComposeMethods.All"; public const string ComposeMethodsTypes = "ComposeMethods.Types"; public const string DiscoveredCollected = "Discovered.Collected"; diff --git a/src/Compono.Generators/Discovery/ComposeMethodDiscovery.cs b/src/Compono.Generators/Discovery/ComposeMethodDiscovery.cs index f5835d9..0a58f9c 100644 --- a/src/Compono.Generators/Discovery/ComposeMethodDiscovery.cs +++ b/src/Compono.Generators/Discovery/ComposeMethodDiscovery.cs @@ -41,6 +41,18 @@ internal static class ComposeMethodDiscovery /// public const string GenericAttributeMetadataName = "Compono.XunitV3.ComposeAttribute`1"; + /// + /// The metadata name of the two-type-parameter form, [Compose<TProfile, TConfig>] - + /// same reasoning as : its attribute class metadata + /// name is the distinct, arity-suffixed Compono.XunitV3.ComposeAttribute`2, invisible to + /// a provider registered against either of the other two metadata names, and needs its own + /// independently-registered provider (PR #65 review - the packaged sample's only composed + /// parameter type was a registered string, which never needs a generated plan and masked + /// this gap; a concrete, undiscovered-elsewhere parameter type reached only through + /// [Compose<TProfile, TConfig>] failed at GetData time with no plan found). + /// + public const string TwoTypeParameterAttributeMetadataName = "Compono.XunitV3.ComposeAttribute`2"; + public static TransitiveClosureResult TransformMethod(GeneratorAttributeSyntaxContext context, CancellationToken cancellationToken) { if (context.TargetSymbol is not IMethodSymbol method || method.IsGenericMethod) diff --git a/src/Compono.XunitV3/Binding/BindingPlan.cs b/src/Compono.XunitV3/Binding/BindingPlan.cs index f7a7600..c95b0ee 100644 --- a/src/Compono.XunitV3/Binding/BindingPlan.cs +++ b/src/Compono.XunitV3/Binding/BindingPlan.cs @@ -93,14 +93,15 @@ internal static string MethodDisplayName(MethodInfo testMethod) => var methodDisplayName = MethodDisplayName(testMethod); // [AttributeUsage(AllowMultiple = false)] is enforced per exact attribute type by the - // compiler, not across a base/derived family - [Compose] and [Compose] (or two - // differently-closed [Compose] forms) are distinct types that each individually - // satisfy their own AllowMultiple = false, so nothing stops stacking more than one - // Compose-family attribute on the same method without this explicit check (PR #23 review). + // compiler, not across a base/derived family - [Compose], [Compose], and + // [Compose] (or two differently-closed forms of either generic one) are + // distinct types that each individually satisfy their own AllowMultiple = false, so nothing + // stops stacking more than one Compose-family attribute on the same method without this + // explicit check (PR #23 review; extended to the two-type-parameter form by PR #65 review). var composeAttributeCount = testMethod.GetCustomAttributes().Count(); if (composeAttributeCount > 1) - return $"More than one [Compose]/[Compose] attribute on '{methodDisplayName}' - only one Compose-family attribute per test method is allowed."; + return $"More than one [Compose]/[Compose]/[Compose] attribute on '{methodDisplayName}' - only one Compose-family attribute per test method is allowed."; if (testMethod.IsGenericMethodDefinition) return $"Compono.XunitV3 does not support generic test methods ('{methodDisplayName}')."; diff --git a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs new file mode 100644 index 0000000..8620f39 --- /dev/null +++ b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs @@ -0,0 +1,184 @@ +using System.Reflection; +using System.Runtime.ExceptionServices; + +namespace Compono.XunitV3.Binding; + +/// +/// Resolves and invokes the single constructors uses +/// to build a TConfig from profile configuration arguments and a TProfile from that +/// TConfig - see +/// docs/adr/0036-parameterized-composition-profile-selection.md's "Constructor contracts" +/// section. Deliberately narrow and deterministic: no "best constructor match" heuristic exists +/// anywhere here - an unsupported or ambiguous constructor shape is always a clear, named +/// , never a guessed resolution. +/// +/// +/// Every method here is reflection - and +/// , not the cached / +/// shape uses. That +/// shape exists there to close a generic method over a parameter type known only at runtime, once per +/// parameter, so the per-row path never reflects again. TConfig/TProfile need no +/// equivalent: they're already compile-time-closed generic arguments on +/// itself, and this binder's methods are only ever +/// called from - itself only ever +/// invoked once per attribute instance, from inside the base 's existing +/// Lazy<Composer>-backed caching. That existing caching is what bounds this reflection to +/// once per attribute instance, never the repeated per-row GetData path - no separate caching +/// layer is needed here. +/// +internal static class ConfigProfileBinder +{ + /// + /// Binds positionally to 's single + /// public constructor and invokes it - the same count/nullability/assignability validation + /// 's own inline-value binding uses (ADR-0022), retargeted at a + /// constructor's parameters instead of a test method's. + /// + /// + /// does not have exactly one public constructor; the supplied + /// argument count doesn't match that constructor's parameter count; a supplied argument is + /// for a non-nullable parameter; or a supplied argument's type isn't + /// assignable to its parameter's type. + /// + public static object BindConfig(Type configType, IReadOnlyList configArguments) + { + var constructor = ResolveSingleConstructor(configType); + var parameters = constructor.GetParameters(); + + if (configArguments.Count != parameters.Length) + { + throw new CompositionException( + $"'{configType}' requires {parameters.Length} profile configuration argument(s), but {configArguments.Count} were supplied."); + } + + var arguments = new object?[parameters.Length]; + var nullabilityContext = new NullabilityInfoContext(); + + for (var i = 0; i < parameters.Length; i++) + { + var parameter = parameters[i]; + var value = configArguments[i]; + var nullability = IsNullable(nullabilityContext, parameter) ? Nullability.Nullable : Nullability.NotNullable; + + switch (PositionalArgumentBinder.Validate(parameter.ParameterType, nullability, value)) + { + case PositionalArgumentValidation.NullNotAllowed: + throw new CompositionException( + $"Profile configuration argument for parameter '{parameter.Name}' of '{configType}' is null, but the parameter is not nullable."); + + case PositionalArgumentValidation.TypeMismatch: + throw new CompositionException( + $"Profile configuration argument for parameter '{parameter.Name}' of '{configType}' has type '{value!.GetType()}', which is not assignable to '{parameter.ParameterType}'."); + } + + arguments[i] = value; + } + + return Invoke(constructor, arguments); + } + + /// + /// Constructs a from , via + /// 's single public constructor accepting exactly one + /// -typed parameter. + /// + /// + /// does not have exactly one public constructor accepting exactly + /// one -typed parameter. + /// + public static TProfile BuildProfile(object config) + where TProfile : ICompositionProfile + { + var constructor = ResolveSingleProfileConstructor(typeof(TProfile), typeof(TConfig)); + + return (TProfile)Invoke(constructor, [config]); + } + + // ConstructorInfo.Invoke wraps any exception the constructor body itself throws (e.g. a + // CompositionException from custom TConfig/TProfile validation logic) in a + // TargetInvocationException - without unwrapping it here, ApplyProfile's own + // catch (CompositionException) could never observe it, and the caller would see a generic + // reflection failure with no seed-reporting instead of the constructor's own actionable + // exception (PR #65 review). ExceptionDispatchInfo.Capture(...).Throw() re-throws the inner + // exception with its original stack trace preserved, rather than a bare `throw + // exception.InnerException` (which would reset it) or catching only CompositionException + // specifically (which would still let a non-CompositionException constructor-thrown exception + // stay wrongly wrapped). + private static object Invoke(ConstructorInfo constructor, object?[] arguments) + { + try + { + return constructor.Invoke(arguments); + } + catch (TargetInvocationException exception) when (exception.InnerException is not null) + { + ExceptionDispatchInfo.Capture(exception.InnerException).Throw(); + throw; // Unreachable - Throw() always throws; satisfies every code path returning a value. + } + } + + private static ConstructorInfo ResolveSingleConstructor(Type type) + { + // An abstract type can still declare a public constructor (invoked only by a derived type's + // own constructor chain) - GetConstructors would find it and this method would otherwise + // hand it back as "the one constructor," but ConstructorInfo.Invoke on it throws + // MemberAccessException ("Cannot create an abstract class"), not the documented + // CompositionException - reject explicitly, before the constructor count check, so an + // abstract TConfig fails with the same named diagnostic shape as every other unsupported + // shape here (PR #65 review). + if (type.IsAbstract) + { + throw new CompositionException( + $"'{type}' is abstract and cannot be used as profile configuration - it must be a concrete, constructible type."); + } + + var constructors = type.GetConstructors(BindingFlags.Public | BindingFlags.Instance); + + if (constructors.Length != 1) + { + throw new CompositionException( + $"'{type}' must have exactly one public constructor to be used as profile configuration, but has {constructors.Length}."); + } + + return constructors[0]; + } + + private static ConstructorInfo ResolveSingleProfileConstructor(Type profileType, Type configType) + { + // Same reasoning as ResolveSingleConstructor's abstract check above - an abstract TProfile + // with a matching public constructor would otherwise reach ConstructorInfo.Invoke and throw + // MemberAccessException instead of the documented CompositionException. + if (profileType.IsAbstract) + { + throw new CompositionException( + $"'{profileType}' is abstract and cannot be used as a profile - it must be a concrete, constructible type."); + } + + var matching = profileType.GetConstructors(BindingFlags.Public | BindingFlags.Instance) + .Where(constructor => + { + var parameters = constructor.GetParameters(); + return parameters.Length == 1 && parameters[0].ParameterType == configType; + }) + .ToArray(); + + if (matching.Length != 1) + { + throw new CompositionException( + $"'{profileType}' must have exactly one public constructor accepting a single '{configType}' parameter, but has {matching.Length}."); + } + + return matching[0]; + } + + private static bool IsNullable(NullabilityInfoContext nullabilityContext, ParameterInfo parameter) + { + if (Nullable.GetUnderlyingType(parameter.ParameterType) is not null) + return true; + + if (parameter.ParameterType.IsValueType) + return false; + + return nullabilityContext.Create(parameter).ReadState == NullabilityState.Nullable; + } +} diff --git a/src/Compono.XunitV3/Binding/PositionalArgumentBinder.cs b/src/Compono.XunitV3/Binding/PositionalArgumentBinder.cs new file mode 100644 index 0000000..34af98e --- /dev/null +++ b/src/Compono.XunitV3/Binding/PositionalArgumentBinder.cs @@ -0,0 +1,57 @@ +namespace Compono.XunitV3.Binding; + +/// +/// The outcome of - a validated value, a +/// value against a non-nullable parameter, or a value whose type isn't +/// assignable to the parameter's type. +/// +internal enum PositionalArgumentValidation +{ + /// The value is valid for the parameter. + Valid, + + /// The value is , but the parameter is not nullable-annotated. + NullNotAllowed, + + /// The value's runtime type is not assignable to the parameter's type. + TypeMismatch, +} + +/// +/// The single null/-unwrap/assignability check every positional-argument +/// binding target in Compono.XunitV3 needs - 's inline-value +/// binding (test-method parameters, ADR-0022) and ConfigProfileBinder's profile-configuration- +/// argument binding (a TConfig type's constructor parameters, ADR-0036) both call this, +/// rather than each independently reimplementing the check - a correction to this logic now reaches +/// every binding target that uses it, not just whichever one it happened to be written against +/// (PR #65 review). Message text stays owned by each call site, not centralized here: the two +/// targets describe the value being validated differently ("Inline value for parameter... on..." +/// vs. "Profile configuration argument for parameter... of..."), and centralizing the message text +/// as well would either lose that distinction or force an awkward shared template. +/// +internal static class PositionalArgumentBinder +{ + /// + /// Validates against a parameter of type + /// with the given . + /// + public static PositionalArgumentValidation Validate(Type parameterType, Nullability nullability, object? value) + { + if (value is null) + { + return nullability == Nullability.Nullable + ? PositionalArgumentValidation.Valid + : PositionalArgumentValidation.NullNotAllowed; + } + + // A non-null Nullable boxes as a boxed T, not a boxed Nullable (a CLR nullable-boxing + // rule) - unwrapping first is a no-op for a non-nullable parameter (Nullable.GetUnderlyingType + // returns null, so ?? falls back to the declared type unchanged) and is what makes e.g. an + // int value valid for an int? parameter. + var underlyingType = Nullable.GetUnderlyingType(parameterType) ?? parameterType; + + return underlyingType.IsInstanceOfType(value) + ? PositionalArgumentValidation.Valid + : PositionalArgumentValidation.TypeMismatch; + } +} diff --git a/src/Compono.XunitV3/Compono.XunitV3.csproj b/src/Compono.XunitV3/Compono.XunitV3.csproj index 66139a7..4f2c0e3 100644 --- a/src/Compono.XunitV3/Compono.XunitV3.csproj +++ b/src/Compono.XunitV3/Compono.XunitV3.csproj @@ -6,7 +6,7 @@ enable net10.0;net11.0 Compono — xUnit v3 Integration - xUnit v3 integration for Compono - [Compose]/[Compose<TProfile>] theory data attributes and [Shared] parameter sharing. + xUnit v3 integration for Compono - [Compose]/[Compose<TProfile>]/[Compose<TProfile, TConfig>] theory data attributes and [Shared] parameter sharing. @@ -15,9 +15,10 @@ only Compono.XunitV3 - caught by test/Compono.XunitV3.SampleTests failing to compose a [Compose]-attributed type until this was added (PLAN-0004 Phase 3). --> - + diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 34360da..d18699b 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -15,12 +15,18 @@ namespace Compono.XunitV3; /// and diagnostics. /// /// -/// Deliberately unsealed - is the one designed extension -/// point, mirroring 's own -/// TProfile : ICompositionProfile, new() constraint. -/// returns : composition is deferred entirely to execution time, so -/// runs for real exactly once per test execution - there is no separate -/// discovery-time composition pass to keep synchronized with it. +/// Deliberately unsealed - and +/// are the two designed extension points. +/// mirrors 's +/// own TProfile : ICompositionProfile, new() constraint (a fixed, default-constructed +/// profile); mirrors +/// 's instance-based overload +/// instead (a profile built from call-site-known profile configuration arguments - see +/// docs/adr/0036-parameterized-composition-profile-selection.md). +/// returns : composition is +/// deferred entirely to execution time, so runs for real exactly once per +/// test execution - there is no separate discovery-time composition pass to keep synchronized with +/// it. /// [AttributeUsage(AttributeTargets.Method, AllowMultiple = false)] public class ComposeAttribute : DataAttribute @@ -60,16 +66,7 @@ public class ComposeAttribute : DataAttribute /// public ComposeAttribute(params object?[] inlineValues) { - _inlineValues = inlineValues switch - { - null => [null], - // Every genuinely expanded-form call (zero or more scalar arguments, including - // Compose()'s empty case) produces a freshly built array whose runtime type is exactly - // object[] - only a single non-expanded reference-array argument arrives with some other - // runtime array type, per the remarks above. - not null when inlineValues.GetType() != typeof(object[]) => [inlineValues], - _ => inlineValues, - }; + _inlineValues = NormalizeParamsArguments(inlineValues); _composer = new Lazy(BuildComposer); } @@ -189,29 +186,17 @@ public override ValueTask> GetData(MethodInf var parameter = parameters[i]; var value = _inlineValues[i]; - if (value is null) + switch (PositionalArgumentBinder.Validate(parameter.ParameterType, parameter.Descriptor.Nullability, value)) { - if (parameter.Descriptor.Nullability != Nullability.Nullable) - { + case PositionalArgumentValidation.NullNotAllowed: throw new CompositionException(AppendSeed( $"Inline value for parameter '{parameter.Name}' on '{methodDisplayName}' is null, but the parameter is not nullable.", row.Seed)); - } - - continue; - } - - // A non-null Nullable boxes as a boxed T, not a boxed Nullable (a CLR - // nullable-boxing rule) - unwrapping first is a no-op for a non-nullable parameter - // (Nullable.GetUnderlyingType returns null, so ?? falls back to the declared type - // unchanged) and is what makes e.g. [Compose(42)] valid for an int? parameter. - var underlyingType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - if (!underlyingType.IsInstanceOfType(value)) - { - throw new CompositionException(AppendSeed( - $"Inline value for parameter '{parameter.Name}' on '{methodDisplayName}' has type '{value.GetType()}', which is not assignable to '{parameter.ParameterType}'.", - row.Seed)); + case PositionalArgumentValidation.TypeMismatch: + throw new CompositionException(AppendSeed( + $"Inline value for parameter '{parameter.Name}' on '{methodDisplayName}' has type '{value!.GetType()}', which is not assignable to '{parameter.ParameterType}'.", + row.Seed)); } } @@ -274,6 +259,21 @@ internal virtual void ApplyProfile(CompositionBuilder builder) { } + // Shared with ComposeAttribute{TProfile,TConfig}'s profile-configuration-argument constructor, + // which faces the exact same params object?[] single-null/single-array binding ambiguity this + // constructor's own remarks document - extracted so both call sites normalize identically rather + // than reimplementing the same edge cases twice. + internal static object?[] NormalizeParamsArguments(object?[] arguments) => arguments switch + { + null => [null], + // Every genuinely expanded-form call (zero or more scalar arguments, including an empty + // case) produces a freshly built array whose runtime type is exactly object[] - only a + // single non-expanded reference-array argument arrives with some other runtime array type, + // per this constructor's own remarks above. + not null when arguments.GetType() != typeof(object[]) => [arguments], + _ => arguments, + }; + // Internal test seam - lets Compono.XunitV3.Tests assert the same BindingPlan instance (and the // same per-parameter invoker delegates on it) is returned across repeated calls with the same // testMethod, proving MakeGenericMethod ran exactly once per parameter, not once per GetData call. @@ -296,7 +296,11 @@ private Composer BuildComposer() => Composer.Create(builder => // matching the same trailing text a propagated pipeline CompositionDiagnostic already renders // (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}"; + // private protected, not private - ComposeAttribute{TProfile,TConfig} reuses this exact + // convention for its own pre-composer negative-seed check (PR #65 review), which must run + // before ApplyProfile does any config/profile binding work, i.e. before a CompositionRow (and + // this method's usual row.Seed source) exists at all. + private protected 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 diff --git a/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs new file mode 100644 index 0000000..c78bc12 --- /dev/null +++ b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs @@ -0,0 +1,108 @@ +using Compono.XunitV3.Binding; + +namespace Compono.XunitV3; + +/// +/// Composes an xUnit v3 theory row's parameters through Compono, applying a profile built from +/// profile configuration arguments known at this attribute's call site - a distinct concept +/// from this attribute family's ordinary inline values (), +/// which bind to the test method's own parameters instead. This constructor never binds to the test +/// method's parameters at all; every one of them is composed in full. is +/// constructed positionally from this attribute's own constructor arguments, then +/// is constructed from that instance and +/// applied via - equivalent to +/// Composer.Create(builder => builder.AddProfile(new TProfile(new TConfig(...)))). See +/// docs/adr/0036-parameterized-composition-profile-selection.md for the full design, including +/// why this exists as a separate attribute rather than overloading +/// 's own inline-value constructor argument. +/// +/// +/// The profile to construct and apply. Must have exactly one public constructor accepting exactly one +/// -typed parameter - no new() constraint, unlike +/// , since this form is never default-constructed. +/// +/// +/// The type this attribute's constructor arguments bind to, positionally, against its own single +/// public constructor. Prefer strongly-typed, attribute-legal values for its constructor parameters - +/// an for a finite choice, via typeof(...) for a CLR +/// type, a plain /numeric/string value where that already carries the real +/// meaning - over loosely-typed primitives standing in for something more specific. +/// params object?[] is a binding mechanism forced by C#'s attribute-argument-must-be-a- +/// compile-time-constant rule, not a license to design around magic +/// strings. +/// +/// +/// Unlike 's compile-time-enforced new() constraint, an +/// unsupported / constructor shape (not +/// exactly one public constructor on ; no exactly-one- +/// -parameter public constructor on ) is a +/// deterministic runtime , not a compile error - there is no C# +/// generic constraint that expresses "has a constructor accepting exactly this type." Both constructor +/// lookups, and the actual construction, are reflection () - bounded +/// and cached to once per attribute instance by this attribute family's existing +/// -backed caching ( is only ever +/// invoked from inside that lazy initializer), never on the repeated per-row GetData path. +/// +[AttributeUsage(AttributeTargets.Method, AllowMultiple = false)] +public sealed class ComposeAttribute : ComposeAttribute + where TProfile : ICompositionProfile +{ + private readonly object?[] _configArguments; + + /// + /// Creates a . + /// + /// + /// Profile configuration arguments, bound positionally to 's single + /// public constructor - an entirely separate binding target from this attribute family's ordinary + /// inline values; every test method parameter is composed in full regardless of what's supplied + /// here. See the type-level remarks for why each argument should use the strongest attribute-legal + /// type available rather than a bare string. + /// + public ComposeAttribute(params object?[] configArguments) : base() + { + _configArguments = NormalizeParamsArguments(configArguments); + } + + internal override void ApplyProfile(CompositionBuilder builder) + { + // A negative configured seed must be rejected before any config/profile binding is + // attempted - otherwise Seed = -1 combined with an invalid TConfig/TProfile shape would + // report the binder failure below with "Seed: -1" embedded instead of the documented + // negative-seed diagnostic the base class's own GetData enforces (PR #65 review). + // SeedAsNullable is exactly what CompositionRow.Seed would resolve to if non-negative + // (Composer.CreateRow's unseeded fallback only ever generates a non-negative value), so + // checking it here - before a CompositionRow even exists - gives the identical guarantee + // GetData's own row.Seed < 0 check gives, just earlier. + if (SeedAsNullable is { } configuredSeed && configuredSeed < 0) + { + throw new CompositionException(AppendSeed( + $"Compono.XunitV3 requires a non-negative seed, but the configured seed was {configuredSeed}.", + configuredSeed)); + } + + try + { + var config = ConfigProfileBinder.BindConfig(typeof(TConfig), _configArguments); + var profile = ConfigProfileBinder.BuildProfile(config); + + builder.AddProfile(profile); + } + catch (CompositionException exception) + { + // ApplyProfile runs while the base class's Lazy is still being built - before + // GetData ever calls Composer.CreateRow, so no CompositionRow/row.Seed exists yet at this + // point (PR #65 review: this attribute's own binder failures were escaping with no seed + // at all, unlike every other Compono.XunitV3-owned pre-composition failure). Report the + // seed this attribute is actually configured with (SeedAsNullable, already proven + // non-negative by the check above) - the same seed row.Seed would resolve to once + // composition succeeds - or a freshly generated one otherwise. Reproducibility isn't + // actually the point for this specific failure category (a constructor-shape/argument + // mismatch fails identically regardless of seed); this is purely ADR-0022's "every + // Compono.XunitV3-owned failure ends with Seed: {value}" convention, applied consistently + // rather than as an exception for this one binding path. + var seed = SeedAsNullable ?? Random.Shared.Next(0, int.MaxValue); + throw CompositionException.WithSeedInMessage(exception, seed); + } + } +} diff --git a/test/Compono.Generators.Tests/CompositionPlanVerifyTests.cs b/test/Compono.Generators.Tests/CompositionPlanVerifyTests.cs index 51383e1..7dce0a7 100644 --- a/test/Compono.Generators.Tests/CompositionPlanVerifyTests.cs +++ b/test/Compono.Generators.Tests/CompositionPlanVerifyTests.cs @@ -1894,6 +1894,79 @@ public static void Creates_statement(Statement statement) """, }, TestContext.Current.CancellationToken); + [Fact] + public Task ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan() => + GeneratorTestHelpers.Verify(new CodeGenerationOptions + { + SourceCode = """ + namespace Compono.XunitV3 + { + // Stands in for the real Compono.XunitV3.ComposeAttribute/ICompositionProfile (a + // separate package/assembly, not referenced from this generator test project) - + // ComposeMethodDiscovery matches on the fully qualified metadata name alone, so + // same-named types here trigger it identically to the real ones. + public interface ICompositionProfile + { + void Configure(object builder); + } + + public class ComposeAttribute : System.Attribute + { + public ComposeAttribute(params object?[] inlineValues) { } + } + + public sealed class ComposeAttribute : ComposeAttribute + where TProfile : ICompositionProfile, new() + { + public ComposeAttribute(params object?[] inlineValues) : base(inlineValues) { } + } + + // [Compose]'s (ADR-0036) attribute class metadata name is the + // arity-suffixed "Compono.XunitV3.ComposeAttribute`2" - distinct from both the + // non-generic and the one-type-parameter forms above, and reached only via + // ComposeMethodDiscovery.TwoTypeParameterAttributeMetadataName's own, separately + // registered discovery provider (PR #65 review). + public sealed class ComposeAttribute : ComposeAttribute + where TProfile : ICompositionProfile + { + public ComposeAttribute(params object?[] configArguments) { } + } + } + + namespace TestNamespace + { + public sealed record InvoiceConfig(string Reference); + + public sealed class InvoiceProfile : Compono.XunitV3.ICompositionProfile + { + public InvoiceProfile(InvoiceConfig config) { } + public void Configure(object builder) { } + } + + public sealed class CreditNote + { + public CreditNote(string reference) { Reference = reference; } + public string Reference { get; } + } + + public static class TestClass + { + // No Create()/CreateMany() call site, no [Composable] + // attribute, and no non-generic/one-type-parameter [Compose] use anywhere in + // this source - CreditNote is reachable only as this + // [Compose]-attributed method's own parameter, proving the + // two-type-parameter-metadata-name discovery path on its own (the exact gap + // PR #65 review caught: a concrete parameter type reached only through this + // attribute form previously had no generated plan at all). + [Compono.XunitV3.Compose("some-reference")] + public static void Creates_creditNote(CreditNote creditNote) + { + } + } + } + """, + }, TestContext.Current.CancellationToken); + [Fact] public Task ComposeAttributedGenericMethodParameter_GeneratesNoPlan() => GeneratorTestHelpers.Verify(new CodeGenerationOptions diff --git a/test/Compono.Generators.Tests/Snapshots/CompositionPlanVerifyTests.ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan#TestNamespace.CreditNote_2e911510.CompositionPlan.g.verified.cs b/test/Compono.Generators.Tests/Snapshots/CompositionPlanVerifyTests.ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan#TestNamespace.CreditNote_2e911510.CompositionPlan.g.verified.cs new file mode 100644 index 0000000..a49d44c --- /dev/null +++ b/test/Compono.Generators.Tests/Snapshots/CompositionPlanVerifyTests.ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan#TestNamespace.CreditNote_2e911510.CompositionPlan.g.verified.cs @@ -0,0 +1,22 @@ +//HintName: TestNamespace.CreditNote_2e911510.CompositionPlan.g.cs +// +#nullable enable + +namespace TestNamespace +{ + [global::System.CodeDom.Compiler.GeneratedCode("Compono.Generators", "REPLACED")] + file sealed class CreditNoteCompositionPlan : global::Compono.ICompositionPlan + { + public global::TestNamespace.CreditNote Compose(global::Compono.ICompositionContext context) => + new global::TestNamespace.CreditNote( + context.Resolve(new global::Compono.CompositionRequestDescriptor(global::Compono.CompositionRequestKind.ConstructorParameter, 0, "reference", typeof(global::TestNamespace.CreditNote), global::Compono.Nullability.NotNullable)) + ); + } + + file static class CreditNoteCompositionPlanRegistration + { + [global::System.Runtime.CompilerServices.ModuleInitializer] + internal static void Register() => + global::Compono.PlanCache.Instance = new CreditNoteCompositionPlan(); + } +} diff --git a/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs new file mode 100644 index 0000000..ba727b9 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs @@ -0,0 +1,76 @@ +namespace Compono.XunitV3.SampleTests; + +// A finite-choice profile configuration argument uses an enum, not a string - ADR-0036's "no +// stringly typed configuration" principle, modeled on the real trivia-platform PersistenceAutoData +// shape RESEARCH-0002 Finding 1 is drawn from. +public enum RepositoryKind +{ + Player, + Game, +} + +public sealed record RepositoryTestConfig(RepositoryKind Repository); + +// Composed only as ConfigProfileTests' own [Compose]-attributed theory methods' +// parameter type - no Create()/CreateMany() call site, no +// [Composable], and no other Compose-family use of it anywhere else in this project. This is the +// exact shape PR #65 review caught: ComposeMethodDiscovery originally had no registered provider for +// the two-type-parameter attribute's own metadata name, so a concrete parameter type reached only +// this way got no generated ICompositionPlan at all and failed at GetData time - the earlier +// version of this file used only a registered `string` parameter, which never needs a generated plan +// and masked the gap entirely. RepositoryConsumer's own nested `string` constructor dependency is +// still satisfied by the profile's registration below, proving both paths (attribute-only discovery, +// and registration-backed nested resolution) together. +public sealed class RepositoryConsumer +{ + public RepositoryConsumer(string repositoryName) => RepositoryName = repositoryName; + + public string RepositoryName { get; } +} + +// Reached only through ConfigProfileTests' own [Compose] theory parameters - proves +// ComposeAttribute actually binds profile configuration arguments and applies the +// resulting profile through the real packaged pipeline, not just Compono.XunitV3.Tests' in-process +// GetData checks. +public sealed class RepositoryTestProfile : ICompositionProfile +{ + public RepositoryTestProfile(RepositoryTestConfig config) => Config = config; + + public RepositoryTestConfig Config { get; } + + public void Configure(CompositionBuilder builder) => + builder.Register(() => Config.Repository switch + { + RepositoryKind.Player => "player-repository", + RepositoryKind.Game => "game-repository", + _ => throw new ArgumentOutOfRangeException(nameof(Config)), + }); +} + +// Deliberately has no constructor accepting a RepositoryTestConfig - reserved for +// FailingConfigProfileTests below, which needs ConfigProfileBinder's own pre-composition +// constructor-shape failure, not a genuine composition failure (mirrors FailingCompositionTests' +// distinction for the ordinary [Compose]/[Compose] forms). +public sealed class ProfileWithNoMatchingConstructor : ICompositionProfile +{ + public void Configure(CompositionBuilder builder) + { + } +} + +public sealed class ConfigProfileTests +{ + [Theory] + [Compose(RepositoryKind.Player)] + public void ComposesTheProfileBuiltFromConfigArguments(RepositoryConsumer consumer) + { + consumer.RepositoryName.Should().Be("player-repository"); + } + + [Theory] + [Compose(RepositoryKind.Game)] + public void DifferentConfigArguments_ProduceADifferentlyConfiguredProfile(RepositoryConsumer consumer) + { + consumer.RepositoryName.Should().Be("game-repository"); + } +} diff --git a/test/Compono.XunitV3.SampleTests/FailingConfigProfileTests.cs b/test/Compono.XunitV3.SampleTests/FailingConfigProfileTests.cs new file mode 100644 index 0000000..42cfde9 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/FailingConfigProfileTests.cs @@ -0,0 +1,21 @@ +namespace Compono.XunitV3.SampleTests; + +// Deliberately fails, on every run, via ConfigProfileBinder's own pre-composition constructor-shape +// validation (ProfileWithNoMatchingConstructor has no constructor accepting a RepositoryTestConfig) +// - not a genuine composition failure. Proves the diagnostic reaches a real xUnit v3 runner's actual +// output before the test body ever executes, through the real packaged pipeline, mirroring +// FailingCompositionTests' own separate-class pattern for exactly this reason: this project's CI +// "Local-feed packed-consumer smoke test" step (.github/workflows/package-validation.yaml) filters +// out every class whose name starts with "Failing", so a deliberately-failing proof test lives in +// its own class matching that naming convention rather than inside an otherwise-green test class - +// keeping it in ConfigProfileTests.cs's own class caused the CI gate itself to fail (PR #65 review; +// caught live in CI after this file didn't exist yet). +public sealed class FailingConfigProfileTests +{ + [Theory] + [Compose(RepositoryKind.Player)] + public void MismatchedProfileConstructorShape_FailsBeforeTheTestExecutes(string repositoryName) + { + repositoryName.Should().BeNull("GetData throws before this body ever runs - this line never executes"); + } +} diff --git a/test/Compono.XunitV3.Tests/BindingPlanTests.cs b/test/Compono.XunitV3.Tests/BindingPlanTests.cs index 889dd72..914a6a7 100644 --- a/test/Compono.XunitV3.Tests/BindingPlanTests.cs +++ b/test/Compono.XunitV3.Tests/BindingPlanTests.cs @@ -97,6 +97,20 @@ public void Build_ReportsASignatureError_ForMultipleComposeFamilyAttributes() plan.Parameters.Should().BeEmpty(); } + [Fact] + public void Build_ReportsASignatureError_ForComposeStackedWithTheTwoTypeParameterForm() + { + // Detection (testMethod.GetCustomAttributes()) already covers this form + // since ComposeAttribute derives from ComposeAttribute - this test proves + // the reported message names it too, not just the two original forms (PR #65 review). + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithComposeAndTwoTypeParameterComposeAttributes))!; + + var plan = BindingPlan.Build(method); + + plan.SignatureError.Should().Contain("Compose"); + plan.Parameters.Should().BeEmpty(); + } + [Fact] public void Build_MarksTheParameterAsShared_WhenAttributed() { diff --git a/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs new file mode 100644 index 0000000..de839b6 --- /dev/null +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs @@ -0,0 +1,289 @@ +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +// Compono.XunitV3.SampleTests carries the packaged-consumer, real-runner proof (per ADR-0022's own +// packaged-consumer precedent); these tests exercise ComposeAttribute{TProfile,TConfig}/ +// ConfigProfileBinder directly via GetData, the same fast, no-real-runner style every other file in +// this project uses. +public sealed class ComposeAttributeConfigBindingTests +{ + [Fact] + public async Task GetData_ConstructsProfileFromConfig_AndComposesEveryTestParameter() + { + var attribute = new ComposeAttribute("from-config"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var rows = await attribute.GetData(method, tracker); + var data = rows.Single().GetData(); + + data.Should().Equal("from-config"); + } + + [Fact] + public void ConfigArguments_AreNeverBoundAsInlineValues() + { + // Profile configuration arguments and inline values are two entirely separate binding + // targets (ADR-0036's terminology split) - this attribute's base-class InlineValues must + // stay empty regardless of how many profile configuration arguments are supplied, proving + // WithNonNullableReferenceParameter's own parameter is composed via the profile's + // registration in the test above, never bound directly from "from-config" the way an inline + // value would be. + var attribute = new ComposeAttribute("from-config"); + + attribute.InlineValues.Should().BeEmpty(); + } + + [Fact] + public async Task GetData_Throws_WhenConfigTypeHasNoPublicConstructor() + { + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*exactly one public constructor*has 0*"); + } + + [Fact] + public async Task GetData_Throws_WhenConfigTypeHasMultiplePublicConstructors() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*exactly one public constructor*has 2*"); + } + + [Fact] + public async Task GetData_Throws_WhenProfileTypeHasNoConstructorAcceptingTheConfigType() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*must have exactly one public constructor accepting a single*TestConfig*parameter*has 0*"); + } + + [Fact] + public async Task GetData_Throws_WhenTooFewProfileConfigurationArgumentsAreSupplied() + { + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*requires 1 profile configuration argument(s)*0 were supplied*"); + } + + [Fact] + public async Task GetData_Throws_WhenTooManyProfileConfigurationArgumentsAreSupplied() + { + var attribute = new ComposeAttribute("one", "two"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*requires 1 profile configuration argument(s)*2 were supplied*"); + } + + [Fact] + public async Task GetData_Throws_WhenConfigTypeIsAbstract() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + // AbstractConfig has exactly one public constructor - passes the "exactly one constructor" + // count check on its own, so this proves the explicit IsAbstract guard, not the count check + // (PR #65 review: without it, this would throw MemberAccessException from + // ConstructorInfo.Invoke instead of the documented CompositionException). + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*abstract*cannot be used as profile configuration*"); + } + + [Fact] + public async Task GetData_Throws_WhenProfileTypeIsAbstract() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*abstract*cannot be used as a profile*"); + } + + [Fact] + public async Task GetData_AppendsTheConfiguredSeed_WhenProfileConstructionFailsBeforeARowExists() + { + // Every Compono.XunitV3-owned pre-composition failure ends with "Seed: {value}" (ADR-0022) - + // this failure category is special because it's thrown from inside the base class's + // Lazy initialization, before GetData ever calls Composer.CreateRow, so there is no + // CompositionRow/row.Seed to read from yet (PR #65 review: this was previously missing + // entirely for config/profile binder failures). Using an explicitly configured Seed proves the + // reported value is the one this attribute is actually configured with, not an unrelated + // throwaway number. + var attribute = new ComposeAttribute("value") { Seed = 492173 }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*Seed: 492173*"); + } + + [Fact] + public async Task GetData_AppendsAGeneratedSeed_WhenProfileConstructionFailsWithNoSeedConfigured() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + // No explicit seed configured, so only the convention (a trailing "Seed: ") + // is checked, not a specific value. + await act.Should().ThrowAsync() + .WithMessage("*\nSeed: *"); + } + + [Fact] + public async Task GetData_ReportsTheNegativeSeedDiagnostic_NotTheBinderFailure_WhenBothApply() + { + // Seed = -1 combined with an invalid profile/config shape must report the documented + // negative-seed diagnostic, not the binder failure with "Seed: -1" embedded (PR #65 review) - + // the negative-seed check has to run before any config/profile binding is even attempted. + var attribute = new ComposeAttribute("value") { Seed = -1 }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*non-negative seed*-1*"); + } + + [Fact] + public async Task GetData_UnwrapsAndReportsTheOriginalException_WhenTheConfigConstructorThrows() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + // Proves ConstructorInfo.Invoke's TargetInvocationException wrapper was unwrapped - the + // caller sees ThrowingTestConfig's own actionable message (and the seed convention still + // applies), not a generic reflection failure. + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*custom validation failed for 'value'*Seed: *"); + } + + [Fact] + public async Task GetData_UnwrapsAndReportsTheOriginalException_WhenTheProfileConstructorThrows() + { + var attribute = new ComposeAttribute("value"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*custom validation failed for 'value'*Seed: *"); + } + + [Fact] + public async Task GetData_Throws_WhenAProfileConfigurationArgumentHasAnIncompatibleType() + { + var attribute = new ComposeAttribute(42); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*not assignable to*"); + } + + [Fact] + public async Task GetData_Throws_WhenANullProfileConfigurationArgumentTargetsANonNullableParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var act = () => attribute.GetData(method, tracker).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*is null, but the parameter is not nullable*"); + } + + [Fact] + public async Task GetData_AcceptsANonNullValueTypeArgument_ForANullableValueTypeParameter() + { + // 42 boxes as System.Int32, not System.Nullable (a CLR nullable-boxing rule) - + // this proves ConfigProfileBinder unwraps Nullable before the assignability check the + // same way ComposeAttribute's own inline-value binding already does, rather than the check + // wrongly rejecting a valid int argument for an int? config constructor parameter. + var attribute = new ComposeAttribute(42); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableValueParameter))!; + var tracker = new DisposalTracker(); + + var rows = await attribute.GetData(method, tracker); + var data = rows.Single().GetData(); + + data.Should().Equal(42); + } + + [Fact] + public async Task GetData_AcceptsANullProfileConfigurationArgument_ForANullableParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + var rows = await attribute.GetData(method, tracker); + var data = rows.Single().GetData(); + + data.Should().Equal("null"); + } + + [Fact] + public async Task GetData_ConstructsTheProfileExactlyOnce_AcrossRepeatedGetDataCalls() + { + var attribute = new ComposeAttribute("from-config"); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + var tracker = new DisposalTracker(); + + // ApplyProfile (and, inside it, ConfigProfileBinder's reflection) only ever runs the first + // time the base class's Lazy is evaluated - asserting the same Composer instance is + // reused across repeated GetData calls is what proves the config/profile construction ran + // exactly once, not once per call, mirroring ComposeAttributeCachingTests' existing style for + // ComposeAttribute. + var composerBeforeFirstCall = attribute.GetComposer(); + + await attribute.GetData(method, tracker); + await attribute.GetData(method, tracker); + + attribute.GetComposer().Should().BeSameAs(composerBeforeFirstCall); + } +} diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index 24c37c9..bd079f1 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -112,6 +112,16 @@ public static void Generic(T value) public static void WithMultipleComposeAttributes(int value) { } + + // Same reasoning as WithMultipleComposeAttributes above, pairing the two-type-parameter form + // with the plain one instead of the one-type-parameter form - proves BindingPlan's stacking + // detection (and its message) covers ComposeAttribute too, not just the + // original two forms (PR #65 review). + [Compose] + [Compose("value")] + public static void WithComposeAndTwoTypeParameterComposeAttributes(int value) + { + } #pragma warning restore xUnit1008 public sealed class TestProfile : ICompositionProfile @@ -119,6 +129,130 @@ public sealed class TestProfile : ICompositionProfile public void Configure(CompositionBuilder builder) => builder.Register(() => "from-profile"); } + // ComposeAttribute{TProfile,TConfig} fixtures - a config record with exactly one public + // constructor (the supported shape), a profile with exactly one public constructor accepting + // exactly that config type, and one broken variant per ConfigProfileBinder failure mode. + + public sealed record TestConfig(string Value); + + public sealed class ParameterizedTestProfile : ICompositionProfile + { + public ParameterizedTestProfile(TestConfig config) => Config = config; + + public TestConfig Config { get; } + + public void Configure(CompositionBuilder builder) => builder.Register(() => Config.Value); + } + + public sealed record NullableTestConfig(string? Value); + + public sealed class NullableParameterizedTestProfile : ICompositionProfile + { + public NullableParameterizedTestProfile(NullableTestConfig config) => Config = config; + + public NullableTestConfig Config { get; } + + public void Configure(CompositionBuilder builder) => builder.Register(() => Config.Value ?? "null"); + } + + // A non-null value-typed profile configuration argument for a Nullable constructor parameter + // - proves ConfigProfileBinder's Nullable-boxing unwrap (a non-null int? boxes as a boxed + // int, not a boxed int?) the same way ComposeAttribute's own inline-value binding already covers + // it, retargeted at a config type's constructor instead of a test method's parameters (PR #65 + // review: this exact case had no regression coverage). + public sealed record NullableIntTestConfig(int? Value); + + public sealed class NullableIntParameterizedTestProfile : ICompositionProfile + { + public NullableIntParameterizedTestProfile(NullableIntTestConfig config) => Config = config; + + public NullableIntTestConfig Config { get; } + + public void Configure(CompositionBuilder builder) => builder.Register(() => Config.Value ?? -1); + } + + // Zero public constructors - ConfigProfileBinder.BindConfig's "exactly one" check, zero case. + public sealed class ConfigWithNoPublicConstructor + { + private ConfigWithNoPublicConstructor() + { + } + } + + // Two public constructors - ConfigProfileBinder.BindConfig's "exactly one" check, ambiguous case. + public sealed class ConfigWithMultiplePublicConstructors + { + public ConfigWithMultiplePublicConstructors(string value) => Value = value; + + public ConfigWithMultiplePublicConstructors(string value, string extra) + { + Value = value; + Extra = extra; + } + + public string Value { get; } + + public string? Extra { get; } + } + + // No constructor accepting exactly one TestConfig parameter - ConfigProfileBinder.BuildProfile's + // "exactly one matching constructor" check, zero-match case. + public sealed class ProfileWithoutMatchingConstructor : ICompositionProfile + { + public void Configure(CompositionBuilder builder) + { + } + } + + // Abstract with an otherwise-qualifying public constructor - ConfigProfileBinder.BindConfig's + // abstract-type rejection, not the "exactly one constructor" count check (PR #65 review: without + // the explicit IsAbstract check, this shape would pass the count check and then throw + // MemberAccessException from ConstructorInfo.Invoke instead of the documented CompositionException). + public abstract class AbstractConfig + { + // Public, not protected - an abstract type's constructor accessibility is independent of + // whether the type itself can be instantiated; C# and the CLR both allow a public constructor + // on an abstract type (only a derived type can actually call it), which is exactly what makes + // this shape reach ConstructorInfo.Invoke without the explicit IsAbstract guard. + public AbstractConfig(string value) => Value = value; + + public string Value { get; } + } + + // Abstract with an otherwise-qualifying public constructor accepting TestConfig - + // ConfigProfileBinder.BuildProfile's abstract-type rejection, same reasoning as AbstractConfig + // above. + public abstract class AbstractProfile : ICompositionProfile + { + public AbstractProfile(TestConfig config) => Config = config; + + public TestConfig Config { get; } + + public void Configure(CompositionBuilder builder) + { + } + } + + // A single public constructor that itself throws - ConfigProfileBinder.Invoke's + // TargetInvocationException-unwrapping, config-construction case (PR #65 review: + // ConstructorInfo.Invoke wraps a constructor-thrown exception in TargetInvocationException; + // without unwrapping, ApplyProfile's own catch (CompositionException) never observes this). + public sealed class ThrowingTestConfig + { + public ThrowingTestConfig(string value) => throw new CompositionException($"custom validation failed for '{value}'"); + } + + // Same reasoning as ThrowingTestConfig above, but for the profile-construction call site instead + // of the config-construction one. + public sealed class ThrowingTestProfile : ICompositionProfile + { + public ThrowingTestProfile(TestConfig config) => throw new CompositionException($"custom validation failed for '{config.Value}'"); + + public void Configure(CompositionBuilder builder) + { + } + } + // Mirrors CollectionPlan.scriban's own HashSet 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 diff --git a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs index 5b701fb..a7ae173 100644 --- a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs +++ b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs @@ -23,6 +23,7 @@ public void Assembly_ExposesExactlyTheDocumentedPublicTypes() [ "Compono.XunitV3.ComposeAttribute", "Compono.XunitV3.ComposeAttribute`1", + "Compono.XunitV3.ComposeAttribute`2", "Compono.XunitV3.SharedAttribute", ]); }