From 68975407a3481d88a31eedc72b248647eeb13382 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 21:00:41 -0400 Subject: [PATCH 1/8] feat(xunitv3): add ComposeAttribute for call-site profile configuration Compono.XunitV3's [Compose] can only select a fixed, default-constructed profile - real migration evidence from a much larger AutoFixture test suite (RESEARCH-0002) showed no clean way for a compile-time-constant value known at a test's call site to reach configuration logic running inside a profile (e.g. a parameterized PersistenceAutoData(repositoryName)-shaped custom AutoDataAttribute). Adds ComposeAttribute: profile configuration arguments bind positionally to TConfig's single public constructor (reusing the existing inline-value validation), TProfile is built from that TConfig via its own single qualifying constructor, and the result flows through the already-existing, unchanged AddProfile(ICompositionProfile) core API - zero changes to core Compono. Both constructor-shape checks are deterministic, cached-once-per-attribute-instance runtime diagnostics (no "best match" heuristic), a documented tradeoff against [Compose]'s compile-time new() enforcement. Also documents CompositionProviderRequest.Name-based provider matching (a separate, previously-undocumented capability RESEARCH-0002 flagged as a real migration-guide gap) and updates the published compono skill/evals so an agent recognizes the parameterized-AutoDataAttribute migration pattern instead of recommending a combinatorial subclass explosion, an inline Composer.Create escape hatch, or invented ambient scenario state. See ADR-0036 and PLAN-0036 for the full design/evaluation record. Co-Authored-By: Claude Sonnet 5 --- ...meterized-composition-profile-selection.md | 486 ++++++++++++++++++ docs/adr/README.md | 1 + docs/how-to/use-profiles.md | 12 + docs/migrating-from-autofixture.md | 143 +++++- docs/packages/compono-xunitv3.md | 69 +++ ...e-values-influencing-nested-composition.md | 402 +++++++++++++++ docs/plans/README.md | 1 + .../Compono.XunitV3.ComposeAttribute.md | 1 + ...ile,TConfig_.ComposeAttribute(object[]).md | 21 + ...itV3.ComposeAttribute_TProfile,TConfig_.md | 62 +++ .../api/Compono.XunitV3/Compono.XunitV3.md | 1 + .../0002-trivia-platform-comparison.md | 283 ++++++++++ docs/roadmap/post-mvp.md | 36 +- docs/troubleshooting/common-errors.md | 37 ++ skills/compono-evals/evals.json | 13 + skills/compono/SKILL.md | 9 + .../references/patterns-and-antipatterns.md | 18 + .../registrations-profiles-and-scopes.md | 45 ++ skills/compono/references/xunit-v3.md | 54 ++ .../Binding/ConfigProfileBinder.cs | 150 ++++++ src/Compono.XunitV3/ComposeAttribute.cs | 26 +- .../ComposeAttribute{TProfile,TConfig}.cs | 74 +++ .../ConfigProfileTests.cs | 74 +++ .../ComposeAttributeConfigBindingTests.cs | 162 ++++++ .../Fixtures/SampleTestMethods.cs | 59 +++ .../PublicApiSurfaceTests.cs | 1 + 26 files changed, 2226 insertions(+), 14 deletions(-) create mode 100644 docs/adr/0036-parameterized-composition-profile-selection.md create mode 100644 docs/plans/0036-call-site-values-influencing-nested-composition.md create mode 100644 docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.ComposeAttribute(object[]).md create mode 100644 docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute_TProfile,TConfig_.md create mode 100644 docs/research/0002-trivia-platform-comparison.md create mode 100644 src/Compono.XunitV3/Binding/ConfigProfileBinder.cs create mode 100644 src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs create mode 100644 test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs create mode 100644 test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs 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..cec03c4 --- /dev/null +++ b/docs/adr/0036-parameterized-composition-profile-selection.md @@ -0,0 +1,486 @@ +# [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. + +## 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/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..c101ab9 --- /dev/null +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -0,0 +1,402 @@ +# [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] No change needed to `src/Compono.XunitV3/Binding/BindingPlan.cs` — + see Notes for why the originally-sketched approach (extending + `BindingPlan`) turned out unnecessary. + +**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` and + `docs/packages/index.md`'s one-line package-table cell don't + enumerate every Compose-family form individually (adding a third + form to a summary cell would clutter it, not help — the package's + own guide is the right place, already updated), so left unchanged + by design; `docs/how-to/use-profiles.md` did enumerate the + `[Compose]` constraint specifically and got a new + paragraph pointing to the config form. + +**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 (pure extraction: + `NormalizeParamsArguments` pulled out, behavior unchanged); left + `ComposeAttribute{TProfile}.cs` untouched. +- `test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs` — new, + 10 cases. +- `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). +- `docs/packages/compono-xunitv3.md`, `docs/migrating-from-autofixture.md`, + `docs/troubleshooting/common-errors.md`, `docs/how-to/use-profiles.md`, + `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`, `skills/compono-evals/evals.json` — + published-skill updates, in scope per ADR-0035. + +## Test Plan + +**All executed; full solution `dotnet test` (`Compono.slnx`, Debug): +893 passed, 0 failed, 0 skipped** (11 new `Compono.XunitV3.Tests` cases: +10 `ComposeAttributeConfigBindingTests` plus +`ConfigArguments_AreNeverBoundAsInlineValues`, per TFM). +`Compono.XunitV3.SampleTests` (excluded +from `Compono.slnx`/CI by design — see its own csproj comment — run +manually per this plan): 20 passed (10 per TFM, including the two new +`ComposeAttribute` passing cases) + 2 pre-existing +`FailingCompositionTests` failures (expected, unrelated) + 2 new +deliberate `ConfigProfileTests` failures (expected, see below), per TFM. + +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 (e.g. a `TConfig` with two public + constructors) asserted to fail before the test method ever executes, + with the expected diagnostic text. + +**`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 one + deliberately-failing theory 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 — + full solution suite (891 tests) passes; no existing test file's + assertions were modified, only `PublicApiSurfaceTests.cs`'s exact-set + list extended (expected, additive) and `ComposeAttribute.cs`'s + normalization logic extracted 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 at all.** 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: `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. 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..494dff9 100644 --- a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md @@ -16,6 +16,7 @@ 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 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..59f6ab5 100644 --- a/docs/roadmap/post-mvp.md +++ b/docs/roadmap/post-mvp.md @@ -12,7 +12,7 @@ acceptable alternatives do not become roadmap items" — this page is not a general findings log, and non-candidate findings belong in the research record and their governing ADR's Amendments, not here. -## Current state: no roadmap candidates +## Current state: one roadmap candidate Milestone 7's dogfooding pass (migrating `ncipollina/cosmere-tracker`'s AutoFixture-based test kit to Compono) surfaced ten findings. **None were @@ -28,5 +28,35 @@ 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. +not an exhaustive survey) — and a second survey did. + +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.** `trivia-platform`'s + custom `AutoDataAttribute` subclasses overwhelmingly take runtime + constructor arguments (e.g. `PersistenceAutoData(repositoryName)` — ~45 + call sites; `AnnouncementsAutoData` — 8 boolean/locale parameters, 18 + call sites) that change what a composition decision made *inside* the + composed graph produces, not just which top-level type gets composed — + distinct from the already-solved cases of resolution-site-name matching + (`ICompositionValueProvider`) and fixed member overrides (`.Member()`). + Compono has no documented way for a compile-time-constant value known at + the test call site to reach that nested decision. **Impact:** high — + every current workaround (a dedicated profile per configuration variant, + or an inline `Composer.Create` call per test) trades away the concise, + declarative attribute-based idiom. **Confidence:** high (real, high-frequency call sites; no identified + principle conflict). **Recorded in:** + [ADR-0036](../adr/0036-parameterized-composition-profile-selection.md) + — a deep-design pass has since run and the ADR is now `Accepted`: a new + `Compono.XunitV3`-only `ComposeAttribute` attribute + binds compile-time-constant **profile configuration arguments** + positionally to a `TConfig` type, which then constructs an + `ICompositionProfile` via the already-existing, unchanged + `AddProfile(ICompositionProfile)` core API — zero changes to core + `Compono`. Implemented and shipped — + [PLAN-0036](../plans/0036-call-site-values-influencing-nested-composition.md) + is `Done`; see [`Compono.XunitV3`'s Package Guide](../packages/compono-xunitv3.md#profile-configuration-arguments) + for usage. diff --git a/docs/troubleshooting/common-errors.md b/docs/troubleshooting/common-errors.md index dc28c40..117ed2a 100644 --- a/docs/troubleshooting/common-errors.md +++ b/docs/troubleshooting/common-errors.md @@ -86,6 +86,43 @@ 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" + +Three 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): + +- **`'{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. +- **`'{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. +- **`'{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..3353101 100644 --- a/skills/compono/SKILL.md +++ b/skills/compono/SKILL.md @@ -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()`/ 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.XunitV3/Binding/ConfigProfileBinder.cs b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs new file mode 100644 index 0000000..292a67c --- /dev/null +++ b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs @@ -0,0 +1,150 @@ +using System.Reflection; + +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]; + + if (value is null) + { + if (!IsNullable(nullabilityContext, parameter)) + { + throw new CompositionException( + $"Profile configuration argument for parameter '{parameter.Name}' of '{configType}' is null, but the parameter is not nullable."); + } + + arguments[i] = null; + 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 and + // is what makes e.g. an int argument valid for an int? parameter. Same rule as + // ComposeAttribute's own inline-value validation (ADR-0022). + var underlyingType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; + + if (!underlyingType.IsInstanceOfType(value)) + { + 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 constructor.Invoke(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)constructor.Invoke([config]); + } + + private static ConstructorInfo ResolveSingleConstructor(Type 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) + { + 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/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 34360da..8e189b8 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -60,16 +60,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); } @@ -274,6 +265,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. diff --git a/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs new file mode 100644 index 0000000..bdcc3b1 --- /dev/null +++ b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs @@ -0,0 +1,74 @@ +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) + { + var config = ConfigProfileBinder.BindConfig(typeof(TConfig), _configArguments); + var profile = ConfigProfileBinder.BuildProfile(config); + + builder.AddProfile(profile); + } +} diff --git a/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs new file mode 100644 index 0000000..477348c --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs @@ -0,0 +1,74 @@ +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); + +// 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 +// MismatchedProfileConstructorShape_FailsBeforeTheTestExecutes 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(string repositoryName) + { + repositoryName.Should().Be("player-repository"); + } + + [Theory] + [Compose(RepositoryKind.Game)] + public void DifferentConfigArguments_ProduceADifferentlyConfiguredProfile(string repositoryName) + { + repositoryName.Should().Be("game-repository"); + } + + // 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' existing pattern (this whole project is deliberately excluded from + // Compono.slnx/CI for exactly this reason - it's packaged-consumer verification, run manually, not + // an automated gate). + [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/ComposeAttributeConfigBindingTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs new file mode 100644 index 0000000..347a7c3 --- /dev/null +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs @@ -0,0 +1,162 @@ +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_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_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..38d0c1f 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -119,6 +119,65 @@ 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"); + } + + // 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) + { + } + } + // 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", ]); } From 3c1b59d3c352243013d7ff7c643c20ed3a2657d0 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 21:58:25 -0400 Subject: [PATCH 2/8] fix(generators): register ComposeAttribute for plan discovery Codex review on PR #65 caught that ComposeMethodDiscovery was only registered against the non-generic and one-type-parameter ComposeAttribute metadata names - ComposeAttribute's own arity-suffixed metadata name (ComposeAttribute`2) was never registered with the generator, so a concrete parameter type reached only through [Compose] got no generated ICompositionPlan at all and would fail at GetData time. The packaged sample didn't catch this because its only composed parameter type was a registered string, which never needs a generated plan. Adds a third ForAttributeWithMetadataName registration for the arity-2 metadata name, merged into the same discovery pipeline as the other two arities. New isolated Compono.Generators.Tests snapshot proves a concrete type reached only this way now gets a plan; Compono.XunitV3.SampleTests' ConfigProfileTests now composes a real concrete class instead of a bare string, so the packaged sample actually exercises the fixed path. Also corrects docs/roadmap/post-mvp.md, which listed ADR-0036's shipped capability as an outstanding candidate - contradicting the page's own stated "not fully available" scope (docs/roadmap/index.md). Returned to a no-current-candidates state; the historical evidence trail stays intact via ADR-0036/RESEARCH-0002/PLAN-0036. Co-Authored-By: Claude Sonnet 5 --- ...e-values-influencing-nested-composition.md | 33 ++++++++ docs/roadmap/post-mvp.md | 78 ++++++++----------- .../ComponoIncrementalGenerator.cs | 26 +++++-- .../Discovery/ComposeMethodDiscovery.cs | 12 +++ .../CompositionPlanVerifyTests.cs | 73 +++++++++++++++++ ...ote_2e911510.CompositionPlan.g.verified.cs | 22 ++++++ .../ConfigProfileTests.cs | 25 +++++- 7 files changed, 214 insertions(+), 55 deletions(-) create mode 100644 test/Compono.Generators.Tests/Snapshots/CompositionPlanVerifyTests.ComposeTwoTypeParameterAttributedMethodParameter_GeneratesCompositionPlan#TestNamespace.CreditNote_2e911510.CompositionPlan.g.verified.cs diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index c101ab9..acb658e 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -400,3 +400,36 @@ stack-traced through `ConfigProfileBinder.ResolveSingleProfileConstructor` 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. diff --git a/docs/roadmap/post-mvp.md b/docs/roadmap/post-mvp.md index 59f6ab5..62a00b6 100644 --- a/docs/roadmap/post-mvp.md +++ b/docs/roadmap/post-mvp.md @@ -12,51 +12,39 @@ acceptable alternatives do not become roadmap items" — this page is not a general findings log, and non-candidate findings belong in the research record and their governing ADR's Amendments, not here. -## Current state: one roadmap candidate +## 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) — and a second survey did. - -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.** `trivia-platform`'s - custom `AutoDataAttribute` subclasses overwhelmingly take runtime - constructor arguments (e.g. `PersistenceAutoData(repositoryName)` — ~45 - call sites; `AnnouncementsAutoData` — 8 boolean/locale parameters, 18 - call sites) that change what a composition decision made *inside* the - composed graph produces, not just which top-level type gets composed — - distinct from the already-solved cases of resolution-site-name matching - (`ICompositionValueProvider`) and fixed member overrides (`.Member()`). - Compono has no documented way for a compile-time-constant value known at - the test call site to reach that nested decision. **Impact:** high — - every current workaround (a dedicated profile per configuration variant, - or an inline `Composer.Create` call per test) trades away the concise, - declarative attribute-based idiom. **Confidence:** high (real, high-frequency call sites; no identified - principle conflict). **Recorded in:** +- 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) - — a deep-design pass has since run and the ADR is now `Accepted`: a new - `Compono.XunitV3`-only `ComposeAttribute` attribute - binds compile-time-constant **profile configuration arguments** - positionally to a `TConfig` type, which then constructs an - `ICompositionProfile` via the already-existing, unchanged - `AddProfile(ICompositionProfile)` core API — zero changes to core - `Compono`. Implemented and shipped — - [PLAN-0036](../plans/0036-call-site-values-influencing-nested-composition.md) - is `Done`; see [`Compono.XunitV3`'s Package Guide](../packages/compono-xunitv3.md#profile-configuration-arguments) - for usage. + 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/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/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 index 477348c..9176763 100644 --- a/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs +++ b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs @@ -11,6 +11,23 @@ public enum RepositoryKind 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 @@ -46,16 +63,16 @@ public sealed class ConfigProfileTests { [Theory] [Compose(RepositoryKind.Player)] - public void ComposesTheProfileBuiltFromConfigArguments(string repositoryName) + public void ComposesTheProfileBuiltFromConfigArguments(RepositoryConsumer consumer) { - repositoryName.Should().Be("player-repository"); + consumer.RepositoryName.Should().Be("player-repository"); } [Theory] [Compose(RepositoryKind.Game)] - public void DifferentConfigArguments_ProduceADifferentlyConfiguredProfile(string repositoryName) + public void DifferentConfigArguments_ProduceADifferentlyConfiguredProfile(RepositoryConsumer consumer) { - repositoryName.Should().Be("game-repository"); + consumer.RepositoryName.Should().Be("game-repository"); } // Deliberately fails, on every run, via ConfigProfileBinder's own pre-composition constructor-shape From 3c711c16e0460cff3c3cfcfb955db4e9b63b71b8 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 22:12:21 -0400 Subject: [PATCH 3/8] fix(xunitv3): append seed and reject abstract types in config/profile binding Second Codex review round on PR #65 caught two more real gaps in ComposeAttribute's config/profile binding: - ApplyProfile runs inside the base class's Lazy initialization, before GetData ever creates a CompositionRow, so a ConfigProfileBinder failure had no row.Seed to read and escaped without the "Seed: ..." suffix every other Compono.XunitV3-owned pre-composition failure carries (ADR-0022). ApplyProfile now catches CompositionException and rethrows via the existing CompositionException.WithSeedInMessage helper. - An abstract TConfig/TProfile with an otherwise-qualifying public constructor passed the "exactly one constructor" check and then threw MemberAccessException from ConstructorInfo.Invoke instead of the documented CompositionException. Both constructor-resolution methods now explicitly reject abstract types first. Regression tests added for both; documented in docs/troubleshooting/common-errors.md. Full solution: 903/903 passed. Co-Authored-By: Claude Sonnet 5 --- ...e-values-influencing-nested-composition.md | 32 +++++++++ docs/troubleshooting/common-errors.md | 17 ++++- .../Binding/ConfigProfileBinder.cs | 22 +++++++ .../ComposeAttribute{TProfile,TConfig}.cs | 24 ++++++- .../ComposeAttributeConfigBindingTests.cs | 65 +++++++++++++++++++ .../Fixtures/SampleTestMethods.cs | 29 +++++++++ 6 files changed, 184 insertions(+), 5 deletions(-) diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index acb658e..a2d6cc6 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -433,3 +433,35 @@ review round — the page's own stated purpose 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. diff --git a/docs/troubleshooting/common-errors.md b/docs/troubleshooting/common-errors.md index 117ed2a..45f0ba7 100644 --- a/docs/troubleshooting/common-errors.md +++ b/docs/troubleshooting/common-errors.md @@ -88,17 +88,27 @@ no last-write-wins fallback to rely on instead. ### "`ComposeAttribute` throws before my test even runs" -Three distinct, deterministic, pre-composition failures — all plain-message +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): +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 @@ -106,6 +116,9 @@ for the full design): 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 diff --git a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs index 292a67c..30f02ee 100644 --- a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs +++ b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs @@ -107,6 +107,19 @@ public static TProfile BuildProfile(object config) 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) @@ -120,6 +133,15 @@ private static ConstructorInfo ResolveSingleConstructor(Type type) 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 => { diff --git a/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs index bdcc3b1..8428821 100644 --- a/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs +++ b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs @@ -66,9 +66,27 @@ public ComposeAttribute(params object?[] configArguments) : base() internal override void ApplyProfile(CompositionBuilder builder) { - var config = ConfigProfileBinder.BindConfig(typeof(TConfig), _configArguments); - var profile = ConfigProfileBinder.BuildProfile(config); + try + { + var config = ConfigProfileBinder.BindConfig(typeof(TConfig), _configArguments); + var profile = ConfigProfileBinder.BuildProfile(config); - builder.AddProfile(profile); + 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) - 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.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs index 347a7c3..365b2b6 100644 --- a/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs @@ -101,6 +101,71 @@ 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_Throws_WhenAProfileConfigurationArgumentHasAnIncompatibleType() { diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index 38d0c1f..5fb1d92 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -178,6 +178,35 @@ 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) + { + } + } + // 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 From d5c44ebf5e233d6ebcd9c42ea5f325e571a721fd Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 22:25:47 -0400 Subject: [PATCH 4/8] fix(xunitv3): preserve negative-seed priority and unwrap constructor exceptions Third Codex review round on PR #65: - A negative configured seed (Seed = -1) combined with an invalid config/profile shape reported the binder failure with "Seed: -1" embedded instead of the documented negative-seed diagnostic. ApplyProfile now checks for a negative SeedAsNullable before attempting any config/profile binding, reusing the base class's exact negative-seed message (AppendSeed promoted to private protected so both share it). - ConstructorInfo.Invoke wraps any exception a TConfig/TProfile constructor itself throws in TargetInvocationException, so ApplyProfile's catch (CompositionException) never observed a constructor's own actionable exception. ConfigProfileBinder's shared Invoke helper now unwraps TargetInvocationException via ExceptionDispatchInfo.Capture(...).Throw(), preserving the original stack trace, for both the TConfig and TProfile construction call sites. Regression tests added for both. Full solution: 909/909 passed. Co-Authored-By: Claude Sonnet 5 --- ...e-values-influencing-nested-composition.md | 30 +++++++++++++ .../Binding/ConfigProfileBinder.cs | 28 +++++++++++- src/Compono.XunitV3/ComposeAttribute.cs | 6 ++- .../ComposeAttribute{TProfile,TConfig}.cs | 28 +++++++++--- .../ComposeAttributeConfigBindingTests.cs | 45 +++++++++++++++++++ .../Fixtures/SampleTestMethods.cs | 20 +++++++++ 6 files changed, 148 insertions(+), 9 deletions(-) diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index a2d6cc6..5251064 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -465,3 +465,33 @@ Both fixes have dedicated `Compono.XunitV3.Tests` regression coverage `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. diff --git a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs index 30f02ee..4e1b3ee 100644 --- a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs +++ b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs @@ -1,4 +1,5 @@ using System.Reflection; +using System.Runtime.ExceptionServices; namespace Compono.XunitV3.Binding; @@ -85,7 +86,7 @@ public static object BindConfig(Type configType, IReadOnlyList configAr arguments[i] = value; } - return constructor.Invoke(arguments); + return Invoke(constructor, arguments); } /// @@ -102,7 +103,30 @@ public static TProfile BuildProfile(object config) { var constructor = ResolveSingleProfileConstructor(typeof(TProfile), typeof(TConfig)); - return (TProfile)constructor.Invoke([config]); + 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) diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 8e189b8..685aaac 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -302,7 +302,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 index 8428821..c78bc12 100644 --- a/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs +++ b/src/Compono.XunitV3/ComposeAttribute{TProfile,TConfig}.cs @@ -66,6 +66,21 @@ public ComposeAttribute(params object?[] configArguments) : base() 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); @@ -79,12 +94,13 @@ internal override void ApplyProfile(CompositionBuilder builder) // 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) - 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. + // 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.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs index 365b2b6..f3f5438 100644 --- a/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs @@ -166,6 +166,51 @@ 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() { diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index 5fb1d92..d71539c 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -207,6 +207,26 @@ 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 From 6dc47deaa376a899fc82a1a8215bc9ab8d492020 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 22:39:01 -0400 Subject: [PATCH 5/8] docs(xunitv3): amend ADR-0036 and refresh stale extension-point docs Fourth Codex review round on PR #65: - ADR-0036's "Reflection is bounded and cached" section specified the same cached-delegate (MakeGenericMethod/Delegate.CreateDelegate) shape ADR-0022 uses for RowInvokers - the actual implementation uses direct ConstructorInfo.Invoke instead, relying on the base attribute's existing Lazy caching for the "once per attribute instance" guarantee. Per this repo's Amendment mechanic, that correction now has its own dated Amendment 1 on the ADR, not just a plan-side note. - ComposeAttribute's XML remarks still described ComposeAttribute as "the one designed extension point," and Compono.XunitV3.csproj's package Description listed only [Compose]/[Compose]. Both updated to describe the new ComposeAttribute form; API reference regenerated from the corrected XML docs. A third finding (move the CompositionProviderRequest.Name migration-guide section to a separate PR) was reviewed and deliberately not actioned - including it in this PR was an explicit user instruction when this plan was scoped, not an oversight; replied on the thread explaining this. Full solution: 909/909 passed. Co-Authored-By: Claude Sonnet 5 --- ...meterized-composition-profile-selection.md | 44 +++++++++++++++++++ ...e-values-influencing-nested-composition.md | 37 ++++++++++++++++ .../Compono.XunitV3.ComposeAttribute.md | 18 +++++--- src/Compono.XunitV3/Compono.XunitV3.csproj | 9 ++-- src/Compono.XunitV3/ComposeAttribute.cs | 18 +++++--- 5 files changed, 110 insertions(+), 16 deletions(-) diff --git a/docs/adr/0036-parameterized-composition-profile-selection.md b/docs/adr/0036-parameterized-composition-profile-selection.md index cec03c4..847f3e9 100644 --- a/docs/adr/0036-parameterized-composition-profile-selection.md +++ b/docs/adr/0036-parameterized-composition-profile-selection.md @@ -467,6 +467,50 @@ consumer exists. 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) — diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index 5251064..f78c6be 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -495,3 +495,40 @@ Regression coverage: `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. diff --git a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md index 494dff9..6cb9e41 100644 --- a/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md +++ b/docs/reference/api/Compono.XunitV3/Compono.XunitV3.ComposeAttribute.md @@ -20,12 +20,18 @@ Derived ↳ [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/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 685aaac..9db7a8a 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 From 2df7c8122c96a86482f5ac11ddabb544a14491bc Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 23:00:08 -0400 Subject: [PATCH 6/8] fix(xunitv3): share positional binding validation, fix stacking message, fix CI gate Fifth Codex review round on PR #65, plus a live CI regression this round's own fix introduced: - ConfigProfileBinder independently reimplemented the exact null/ Nullable-unwrap/assignability check ComposeAttribute.GetData's own inline-value binding already had, contradicting ADR-0036's "retargeted rather than reimplemented" promise. Extracted both into a new shared Binding/PositionalArgumentBinder.cs, used by both call sites. - No regression test covered a non-null value-typed config argument against a Nullable constructor parameter (e.g. 42 for int?). Added fixtures and a dedicated test. - BindingPlan's Compose-family-stacking diagnostic still named only the original two attribute forms. Updated the message and added regression coverage combining the new form with [Compose]. - Live CI regression (caught by the user via the package-validation workflow, not by Codex): that workflow's "Local-feed packed-consumer smoke test" step runs Compono.XunitV3.SampleTests directly, filtered to exclude only FailingCompositionTests by exact name. The new ConfigProfileTests.cs's own deliberately-failing test lived inside an otherwise-green class, so it wasn't excluded and failed the gate. Moved it into its own FailingConfigProfileTests class (mirroring FailingCompositionTests' pattern) and changed the workflow filter to a trailing wildcard covering the whole naming convention, verified directly against the MTP CLI (a mid-string wildcard is rejected). Full solution: 913/913 passed. Verified the corrected package-validation smoke-test command locally: 20/20 passed. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/package-validation.yaml | 20 +++++-- ...e-values-influencing-nested-composition.md | 54 ++++++++++++++++++ src/Compono.XunitV3/Binding/BindingPlan.cs | 11 ++-- .../Binding/ConfigProfileBinder.cs | 24 ++------ .../Binding/PositionalArgumentBinder.cs | 57 +++++++++++++++++++ src/Compono.XunitV3/ComposeAttribute.cs | 24 ++------ .../ConfigProfileTests.cs | 21 +------ .../FailingConfigProfileTests.cs | 21 +++++++ .../Compono.XunitV3.Tests/BindingPlanTests.cs | 14 +++++ .../ComposeAttributeConfigBindingTests.cs | 17 ++++++ .../Fixtures/SampleTestMethods.cs | 26 +++++++++ 11 files changed, 226 insertions(+), 63 deletions(-) create mode 100644 src/Compono.XunitV3/Binding/PositionalArgumentBinder.cs create mode 100644 test/Compono.XunitV3.SampleTests/FailingConfigProfileTests.cs 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/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index f78c6be..b32b780 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -532,3 +532,57 @@ deliberately not actioned:** 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. 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 index 4e1b3ee..8620f39 100644 --- a/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs +++ b/src/Compono.XunitV3/Binding/ConfigProfileBinder.cs @@ -58,29 +58,17 @@ public static object BindConfig(Type configType, IReadOnlyList configAr { var parameter = parameters[i]; var value = configArguments[i]; + var nullability = IsNullable(nullabilityContext, parameter) ? Nullability.Nullable : Nullability.NotNullable; - if (value is null) + switch (PositionalArgumentBinder.Validate(parameter.ParameterType, nullability, value)) { - if (!IsNullable(nullabilityContext, parameter)) - { + case PositionalArgumentValidation.NullNotAllowed: throw new CompositionException( $"Profile configuration argument for parameter '{parameter.Name}' of '{configType}' is null, but the parameter is not nullable."); - } - arguments[i] = null; - 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 and - // is what makes e.g. an int argument valid for an int? parameter. Same rule as - // ComposeAttribute's own inline-value validation (ADR-0022). - var underlyingType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; - - if (!underlyingType.IsInstanceOfType(value)) - { - throw new CompositionException( - $"Profile configuration argument for parameter '{parameter.Name}' of '{configType}' has type '{value.GetType()}', which is not assignable to '{parameter.ParameterType}'."); + 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; 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/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 9db7a8a..d18699b 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -186,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)); } } diff --git a/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs index 9176763..ba727b9 100644 --- a/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs +++ b/test/Compono.XunitV3.SampleTests/ConfigProfileTests.cs @@ -48,10 +48,9 @@ public void Configure(CompositionBuilder builder) => } // Deliberately has no constructor accepting a RepositoryTestConfig - reserved for -// MismatchedProfileConstructorShape_FailsBeforeTheTestExecutes 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). +// 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) @@ -74,18 +73,4 @@ public void DifferentConfigArguments_ProduceADifferentlyConfiguredProfile(Reposi { consumer.RepositoryName.Should().Be("game-repository"); } - - // 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' existing pattern (this whole project is deliberately excluded from - // Compono.slnx/CI for exactly this reason - it's packaged-consumer verification, run manually, not - // an automated gate). - [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.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 index f3f5438..de839b6 100644 --- a/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConfigBindingTests.cs @@ -237,6 +237,23 @@ 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() { diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index d71539c..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 @@ -145,6 +155,22 @@ public sealed class NullableParameterizedTestProfile : ICompositionProfile 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 { From 7d31a7fc2bd9f6645276603b90ca3e20d2a152f6 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 23:12:40 -0400 Subject: [PATCH 7/8] docs(xunitv3): list the new Compose form in remaining discovery surfaces Sixth Codex review round on PR #65: docs/packages/index.md's top-level package catalog table and skills/compono/SKILL.md's Detection table and reference table still enumerated only [Compose]/[Compose] - round 4 fixed the base attribute's XML docs and the NuGet package description but missed these. Fixed all three, then grepped the whole repo's markdown for the same stale pattern to confirm nothing else was missed. Co-Authored-By: Claude Sonnet 5 --- docs/packages/index.md | 2 +- ...call-site-values-influencing-nested-composition.md | 11 +++++++++++ skills/compono/SKILL.md | 4 ++-- 3 files changed, 14 insertions(+), 3 deletions(-) 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 index b32b780..4517aac 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -586,3 +586,14 @@ by the MTP CLI). Verified locally with the exact corrected command: 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). diff --git a/skills/compono/SKILL.md b/skills/compono/SKILL.md index 3353101..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 | @@ -198,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 | From eae1263298f0876e7447c1b25e48c008c78af2b4 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Sat, 8 Aug 2026 23:27:59 -0400 Subject: [PATCH 8/8] docs(plans): refresh PLAN-0036's stale summary sections to final state Seventh Codex review round on PR #65: as a Done plan, PLAN-0036's Test Plan summary, Critical Files, and Verification-checklist sections are the record maintainers rely on, but still described the pre-review-round state - stale test counts (893/891 vs. the actual final 913), a "left unchanged by design" claim round 6's own fix reversed, an "excluded from CI" claim already corrected in code comments but not here, and a "no BindingPlan.cs changes" claim round 5's own diagnostic-message fix quietly contradicted. Edited those sections directly to the final, true state rather than adding another dated note on top of six already-accumulated ones - this is a living plan document, not an ADR, so a correction here is a direct edit. The round-by-round Notes entries stay as the historical trail; only the summary sections a reader checks first for final state needed refreshing. Co-Authored-By: Claude Sonnet 5 --- ...e-values-influencing-nested-composition.md | 146 +++++++++++++----- 1 file changed, 107 insertions(+), 39 deletions(-) diff --git a/docs/plans/0036-call-site-values-influencing-nested-composition.md b/docs/plans/0036-call-site-values-influencing-nested-composition.md index 4517aac..7feb0bf 100644 --- a/docs/plans/0036-call-site-values-influencing-nested-composition.md +++ b/docs/plans/0036-call-site-values-influencing-nested-composition.md @@ -134,9 +134,13 @@ not something to decide as a side effect of one feature's plan. constructor for its own, separate `configArguments` parameter — a behavior-preserving refactor (see Notes), not a change to existing binding semantics. -- [x] No change needed to `src/Compono.XunitV3/Binding/BindingPlan.cs` — - see Notes for why the originally-sketched approach (extending - `BindingPlan`) turned out unnecessary. +- [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): @@ -172,14 +176,18 @@ not something to decide as a side effect of one feature's plan. 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` and - `docs/packages/index.md`'s one-line package-table cell don't - enumerate every Compose-family form individually (adding a third - form to a summary cell would clutter it, not help — the package's - own guide is the right place, already updated), so left unchanged - by design; `docs/how-to/use-profiles.md` did enumerate the +- [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. + 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 @@ -207,40 +215,76 @@ new one per package): 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 (pure extraction: - `NormalizeParamsArguments` pulled out, behavior unchanged); left +- `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, - 10 cases. + 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). + 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`, `skills/compono-evals/evals.json` — + `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 -**All executed; full solution `dotnet test` (`Compono.slnx`, Debug): -893 passed, 0 failed, 0 skipped** (11 new `Compono.XunitV3.Tests` cases: -10 `ComposeAttributeConfigBindingTests` plus -`ConfigArguments_AreNeverBoundAsInlineValues`, per TFM). -`Compono.XunitV3.SampleTests` (excluded -from `Compono.slnx`/CI by design — see its own csproj comment — run -manually per this plan): 20 passed (10 per TFM, including the two new -`ComposeAttribute` passing cases) + 2 pre-existing -`FailingCompositionTests` failures (expected, unrelated) + 2 new -deliberate `ConfigProfileTests` failures (expected, see below), per TFM. +**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) @@ -290,9 +334,11 @@ split, matching how ADR-0022's own binding algorithm was verified: 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 (e.g. a `TConfig` with two public - constructors) asserted to fail before the test method ever executes, - with the expected diagnostic text. +- 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: @@ -326,16 +372,18 @@ Explicit exit checklist — every item confirmed before this plan moves to 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 one - deliberately-failing theory all ran successfully against that real - packaged build (see Notes for the exact `dotnet test` output + 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 — - full solution suite (891 tests) passes; no existing test file's - assertions were modified, only `PublicApiSurfaceTests.cs`'s exact-set - list extended (expected, additive) and `ComposeAttribute.cs`'s - normalization logic extracted with no behavioral change (verified by + 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 @@ -364,10 +412,13 @@ 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 at all.** 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: `ComposeAttribute.ApplyProfile` is +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 @@ -597,3 +648,20 @@ 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.