From 69aabc4a4eea4cbfe0bd398eb98331ee4e5d666c Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 08:45:13 -0400 Subject: [PATCH 01/10] feat(core): milestone 4 phase 3 - test suites and verification Closes out PLAN-0004 Phase 3 (ADR-0022's Testing Strategy): nullable/ inline-null binding-algorithm coverage, seed-message-content proofs, a concurrency-stress test, a public-API-surface lock, and a new Compono.XunitV3.SampleTests project that consumes Compono.XunitV3 as a real packaged dependency (PackageReference against a local feed populated by dotnet pack, never a ProjectReference). That packaged-consumer setup immediately caught a real bug: Compono.XunitV3.csproj's ProjectReference to Compono had no asset-flow override, so dotnet pack applied the default PrivateAssets (contentfiles;build;analyzers), which excludes the generator from ever reaching a consumer that references only Compono.XunitV3. Fixed with PrivateAssets="none". SampleTests is deliberately left out of Compono.slnx (it has one theory that always fails, by design) and is instead exercised end to end by a new RealRunnerTests test that shells out `dotnet test` against it and asserts on the real pass/fail split and seed output. Co-Authored-By: Claude Sonnet 5 --- .gitignore | 4 + Directory.Packages.props | 4 + .../0004-milestone-4-xunit-integration.md | 114 +++++++++++++++-- src/Compono.XunitV3/Compono.XunitV3.csproj | 13 +- .../Compono.XunitV3.SampleTests.csproj | 42 +++++++ test/Compono.XunitV3.SampleTests/Domain.cs | 20 +++ .../FailingCompositionTests.cs | 15 +++ .../InlineAndComposedTests.cs | 37 ++++++ .../SharedTests.cs | 12 ++ test/Compono.XunitV3.SampleTests/nuget.config | 10 ++ .../xunit.runner.json | 3 + .../ComposeAttributeConcurrencyTests.cs | 21 ++++ .../Fixtures/SampleTestMethods.cs | 45 +++++++ .../InlineNullHandlingTests.cs | 117 ++++++++++++++++++ .../PublicApiSurfaceTests.cs | 24 ++++ test/Compono.XunitV3.Tests/RealRunnerTests.cs | 60 +++++++++ .../SeedReportingTests.cs | 52 ++++++++ 17 files changed, 580 insertions(+), 13 deletions(-) create mode 100644 test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj create mode 100644 test/Compono.XunitV3.SampleTests/Domain.cs create mode 100644 test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs create mode 100644 test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs create mode 100644 test/Compono.XunitV3.SampleTests/SharedTests.cs create mode 100644 test/Compono.XunitV3.SampleTests/nuget.config create mode 100644 test/Compono.XunitV3.SampleTests/xunit.runner.json create mode 100644 test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs create mode 100644 test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs create mode 100644 test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs create mode 100644 test/Compono.XunitV3.Tests/RealRunnerTests.cs create mode 100644 test/Compono.XunitV3.Tests/SeedReportingTests.cs diff --git a/.gitignore b/.gitignore index 4966531..89e618a 100644 --- a/.gitignore +++ b/.gitignore @@ -435,6 +435,10 @@ FodyWeavers.xsd .idea +# Local NuGet feed populated by Compono.XunitV3.SampleTests' pre-restore pack step +# (test/Compono.XunitV3.SampleTests/nuget.config) - regenerated on every restore, never committed. +.local-nuget-feed/ + # macOS .DS_Store .DS_Store? diff --git a/Directory.Packages.props b/Directory.Packages.props index fc6f54b..bc1e30e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -16,6 +16,10 @@ + + + diff --git a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj new file mode 100644 index 0000000..6dfd783 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj @@ -0,0 +1,42 @@ + + + + enable + enable + Exe + Compono.XunitV3.SampleTests + false + true + + true + true + + $(MSBuildThisFileDirectory)../../.local-nuget-feed + + + + + + + + + + + + + + + + + + diff --git a/test/Compono.XunitV3.SampleTests/Domain.cs b/test/Compono.XunitV3.SampleTests/Domain.cs new file mode 100644 index 0000000..1cf4294 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/Domain.cs @@ -0,0 +1,20 @@ +namespace Compono.XunitV3.SampleTests; + +// Reached only through SharedTests.SharedRepositoryIsReusedByTheService's own [Compose]-attributed +// theory parameters - no [Composable], no Create()/CreateMany(), no direct CompositionRow call +// site anywhere else in this project. Proves Compono.Generators.ComposeMethodDiscovery (Phase 1) +// generates a real plan through the packaged Compono.XunitV3 -> Compono dependency, not just an +// isolated Compono.Generators.Tests snapshot test. +public sealed class Repository; + +public sealed class OrderService +{ + public OrderService(Repository repository) + { + Repository = repository; + } + + public Repository Repository { get; } +} + +public sealed record CreateOrder(string CustomerName, int Quantity); diff --git a/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs new file mode 100644 index 0000000..9b12c82 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs @@ -0,0 +1,15 @@ +namespace Compono.XunitV3.SampleTests; + +// Deliberately fails, on every run - Compono.XunitV3.Tests' RealRunnerTests shells out `dotnet test` +// against this project and asserts the captured output contains "Seed:", proving the milestone's +// "a composition failure's message contains a seed" promise reaches a real xUnit v3 runner's actual +// output, not just an in-process GetData call. +public sealed class FailingCompositionTests +{ + [Theory] + [Compose(Seed = -1)] + public void DeliberatelyFailingComposition_NegativeSeedIsRejected(int value) + { + value.Should().Be(0, "GetData throws before this body ever runs - this line never executes"); + } +} diff --git a/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs b/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs new file mode 100644 index 0000000..690f1d7 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/InlineAndComposedTests.cs @@ -0,0 +1,37 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed class InlineAndComposedTests +{ + [Theory] + [Compose(42, "widget")] + public void InlineValuesAreUsedDirectly(int quantity, string productName) + { + quantity.Should().Be(42); + productName.Should().Be("widget"); + } + + [Theory] + [Compose] + public void ComposedValuesAreProducedForEveryParameter(int quantity, string productName) + { + ((object)quantity).Should().BeOfType(); + productName.Should().NotBeNull(); + } + + [Theory] + [Compose(42)] + public void MixesInlineAndComposedValues(int quantity, string productName) + { + quantity.Should().Be(42); + productName.Should().NotBeNull(); + } + + [Theory] + [Compose] + public async Task ComposesForAnAsyncTheory(string value) + { + await Task.Yield(); + + value.Should().NotBeNull(); + } +} diff --git a/test/Compono.XunitV3.SampleTests/SharedTests.cs b/test/Compono.XunitV3.SampleTests/SharedTests.cs new file mode 100644 index 0000000..9cbf9a1 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/SharedTests.cs @@ -0,0 +1,12 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed class SharedTests +{ + [Theory] + [Compose] + public void SharedRepositoryIsReusedByTheService([Shared] Repository repository, OrderService service, CreateOrder command) + { + service.Repository.Should().BeSameAs(repository); + command.Should().NotBeNull(); + } +} diff --git a/test/Compono.XunitV3.SampleTests/nuget.config b/test/Compono.XunitV3.SampleTests/nuget.config new file mode 100644 index 0000000..5d060c7 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/nuget.config @@ -0,0 +1,10 @@ + + + + + + + + + diff --git a/test/Compono.XunitV3.SampleTests/xunit.runner.json b/test/Compono.XunitV3.SampleTests/xunit.runner.json new file mode 100644 index 0000000..86c7ea0 --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/xunit.runner.json @@ -0,0 +1,3 @@ +{ + "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json" +} diff --git a/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs new file mode 100644 index 0000000..e00b21b --- /dev/null +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs @@ -0,0 +1,21 @@ +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +public sealed class ComposeAttributeConcurrencyTests +{ + [Fact] + public async Task GetData_ProducesNoExceptionsOrDataRaces_WhenCalledConcurrently_OnOneSharedAttributeInstance() + { + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.Simple))!; + + var calls = Enumerable.Range(0, 200) + .Select(_ => attribute.GetData(method, new DisposalTracker()).AsTask()); + + var results = await Task.WhenAll(calls); + + results.Should().OnlyContain(rows => rows.Single().GetData().Length == 2); + } +} diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index 23c5d3b..adbddd2 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -20,6 +20,49 @@ public static void WithNullableParameters(string? nullableReference, string notN { } + public static void WithNullableReferenceParameter(string? value) + { + } + + public static void WithNonNullableReferenceParameter(string value) + { + } + + public static void WithNullableValueParameter(int? value) + { + } + + public static void WithNonNullableValueParameter(int value) + { + } + + // Deliberately single-parameter - a second same-typed parameter would read this shared value + // back from scope too (Phase 0's stage-2 read gate applies to every same-typed request in the + // row, not just ones marked [Shared]) and re-validate it against that parameter's own + // nullability, which isn't what these fixtures exist to test. + public static void WithSharedNullableReferenceParameter([Shared] string? value) + { + } + + public static void WithSharedNonNullableReferenceParameter([Shared] string value) + { + } + + public static void WithSharedNullableValueParameter([Shared] int? value) + { + } + + public static void WithSharedNonNullableValueParameter([Shared] int value) + { + } + + // No provider and no generated plan can ever satisfy this interface (this test project doesn't + // reference Compono.Generators as an analyzer, and nothing registers it) - composing it always + // fails, deterministically, which is exactly what the seed-message-content proof needs. + public static void WithUnregisteredInterfaceParameter(IUnregisteredDependency value) + { + } + public static void WithDisposableParameter(DisposableValue disposable) { } @@ -83,3 +126,5 @@ public sealed class DisposableValue : IDisposable public void Dispose() => DisposeCount++; } + +internal interface IUnregisteredDependency; diff --git a/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs b/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs new file mode 100644 index 0000000..84d8753 --- /dev/null +++ b/test/Compono.XunitV3.Tests/InlineNullHandlingTests.cs @@ -0,0 +1,117 @@ +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +// The four null/non-null-parameter combinations Phase 3's plan requires, each covered for both an +// ordinary parameter and an inline-[Shared] parameter - proving rejection happens before +// ShareExplicit's invoker is ever reached (a rejected inline value fails GetData's own +// pre-composition validation loop, which runs before either binding loop touches a parameter). +public sealed class InlineNullHandlingTests +{ + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForANullableReferenceParameter() + { + var attribute = new ComposeAttribute((string?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableReferenceParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForANullableValueParameter() + { + var attribute = new ComposeAttribute((int?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForANonNullableReferenceParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableReferenceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForANonNullableValueParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNonNullableValueParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForASharedNullableReferenceParameter() + { + var attribute = new ComposeAttribute((string?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNullableReferenceParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_AcceptsANullInlineValue_ForASharedNullableValueParameter() + { + var attribute = new ComposeAttribute((int?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().BeNull(); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForASharedNonNullableReferenceParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNonNullableReferenceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_RejectsANullInlineValue_ForASharedNonNullableValueParameter() + { + var attribute = new ComposeAttribute((object?)null); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithSharedNonNullableValueParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + + await act.Should().ThrowAsync() + .WithMessage("*value*"); + } + + [Fact] + public async Task GetData_AcceptsANonNullInlineValue_ForANullableValueParameter() + { + // The regression test for the boxed-T-vs-boxed-Nullable CLR bug: a non-null int arrives + // boxed as a boxed int, not a boxed int? - without the Nullable.GetUnderlyingType unwrap this + // is wrongly rejected against the raw int? parameter type. + var attribute = new ComposeAttribute(42); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithNullableValueParameter))!; + + var rows = await attribute.GetData(method, new DisposalTracker()); + + rows.Single().GetData()[0].Should().Be(42); + } +} diff --git a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs new file mode 100644 index 0000000..1fd32b1 --- /dev/null +++ b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs @@ -0,0 +1,24 @@ +namespace Compono.XunitV3.Tests; + +// Cheap insurance against accidental public-API drift (Phase 3's plan task) - locks the exact set of +// public types Compono.XunitV3 exposes, matching this milestone's "keep public APIs minimal" +// constraint (docs/public-api.md). A hand-rolled exact-set assertion, not a Verify snapshot: the +// expected shape is a fixed, short list, so a snapshot file would add ceremony without adding +// coverage testing.md doesn't already ask for. +public sealed class PublicApiSurfaceTests +{ + [Fact] + public void Assembly_ExposesExactlyTheDocumentedPublicTypes() + { + var publicTypeNames = typeof(ComposeAttribute).Assembly.GetTypes() + .Where(static type => type.IsPublic) + .Select(static type => type.FullName); + + publicTypeNames.Should().BeEquivalentTo( + [ + "Compono.XunitV3.ComposeAttribute", + "Compono.XunitV3.ComposeAttribute`1", + "Compono.XunitV3.SharedAttribute", + ]); + } +} diff --git a/test/Compono.XunitV3.Tests/RealRunnerTests.cs b/test/Compono.XunitV3.Tests/RealRunnerTests.cs new file mode 100644 index 0000000..428eca3 --- /dev/null +++ b/test/Compono.XunitV3.Tests/RealRunnerTests.cs @@ -0,0 +1,60 @@ +using System.Diagnostics; + +namespace Compono.XunitV3.Tests; + +// The milestone's required "at least one test suite must prove behavior through the real xUnit v3 +// discovery and execution pipeline" coverage (ADR-0022's Testing Strategy) - every other test in this +// project calls GetData directly, which never exercises SupportsDiscoveryEnumeration()'s actual +// effect on discovery/execution sequencing, real theory-row rendering, or a real MTP process exit +// code. This shells out `dotnet test` against test/Compono.XunitV3.SampleTests - a genuinely separate +// project consuming Compono.XunitV3 as a packaged dependency (never a ProjectReference) - and asserts +// on that real process's captured output. +public sealed class RealRunnerTests +{ + [Fact] + public void DotnetTest_OnTheSampleProject_ReportsTheExpectedPassFailSplit_AndSurfacesTheFailingSeed() + { + var sampleProjectDirectory = FindSampleProjectDirectory(); + + var startInfo = new ProcessStartInfo("dotnet", "test -f net10.0") + { + WorkingDirectory = sampleProjectDirectory, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }; + + using var process = Process.Start(startInfo)!; + var output = process.StandardOutput.ReadToEnd(); + var error = process.StandardError.ReadToEnd(); + var exited = process.WaitForExit(TimeSpan.FromMinutes(5)); + + exited.Should().BeTrue("the sample project's own dotnet test run should complete well within this timeout"); + + var combinedOutput = output + error; + + // FailingCompositionTests.DeliberatelyFailingComposition_NegativeSeedIsRejected is the one + // deliberately-failing theory in the sample project - everything else there is shaped to pass. + combinedOutput.Should().Contain("total: 6"); + combinedOutput.Should().Contain("failed: 1"); + combinedOutput.Should().Contain("succeeded: 5"); + combinedOutput.Should().Contain("Seed: -1"); + process.ExitCode.Should().NotBe(0, "the sample project's own deliberately-failing theory makes its dotnet test process report failure"); + } + + // Walks up from this test assembly's own output directory to the repo root (identified by + // Compono.slnx, which only exists there) rather than a relative "../../.." path from bin// + // / - robust to which TFM/configuration this test itself happens to be running under. + private static string FindSampleProjectDirectory() + { + var directory = new DirectoryInfo(AppContext.BaseDirectory); + + while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "Compono.slnx"))) + directory = directory.Parent; + + if (directory is null) + throw new InvalidOperationException("Could not locate the repository root (Compono.slnx) above " + AppContext.BaseDirectory); + + return Path.Combine(directory.FullName, "test", "Compono.XunitV3.SampleTests"); + } +} diff --git a/test/Compono.XunitV3.Tests/SeedReportingTests.cs b/test/Compono.XunitV3.Tests/SeedReportingTests.cs new file mode 100644 index 0000000..e3db70f --- /dev/null +++ b/test/Compono.XunitV3.Tests/SeedReportingTests.cs @@ -0,0 +1,52 @@ +using System.Globalization; +using System.Text.RegularExpressions; +using Compono.XunitV3.Tests.Fixtures; +using Xunit.Sdk; + +namespace Compono.XunitV3.Tests; + +// Proves the pasteable-seed promise itself, not just a "Seed:" label's presence: a deliberately- +// failing composition propagates the pipeline's own CompositionException unwrapped (ComposeAttribute +// never wraps it), so the seed lives on exception.Diagnostic (the same "Console.WriteLine(exception +// .Diagnostic)" surface docs/architecture.md documents), not on exception.Message directly - unlike +// Compono.XunitV3's own pre-composition exceptions (negative seed, signature errors, inline +// validation), which append a "Seed: " line straight into Message via AppendSeed. +public sealed partial class SeedReportingTests +{ + [Fact] + public async Task GetData_FailingComposition_DiagnosticSeedFromAnAutoGeneratedRow_ReproducesTheSameFailure_WhenPastedBack() + { + var attribute = new ComposeAttribute(); + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var firstAct = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var firstException = (await firstAct.Should().ThrowAsync()).Which; + + var match = SeedLine().Match(firstException.Diagnostic!.ToString()); + match.Success.Should().BeTrue("Diagnostic.ToString() renders the 'Seed: ' line docs/architecture.md documents"); + var pastedSeed = int.Parse(match.Groups["seed"].Value, CultureInfo.InvariantCulture); + + var pastedAttribute = new ComposeAttribute { Seed = pastedSeed }; + var secondAct = () => pastedAttribute.GetData(method, new DisposalTracker()).AsTask(); + var secondException = (await secondAct.Should().ThrowAsync()).Which; + + secondException.Diagnostic!.Seed.Should().Be((ulong)pastedSeed); + } + + [Fact] + public async Task GetData_FailingComposition_DiagnosticSeed_MatchesTheExplicitRowSeed() + { + const int seed = 7654321; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync()).Which; + + exception.Diagnostic!.Seed.Should().Be((ulong)seed); + exception.Diagnostic.ToString().Should().Contain(seed.ToString(CultureInfo.InvariantCulture)); + } + + [GeneratedRegex(@"Seed: (?\d+)")] + private static partial Regex SeedLine(); +} From cb65c74d4e1a37e7fc8a87c8393104d2d4ab7568 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 09:00:13 -0400 Subject: [PATCH 02/10] fix(core): serialize local-feed packing to fix CI test race CI's PR build failed: dotnet test runs every TFM concurrently, so Compono.XunitV3.Tests' net10.0 and net11.0 hosts both ran RealRunnerTests at once, each independently triggering a nested dotnet pack of Compono/Compono.XunitV3 into the shared .local-nuget-feed - two unsynchronized pack sequences racing on the same output paths. Reproduced locally (concurrent dotnet test invocations against the sample project, and against Compono.XunitV3.Tests itself, matching CI's real concurrency) and fixed by moving the pack step into pack-to-local-feed.sh, which serializes it behind a portable mkdir-based cross-process lock. Verified the same reproduction scenarios pass cleanly post-fix. Co-Authored-By: Claude Sonnet 5 --- .../0004-milestone-4-xunit-integration.md | 27 +++++++++++++++++ .../Compono.XunitV3.SampleTests.csproj | 11 +++++-- .../pack-to-local-feed.sh | 29 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100755 test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index b3ffa59..a0b0ff1 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -805,6 +805,33 @@ exercised indirectly through `Compono.XunitV3.Tests`. `.local-nuget-feed/` on every restore, so it's self-healing on a clean checkout (including in CI, via `RealRunnerTests`' subprocess) without a separate manual pack step. +- **`PackToLocalFeed` needed a cross-process lock - the first CI run of this + PR failed with exactly the concurrency this note predicted was possible**: + `dotnet test --solution Compono.slnx` runs every test host for every TFM + concurrently, so `Compono.XunitV3.Tests`' net10.0 and net11.0 hosts each + ran `RealRunnerTests` at the same time, each independently triggering its + own nested `dotnet test` against `Compono.XunitV3.SampleTests`, each of + which independently triggered `PackToLocalFeed` - two unsynchronized + `dotnet pack` sequences racing on the identical shared + `.local-nuget-feed/` and `src/Compono`/`src/Compono.Generators` + bin/obj output. CI failed with `NuGet.Build.Tasks.Pack.targets(226,5): + error : Could not find a part of the path '.../Compono.Generators/bin + /Debug/netstandard2.0'`; reproduced locally (running two/three concurrent + `dotnet test` invocations against the sample project from a clean + checkout) as `The process cannot access the file '.../Compono.1.0.0 + .nupkg' because it is being used by another process` - same root cause, + different symptom depending on exactly which file each process touched + first. Fixed by moving the two `dotnet pack` `Exec` calls out of the + `.csproj` target and into a new `pack-to-local-feed.sh`, which wraps them + in a `mkdir`-based lock (atomic across processes and portable across the + bash both macOS dev machines and the Linux CI runner use, so no custom + MSBuild task was needed) - a losing process waits and retries rather than + racing. Verified by reproducing the exact failure locally pre-fix (two + and then three concurrent `dotnet test` invocations against the sample + project, and separately against `Compono.XunitV3.Tests` itself matching + CI's real net10.0+net11.0 concurrency, all from a fully clean checkout + with the local NuGet cache cleared), then confirming all of the same + scenarios pass cleanly post-fix. - Full suite green: `Compono.Tests` 388/388 (unchanged), `Compono.Generators .Tests` 166/166 (unchanged), `Compono.XunitV3.Tests` 92/92 (46 × 2 TFMs - 32 from Phase 2 + 8 nullable/inline-null tests + 1 non-null-into-`int?` diff --git a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj index 6dfd783..fe71056 100644 --- a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj +++ b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj @@ -32,11 +32,16 @@ Self-healing on a clean checkout and always tests today's Compono/Compono.XunitV3 source, which is the point of a "real packaged consumer" test: a regression in packaging (a missing dependency, wrong PrivateAssets, the generator analyzer not flowing transitively) fails this - project's own restore/build/run rather than being masked by a stale package. --> + project's own restore/build/run rather than being masked by a stale package. + + Delegates to pack-to-local-feed.sh, which serializes the two `dotnet pack` calls behind a + cross-process lock - CI (and RealRunnerTests) run this project's net10.0/net11.0 test hosts + concurrently, each independently triggering this target via its own nested `dotnet test`, and + two unsynchronized `dotnet pack` invocations racing on the same shared .local-nuget-feed/ and + src/Compono*/bin/obj output corrupts both (caught in CI - see PLAN-0004 Phase 3 Notes). --> - - + diff --git a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh new file mode 100755 index 0000000..54a26af --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh @@ -0,0 +1,29 @@ +#!/usr/bin/env bash +# Packs Compono and Compono.XunitV3 into the local NuGet feed this project restores against +# (test/Compono.XunitV3.SampleTests/nuget.config), serialized behind a cross-process lock. +# +# Why the lock: this script is invoked from a PackToLocalFeed MSBuild target +# (BeforeTargets="Restore") on every restore of this project. CI (and RealRunnerTests, which shells +# out `dotnet test` against this project) runs Compono.XunitV3.Tests for multiple TFMs concurrently, +# so two independent nested `dotnet test` processes can each trigger this script at the same moment, +# both packing the same src/Compono and src/Compono.Generators projects into the same +# .local-nuget-feed/ directory - a real race (reproduced locally: "The process cannot access the +# file '.../Compono.1.0.0.nupkg' because it is being used by another process", and in CI: "Could not +# find a part of the path '.../Compono.Generators/bin/Debug/netstandard2.0'"). `mkdir` is atomic even +# across processes and platforms, so it works as a portable mutex without a custom MSBuild task. +set -euo pipefail + +compono_csproj="$1" +xunitv3_csproj="$2" +feed_dir="$3" +configuration="$4" + +lock_dir="$feed_dir/.pack.lock" + +until mkdir "$lock_dir" 2>/dev/null; do + sleep 1 +done +trap 'rmdir "$lock_dir"' EXIT + +dotnet pack "$compono_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo +dotnet pack "$xunitv3_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo From c4f099dd74a31f8257fd5cd40769d1d2fbb2db82 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 09:42:36 -0400 Subject: [PATCH 03/10] fix(core): make composition failures pasteable-seed-correct and isolate the sample-project restore PR #26 review (Codex): address two inline findings. 1. FailingCompositionTests used [Compose(Seed = -1)] - a rejected-seed validation failure, not a genuine composition failure - so RealRunnerTests never actually proved the milestone's own promise that a composition failure's message carries a reproducible seed. Fixed by adding CompositionException.WithSeedInMessage as a new internal seam (Compono grants Compono.XunitV3 InternalsVisibleTo) so ComposeAttribute.GetData now rewrites a propagating pipeline failure's Message to include the seed - previously only Diagnostic had it, and a real test runner's failure display shows Message. The sample project's failing theory now composes a genuine unsatisfied dependency (GatewayConsumer -> IUnregisteredGateway) with an explicit seed. 2. PackToLocalFeed's fixed version let a stale global-cache entry silently satisfy a later restore. This took three iterations - two of which passed local testing, were pushed, and still failed in CI in two different ways (documented in full in PLAN-0004 Phase 3 Notes, since each failure mode only reproduced once the exact CI sequence and concurrency were replicated, not just individual pieces in isolation). Landed on: an isolated per-invocation RestorePackagesPath, scoped by an environment variable RealRunnerTests.cs sets before spawning its nested dotnet test, verified stable across MSBuild's separate restore-graph and build evaluation passes and safe under CI's real concurrent net10.0+net11.0 test execution (reproduced and fixed locally, four consecutive clean runs under the exact CI sequence). Also adds a [Compose] theory to the sample project (previously missing entirely - user request, not a posted PR comment), proving TProfile.Configure applies through the real packaged pipeline. Co-Authored-By: Claude Sonnet 5 --- docs/adr/0022-compono-xunit-package-design.md | 70 ++++++++++ .../0004-milestone-4-xunit-integration.md | 123 ++++++++++++++++-- src/Compono.XunitV3/ComposeAttribute.cs | 42 +++++- src/Compono/Compono.csproj | 5 + src/Compono/CompositionException.cs | 24 ++++ .../CompositionExceptionTests.cs | 35 +++++ .../Compono.XunitV3.SampleTests.csproj | 29 ++++- test/Compono.XunitV3.SampleTests/Domain.cs | 22 ++++ .../FailingCompositionTests.cs | 20 ++- .../ProfileTests.cs | 22 ++++ .../pack-to-local-feed.sh | 22 +++- test/Compono.XunitV3.Tests/RealRunnerTests.cs | 24 +++- .../SeedReportingTests.cs | 41 ++++-- 13 files changed, 440 insertions(+), 39 deletions(-) create mode 100644 test/Compono.XunitV3.SampleTests/ProfileTests.cs diff --git a/docs/adr/0022-compono-xunit-package-design.md b/docs/adr/0022-compono-xunit-package-design.md index 8de0f06..0227467 100644 --- a/docs/adr/0022-compono-xunit-package-design.md +++ b/docs/adr/0022-compono-xunit-package-design.md @@ -1038,6 +1038,76 @@ its own design question, deferred - not designed here, and not assumed necessary until a concrete need materializes (this milestone's own non-goals already exclude speculative future-proofing). +## Amendment 5 (2026-07-31): a genuine composition failure's own `Message` must carry the seed too, not only `Diagnostic` + +PR #26 review (Codex, on PLAN-0004 Phase 3) caught that `test/Compono +.XunitV3.SampleTests`' deliberately-failing theory used `[Compose(Seed = +-1)]` - a negative, rejected seed - which exercises `Compono.XunitV3`'s own +seed-validation failure (a pre-composition check that always rejects `-1` +regardless of what it's paired with), not a genuine composition failure. +That meant `RealRunnerTests` never actually proved the milestone's own +Goal statement - "a composition failure's message contains a seed that, +pasted into `[Compose(Seed = ...)]`, reproduces the same failure" - for +the case that statement is actually about: a real, unsatisfied composition +request. + +Investigating why a genuine case wasn't used originally (Phase 3's own +`SeedReportingTests`, added under time pressure to close out that task +list item) surfaced the real gap: a *pipeline*-propagated +`CompositionException` (an ordinary composition failure - nothing could +satisfy the requested type) has never carried its seed in +`Exception.Message` itself, only in `Diagnostic`/`Diagnostic.ToString()` +(the `Console.WriteLine(exception.Diagnostic)` convention +`docs/architecture.md` documents, from Milestone 2/3, per +[ADR-0010](0010-composition-request-pipeline-and-diagnostics-tracing.md)). +A real xUnit v3/MTP test-runner failure display shows `Exception.Message`, +not `Diagnostic.ToString()` - so the pasteable-seed promise was invisible +in real console output for a genuine composition failure the entire time, +not just in this one sample theory. + +**Decision:** `ComposeAttribute.GetData` now catches a `CompositionException` +propagating from a composition call (`ResolveInvoker`/`ResolveSharedInvoker`/ +`ShareExplicitInvoker`, via a new private `InvokeWithSeedOnFailure` helper +wrapping each call site) and rethrows it via a new internal `Compono` core +seam, `CompositionException.WithSeedInMessage(original, seed)` - rewriting +`Message` to `$"{diagnostic.Message}\n\nSeed: {seed}"` while preserving +`Diagnostic` completely unchanged (so `Diagnostic.ToString()` still renders +correctly, without a duplicated `"Seed:"` line) and the original exception +as `InnerException`. This is an `internal` seam, not a new public API - +`Compono.csproj` grants `Compono.XunitV3` `InternalsVisibleTo` for exactly +this, the same pattern already used for `Compono.Tests` - `docs/public +-api.md`'s "keep public APIs minimal" goal is unaffected. + +**Why an internal core seam rather than a workaround entirely inside +`Compono.XunitV3`:** `CompositionException`'s existing constructors always +derive `Message` directly from `Diagnostic.Message` (or accept a plain +string with no `Diagnostic`/`InnerException` at all) - there's no existing +public shape that lets a caller supply a custom `Message` string alongside +an existing `Diagnostic` and `InnerException` together. +`Compono.XunitV3` reconstructing its own `CompositionDiagnostic` copy with +a seed-appended `Message` field was considered and rejected: `Diagnostic +.ToString()`'s own format independently appends `"\n\nSeed: {Seed}"`, so a +diagnostic whose `Message` field *also* had the seed appended would render +that line twice - a visible regression for the documented `Console +.WriteLine(exception.Diagnostic)` path. The internal seam avoids that +entirely by leaving `Diagnostic` untouched and only rewriting the outer +exception's own `Message`. + +**Consequence for `test/Compono.XunitV3.SampleTests`:** `Failing +CompositionTests` now composes `GatewayConsumer` (a concrete class whose +constructor takes `IUnregisteredGateway`, an interface satisfied by +nothing), with an explicit `Seed = 24601` so `RealRunnerTests`' subprocess +assertion stays deterministic rather than needing to parse an +auto-generated seed out of console output. `IUnregisteredGateway` itself is +deliberately never used as the `[Compose]`-attributed parameter's own root +type - that would hit the CMP0003 diagnostic this plan's own Open Items +section already documents as deliberate (an abstract/interface/delegate +root always fails at compile time, regardless of whether a registration +might satisfy it at runtime) - wrapping it as a *nested* constructor +parameter uses the more lenient member-level check instead, so it compiles +and fails only at runtime, which is the failure shape this fixture +actually needs. + ## Links - [ADR-0021](0021-row-composition-entry-point-for-test-framework-integrations.md) — diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index a0b0ff1..966b1f0 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -832,14 +832,121 @@ exercised indirectly through `Compono.XunitV3.Tests`. CI's real net10.0+net11.0 concurrency, all from a fully clean checkout with the local NuGet cache cleared), then confirming all of the same scenarios pass cleanly post-fix. -- Full suite green: `Compono.Tests` 388/388 (unchanged), `Compono.Generators - .Tests` 166/166 (unchanged), `Compono.XunitV3.Tests` 92/92 (46 × 2 TFMs - - 32 from Phase 2 + 8 nullable/inline-null tests + 1 non-null-into-`int?` - regression test + 2 seed-reporting tests + 1 concurrency-stress test + 1 - API-surface approval test + 1 `RealRunnerTests` real-runner test), plus - `Compono.XunitV3.SampleTests` itself (run only via `RealRunnerTests`, not - the main solution/CI test matrix) reporting 5 passed/1 deliberately failed - on both net10.0 and net11.0. +- **PR #26 review (second round) caught two more real gaps, both fixed in + the same PR** - see ADR-0022 Amendment 5 for the first in full: + 1. `FailingCompositionTests` used `[Compose(Seed = -1)]`, which exercises + `Compono.XunitV3`'s own seed-validation failure, not a genuine + composition failure - the milestone's actual Goal statement is about + the latter. Fixed by (a) adding `CompositionException + .WithSeedInMessage` as a new internal `Compono` core seam (`Compono + .XunitV3` granted `InternalsVisibleTo`) so `ComposeAttribute.GetData` + now rewrites a propagating pipeline failure's own `Message` to include + the seed - previously only `Diagnostic` had it, and a real test + runner's failure display shows `Message`, not `Diagnostic.ToString()`; + (b) switching the sample project's failing theory to a genuine + unsatisfied composition (`GatewayConsumer`, whose constructor takes + `IUnregisteredGateway` - deliberately a *nested* dependency, not the + `[Compose]`-attributed parameter's own root type, since an abstract + root there hits the CMP0003 diagnostic this plan's Open Items section + already documents as deliberate) with an explicit `Seed = 24601` so + `RealRunnerTests`' assertion stays deterministic. `SeedReportingTests` + was rewritten to assert on `.Message` directly (not `.Diagnostic`) for + exactly this reason, plus a new test proving `Diagnostic` itself is + left unchanged (no duplicated `"Seed:"` line) alongside the rewritten + `Message`. `Compono.Tests` gained direct coverage of the new internal + `CompositionException.WithSeedInMessage` seam itself, per `testing.md`'s + "verifying a new public entry point" rule extended to this internal + one (an internal seam a different assembly depends on deserves the + same isolated-coverage discipline a public one would). + 2. `PackToLocalFeed`'s fixed `-p:Version=1.0.0` meant a stale entry in + NuGet's global packages folder from an earlier run could silently + satisfy a later restore even after the local feed's `.nupkg` contents + changed - exactly what the earlier note above already described as a + manual "remember to clear the cache" gotcha, left unfixed. This took + **three** attempts, two of which shipped, passed local verification, + were pushed, and still failed in CI - worth recording in full since + the failure mode each time was genuinely different from what local + testing had exercised: + - **First attempt**: `pack-to-local-feed.sh` deleted + `~/.nuget/packages/compono`/`~/.nuget/packages/compono.xunitv3` + (respecting `$NUGET_PACKAGES`) at the start of its locked section, + before every pack. Passed every local check (including a from-clean + run proving the cached DLL's hash changed after a source edit) and + was pushed - CI failed immediately with `Could not find a part of + the path '.../packages/compono/1.0.0'`. The bug: the cross-process + lock only wraps the two `dotnet pack` calls, not the *subsequent* + restore of `Compono.XunitV3.SampleTests.csproj` itself (which is + what actually extracts files into that folder) - so a second + process's delete, running after the first process released the + lock, could remove files the first process's own restore was still + reading. Reproduced locally once the exact CI sequence was + replicated (Release restore+build first, *then* concurrent + net10.0+net11.0 `RealRunnerTests`, rather than testing each + ingredient - clean-state concurrency, and a solo pack after a + Release build - separately, which is why it passed locally the + first time). + - **Second attempt**: replaced the fixed version with a per-evaluation + `$([System.Guid]::NewGuid())` MSBuild property plus `VersionOverride` + on the `PackageReference`, reasoning that a version which never + existed before can't be stale and two random versions can't collide. + Failed at the very first local test (never reached CI) with `NU1102`: + the pack step and the package reference's own version resolution + read two *different* GUIDs for the same nominal build, because + NuGet's restore-graph evaluation pass and the actual build + evaluation pass each re-evaluate a property function like + `NewGuid()` independently - there is no single moment where "this + evaluation" is computed once and reused; a live function call in a + property is recomputed on every separate MSBuild evaluation pass. + - **Third (shipped) attempt**: isolate the restore from the shared + global cache entirely, via a per-project `RestorePackagesPath` + (`obj/.nuget-packages//`) that's cleared unconditionally before + every pack - safe because nothing outside this one isolated path + reads from it, so clearing it can never race a concurrent sibling. + The `` needed its own iteration too: scoping it by + `$(TargetFramework)` seemed like the obvious stable, external, + non-random value - but failed locally under the exact CI-matching + concurrency scenario, because `RealRunnerTests.cs` hardcodes + `"test -f net10.0"` for its nested command *regardless of which + TFM the calling test host itself is* - both concurrent invocations + (from the net10.0 and net11.0 hosts) resolved `$(TargetFramework)` + to the identical `"net10.0"`, isolating nothing. Fixed by having + `RealRunnerTests.cs` set a `Compono_LocalPackagesId` environment + variable to a fresh `Guid.NewGuid()` before each nested + `Process.Start` - an environment variable is set once in the C# + process and inherited stably by that whole child process tree, so + every internal MSBuild re-evaluation the nested `dotnet test` + performs reads the identical value, while a second, + independently-spawned nested process gets its own distinct one + (the same stability problem that broke the GUID-as-property attempt, + solved by moving the "compute once" step outside MSBuild entirely). + Verified by reproducing the *exact* CI sequence four times in a row + (`dotnet restore`/`dotnet build --configuration release + --no-restore` first, matching the shared workflow precisely, then + concurrent net10.0+net11.0 `dotnet test ... --no-restore`) with a + clean local NuGet cache each time - all four passed after this fix, + versus a reliable failure before it under the identical sequence. + - Also folded into this round, at the user's request (not a posted PR + comment): the sample project had no `[Compose]` theory at + all - every existing theory used the profile-less `[Compose]` form. + Added `ProfileTests.ComposesTheProfileConfiguredValue`, using a new + `ApplicationTestProfile` that registers a fixed `NotificationSettings` + value, proving `TProfile.Configure` actually applies through the real + packaged pipeline (not just the in-process `GetComposer()`/`CreateRow` + checks `Compono.XunitV3.Tests` already had). This is why the sample + project's theory count is 7, not 6, and `RealRunnerTests`' assertions + read `total: 7`/`succeeded: 6`/`failed: 1`. +- Full suite green: `Compono.Tests` 392/392 (196 × 2 TFMs - 384 unaffected + + 2 new `CompositionException.WithSeedInMessage` tests, each × 2 TFMs), + `Compono.Generators.Tests` 166/166 (unchanged), `Compono.XunitV3.Tests` + 94/94 (47 × 2 TFMs - 46 from the first PR #26 round + 1 `Diagnostic` + -preserved-unchanged test added alongside the `SeedReportingTests` + rewrite), plus `Compono.XunitV3.SampleTests` itself (run only via + `RealRunnerTests`, not the main solution/CI test matrix) reporting + 6 passed/1 deliberately failed - a genuine composition failure this + time - on both net10.0 and net11.0. Concurrency re-verified after every + change in this round (two and three concurrent `dotnet test` invocations + against the sample project, and the real net10.0+net11.0 + `Compono.XunitV3.Tests` concurrency CI actually exercises) - still clean. ## Open Items diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 18347cf..1527113 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -120,8 +120,10 @@ public int Seed /// parameter, more than one Compose-family attribute, or more than one [Shared] parameter /// of the same type); too many inline values were supplied; a supplied inline value is /// for a non-nullable parameter or has a type not assignable to its - /// parameter; or composition itself fails for a parameter (propagated un-wrapped from the - /// pipeline). + /// parameter; or composition itself fails for a parameter - the pipeline's own + /// propagates, with its + /// rewritten to also carry the row's seed ( and + /// are otherwise unchanged from what the pipeline threw). /// /// /// is deliberately never used to register a composed value. @@ -224,12 +226,12 @@ public override ValueTask> GetData(MethodInf if (i < _inlineValues.Length) { var value = _inlineValues[i]; - parameter.ShareExplicitInvoker(row, parameter.Descriptor, value); + InvokeWithSeedOnFailure(() => parameter.ShareExplicitInvoker(row, parameter.Descriptor, value), row.Seed); values[i] = value; } else { - values[i] = parameter.ResolveSharedInvoker(row, parameter.Descriptor); + values[i] = InvokeWithSeedOnFailure(() => parameter.ResolveSharedInvoker(row, parameter.Descriptor), row.Seed); } } @@ -243,7 +245,7 @@ public override ValueTask> GetData(MethodInf values[i] = i < _inlineValues.Length ? _inlineValues[i] - : parameter.ResolveInvoker(row, parameter.Descriptor); + : InvokeWithSeedOnFailure(() => parameter.ResolveInvoker(row, parameter.Descriptor), row.Seed); } // Assembled in method declaration order - binding order (shared first, then the rest) and @@ -291,4 +293,34 @@ private Composer BuildComposer() => Composer.Create(builder => // (ADR-0022's Seed Policy and Reporting) - so every failure category ends the same way, whether // Compono.XunitV3 constructed the message or the pipeline did. private static string AppendSeed(string message, int seed) => $"{message}\n\nSeed: {seed}"; + + // A genuine composition failure (PR #26 review; ADR-0022 Amendment 5) propagates un-wrapped from + // the pipeline otherwise, and CompositionException.Message alone never carries the seed for that + // case (only exception.Diagnostic does, via its own ToString()) - a real test runner's failure + // display shows Message, not Diagnostic.ToString(), so the pasteable-seed promise wasn't actually + // visible there. CompositionException.WithSeedInMessage rewrites Message to include it while + // preserving Diagnostic and the original exception as InnerException unchanged. + private static TResult InvokeWithSeedOnFailure(Func compose, int seed) + { + try + { + return compose(); + } + catch (CompositionException exception) when (exception.Diagnostic is not null) + { + throw CompositionException.WithSeedInMessage(exception, seed); + } + } + + private static void InvokeWithSeedOnFailure(Action compose, int seed) + { + try + { + compose(); + } + catch (CompositionException exception) when (exception.Diagnostic is not null) + { + throw CompositionException.WithSeedInMessage(exception, seed); + } + } } diff --git a/src/Compono/Compono.csproj b/src/Compono/Compono.csproj index 582667a..89e94e5 100644 --- a/src/Compono/Compono.csproj +++ b/src/Compono/Compono.csproj @@ -35,6 +35,11 @@ already how GeneratorTestHelpers.CompileAndExecute invokes every test's entry point, so this adds no new public/friend surface to the shipped package. --> + + diff --git a/src/Compono/CompositionException.cs b/src/Compono/CompositionException.cs index cd70f18..32cd594 100644 --- a/src/Compono/CompositionException.cs +++ b/src/Compono/CompositionException.cs @@ -97,4 +97,28 @@ internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnost internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, Exception innerException, object diagnosingContextIdentity) => new(diagnostic, innerException) { DiagnosingContextIdentity = diagnosingContextIdentity }; + + // Internal seam for Compono.XunitV3 (PR #26 review; ADR-0022 Amendment 5) - GetData needs a + // pipeline-propagated composition failure's own Message to carry the row's seed too, not only + // Diagnostic (which already renders a "Seed: " line via its own ToString()). A real xUnit + // v3/MTP test-runner failure display shows Exception.Message, not Diagnostic.ToString(), so the + // seed was invisible there for a genuine composition failure without this. Diagnostic itself is + // preserved unchanged (so Diagnostic.ToString() still renders correctly, without a duplicated + // "Seed:" line) and set as InnerException, never discarded. + internal static CompositionException WithSeedInMessage(CompositionException original, int seed) + { + ArgumentNullException.ThrowIfNull(original); + + if (original.Diagnostic is not { } diagnostic) + throw new ArgumentException("The exception being wrapped must have a Diagnostic.", nameof(original)); + + var message = $"{diagnostic.Message}\n\nSeed: {seed}"; + return new CompositionException(message, diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; + } + + private CompositionException(string message, CompositionDiagnostic diagnostic, Exception innerException) + : base(message, innerException) + { + Diagnostic = diagnostic; + } } diff --git a/test/Compono.Tests/CompositionExceptionTests.cs b/test/Compono.Tests/CompositionExceptionTests.cs index 90db5f2..f6abf2f 100644 --- a/test/Compono.Tests/CompositionExceptionTests.cs +++ b/test/Compono.Tests/CompositionExceptionTests.cs @@ -16,6 +16,41 @@ public void Constructor_ThrowsArgumentNullException_WhenDiagnosticIsNull() act.Should().Throw(); } + [Fact] + public void WithSeedInMessage_RewritesMessage_ButPreservesDiagnosticAndSetsInnerException() + { + // Internal seam for Compono.XunitV3 (PR #26 review; ADR-0022 Amendment 5) - a pipeline-thrown + // CompositionException's own Message never carried the seed on its own (only Diagnostic did, + // via its own ToString()), so Compono.XunitV3's GetData rewrites it before propagating. + var diagnostic = new CompositionDiagnostic + { + RootType = typeof(CompositionExceptionTests), + FailedType = typeof(CompositionExceptionTests), + Path = "unrelated-path", + Trace = [], + Seed = 0, + Message = "original pipeline failure message", + }; + var original = CompositionException.CreatePipelineDiagnosed(diagnostic, diagnosingContextIdentity: new object()); + + var wrapped = CompositionException.WithSeedInMessage(original, seed: 492173); + + wrapped.Message.Should().Be("original pipeline failure message\n\nSeed: 492173"); + wrapped.Diagnostic.Should().BeSameAs(diagnostic); + wrapped.Diagnostic!.Message.Should().Be("original pipeline failure message", "Diagnostic's own Message is left untouched - only the outer exception's Message is rewritten"); + wrapped.InnerException.Should().BeSameAs(original); + } + + [Fact] + public void WithSeedInMessage_Throws_WhenTheOriginalExceptionHasNoDiagnostic() + { + var original = new CompositionException("a plain, non-pipeline-diagnosed message"); + + var act = () => CompositionException.WithSeedInMessage(original, seed: 1); + + act.Should().Throw(); + } + [Fact] public void DoesNotDeclareAnyMemberOfTypeCompositionContext() { diff --git a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj index fe71056..c1471a0 100644 --- a/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj +++ b/test/Compono.XunitV3.SampleTests/Compono.XunitV3.SampleTests.csproj @@ -17,6 +17,33 @@ being masked by a ProjectReference build. See PLAN-0004 Phase 3 and ADR-0022's Testing Strategy. --> $(MSBuildThisFileDirectory)../../.local-nuget-feed + + manual + $(MSBuildThisFileDirectory)obj/.nuget-packages/$(Compono_LocalPackagesId)/ @@ -41,7 +68,7 @@ src/Compono*/bin/obj output corrupts both (caught in CI - see PLAN-0004 Phase 3 Notes). --> - + diff --git a/test/Compono.XunitV3.SampleTests/Domain.cs b/test/Compono.XunitV3.SampleTests/Domain.cs index 1cf4294..bae4df1 100644 --- a/test/Compono.XunitV3.SampleTests/Domain.cs +++ b/test/Compono.XunitV3.SampleTests/Domain.cs @@ -18,3 +18,25 @@ public OrderService(Repository repository) } public sealed record CreateOrder(string CustomerName, int Quantity); + +// No provider, registration, [Composable], or generated plan can ever satisfy this - reserved for +// FailingCompositionTests, which needs a genuine (pipeline-propagated) composition failure rather +// than one of Compono.XunitV3's own pre-composition validation failures (PR #26 review). +// +// Deliberately never used as a [Compose]-attributed parameter's own type directly - an +// interface/abstract/delegate root always reports CMP0003 at compile time (a documented, deliberate +// PLAN-0004 Open Item: ComposedTypeAnalyzer's root-level check is stricter than its nested-member +// check), so an abstract root here would be a compile error, not the runtime composition failure this +// fixture needs. GatewayConsumer below wraps it as a nested constructor parameter instead, which uses +// the more lenient member-level check and is left as a runtime Resolve call. +public interface IUnregisteredGateway; + +public sealed class GatewayConsumer +{ + public GatewayConsumer(IUnregisteredGateway gateway) + { + Gateway = gateway; + } + + public IUnregisteredGateway Gateway { get; } +} diff --git a/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs index 9b12c82..3b3edca 100644 --- a/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs +++ b/test/Compono.XunitV3.SampleTests/FailingCompositionTests.cs @@ -1,15 +1,21 @@ namespace Compono.XunitV3.SampleTests; -// Deliberately fails, on every run - Compono.XunitV3.Tests' RealRunnerTests shells out `dotnet test` -// against this project and asserts the captured output contains "Seed:", proving the milestone's -// "a composition failure's message contains a seed" promise reaches a real xUnit v3 runner's actual -// output, not just an in-process GetData call. +// Deliberately fails, on every run, via a genuine (pipeline-propagated) composition failure - not one +// of Compono.XunitV3's own pre-composition validation failures (a negative seed, a signature error, an +// inline-value mismatch), which don't exercise real composition at all (PR #26 review). Uses an +// explicit seed so Compono.XunitV3.Tests' RealRunnerTests can assert on a deterministic value: it +// shells out `dotnet test` against this project and asserts the captured output contains this exact +// seed, proving the milestone's "a composition failure's message contains a seed that reproduces the +// same failure" promise reaches a real xUnit v3 runner's actual output, not just an in-process GetData +// call. public sealed class FailingCompositionTests { + public const int Seed = 24601; + [Theory] - [Compose(Seed = -1)] - public void DeliberatelyFailingComposition_NegativeSeedIsRejected(int value) + [Compose(Seed = Seed)] + public void DeliberatelyFailingComposition_NoProviderCanSatisfyTheNestedInterfaceDependency(GatewayConsumer consumer) { - value.Should().Be(0, "GetData throws before this body ever runs - this line never executes"); + consumer.Should().BeNull("GetData throws before this body ever runs - this line never executes"); } } diff --git a/test/Compono.XunitV3.SampleTests/ProfileTests.cs b/test/Compono.XunitV3.SampleTests/ProfileTests.cs new file mode 100644 index 0000000..729323d --- /dev/null +++ b/test/Compono.XunitV3.SampleTests/ProfileTests.cs @@ -0,0 +1,22 @@ +namespace Compono.XunitV3.SampleTests; + +public sealed record NotificationSettings(string SenderAddress); + +// Reached only through ProfileTests.ComposesTheProfileConfiguredValue's own [Compose] +// theory parameter - proves ComposeAttribute actually applies TProfile.Configure through the +// real packaged pipeline, not just Compono.XunitV3.Tests' in-process GetComposer()/CreateRow checks. +public sealed class ApplicationTestProfile : ICompositionProfile +{ + public void Configure(CompositionBuilder builder) => + builder.Register(() => new NotificationSettings("sample-test-sender@example.com")); +} + +public sealed class ProfileTests +{ + [Theory] + [Compose] + public void ComposesTheProfileConfiguredValue(NotificationSettings settings) + { + settings.SenderAddress.Should().Be("sample-test-sender@example.com"); + } +} diff --git a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh index 54a26af..fc545c1 100755 --- a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh +++ b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash # Packs Compono and Compono.XunitV3 into the local NuGet feed this project restores against -# (test/Compono.XunitV3.SampleTests/nuget.config), serialized behind a cross-process lock. +# (test/Compono.XunitV3.SampleTests/nuget.config), serialized behind a cross-process lock, and clears +# this restore's own isolated packages path before every pack. # # Why the lock: this script is invoked from a PackToLocalFeed MSBuild target # (BeforeTargets="Restore") on every restore of this project. CI (and RealRunnerTests, which shells @@ -11,12 +12,29 @@ # file '.../Compono.1.0.0.nupkg' because it is being used by another process", and in CI: "Could not # find a part of the path '.../Compono.Generators/bin/Debug/netstandard2.0'"). `mkdir` is atomic even # across processes and platforms, so it works as a portable mutex without a custom MSBuild task. +# +# Why $restore_packages_path is cleared here (PR #26 review, second round): NuGet treats a package +# id+version already present in a packages folder as immutable and never re-extracts it, so this +# script repacking different content under the same fixed "1.0.0" version would otherwise let a stale +# entry silently satisfy a later restore. Two earlier fixes were tried and reverted first (see +# PLAN-0004 Phase 3 Notes for the full account): clearing NuGet's *shared global* packages folder here +# raced against a sibling process's own restore still reading from it after this script's lock +# released ("Could not find a part of the path '.../packages/compono/1.0.0'", reproduced locally and +# caught in CI); a per-evaluation-random package version (via MSBuild's $([System.Guid]::NewGuid())) +# turned out unstable, because NuGet's restore-graph evaluation pass and the actual build evaluation +# pass each re-evaluate that property function independently, producing two different GUIDs for the +# same nominal build (a real NU1102 version mismatch, reproduced locally). $restore_packages_path +# (scoped per $(TargetFramework) by the caller) has neither problem: it's private to this one +# project+TFM's own restore, so clearing it can never affect a concurrent sibling process running a +# different TFM, and there is nothing left to make "the same value across evaluation passes" in the +# first place - it's a plain path, not a value that needs to agree with anything else. set -euo pipefail compono_csproj="$1" xunitv3_csproj="$2" feed_dir="$3" configuration="$4" +restore_packages_path="$5" lock_dir="$feed_dir/.pack.lock" @@ -25,5 +43,7 @@ until mkdir "$lock_dir" 2>/dev/null; do done trap 'rmdir "$lock_dir"' EXIT +rm -rf "$restore_packages_path" + dotnet pack "$compono_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo dotnet pack "$xunitv3_csproj" -c "$configuration" -o "$feed_dir" -p:Version=1.0.0 --nologo diff --git a/test/Compono.XunitV3.Tests/RealRunnerTests.cs b/test/Compono.XunitV3.Tests/RealRunnerTests.cs index 428eca3..ab10655 100644 --- a/test/Compono.XunitV3.Tests/RealRunnerTests.cs +++ b/test/Compono.XunitV3.Tests/RealRunnerTests.cs @@ -24,6 +24,16 @@ public void DotnetTest_OnTheSampleProject_ReportsTheExpectedPassFailSplit_AndSur UseShellExecute = false, }; + // Always "-f net10.0" above regardless of whether *this* test is itself running under the + // net10.0 or net11.0 host - CI runs both concurrently, so two RealRunnerTests instances can + // each spawn this exact nested command at the same moment (PR #26 review, second/third + // rounds). The sample project's own RestorePackagesPath isolates each restore by this + // environment variable rather than $(TargetFramework) - the latter is the same "net10.0" for + // both processes here, so it never actually distinguished them, unlike a value computed once + // in this C# process and inherited stably by the whole child process tree (including every + // internal MSBuild restore/build re-evaluation the nested `dotnet test` performs). + startInfo.Environment["Compono_LocalPackagesId"] = Guid.NewGuid().ToString("N"); + using var process = Process.Start(startInfo)!; var output = process.StandardOutput.ReadToEnd(); var error = process.StandardError.ReadToEnd(); @@ -33,12 +43,16 @@ public void DotnetTest_OnTheSampleProject_ReportsTheExpectedPassFailSplit_AndSur var combinedOutput = output + error; - // FailingCompositionTests.DeliberatelyFailingComposition_NegativeSeedIsRejected is the one - // deliberately-failing theory in the sample project - everything else there is shaped to pass. - combinedOutput.Should().Contain("total: 6"); + // FailingCompositionTests.DeliberatelyFailingComposition_NoProviderCanSatisfyTheNestedInterfaceDependency + // is the one deliberately-failing theory in the sample project (a genuine, pipeline-propagated + // composition failure, not one of Compono.XunitV3's own pre-composition validation failures) - + // everything else there is shaped to pass. Its explicit Seed = 24601 is what makes this + // assertion deterministic rather than needing to parse an auto-generated seed out of the + // subprocess's own console output. + combinedOutput.Should().Contain("total: 7"); combinedOutput.Should().Contain("failed: 1"); - combinedOutput.Should().Contain("succeeded: 5"); - combinedOutput.Should().Contain("Seed: -1"); + combinedOutput.Should().Contain("succeeded: 6"); + combinedOutput.Should().Contain("Seed: 24601"); process.ExitCode.Should().NotBe(0, "the sample project's own deliberately-failing theory makes its dotnet test process report failure"); } diff --git a/test/Compono.XunitV3.Tests/SeedReportingTests.cs b/test/Compono.XunitV3.Tests/SeedReportingTests.cs index e3db70f..6227d44 100644 --- a/test/Compono.XunitV3.Tests/SeedReportingTests.cs +++ b/test/Compono.XunitV3.Tests/SeedReportingTests.cs @@ -5,16 +5,19 @@ namespace Compono.XunitV3.Tests; -// Proves the pasteable-seed promise itself, not just a "Seed:" label's presence: a deliberately- -// failing composition propagates the pipeline's own CompositionException unwrapped (ComposeAttribute -// never wraps it), so the seed lives on exception.Diagnostic (the same "Console.WriteLine(exception -// .Diagnostic)" surface docs/architecture.md documents), not on exception.Message directly - unlike -// Compono.XunitV3's own pre-composition exceptions (negative seed, signature errors, inline -// validation), which append a "Seed: " line straight into Message via AppendSeed. +// Proves the pasteable-seed promise itself, not just a "Seed:" label's presence, for a genuine +// (pipeline-propagated) composition failure - as opposed to one of Compono.XunitV3's own +// pre-composition exceptions (negative seed, signature errors, inline validation), which already +// append "Seed: " straight into Message via AppendSeed. A pipeline-thrown CompositionException's +// own Message never carried the seed on its own (only exception.Diagnostic did, via its own +// ToString()) - a real test runner's failure display shows Message, not Diagnostic.ToString(), so +// GetData now rewrites it via CompositionException.WithSeedInMessage (PR #26 review; ADR-0022 +// Amendment 5) before it propagates. Diagnostic itself (and its own correct Seed/ToString() rendering) +// is untouched by this - these tests check both. public sealed partial class SeedReportingTests { [Fact] - public async Task GetData_FailingComposition_DiagnosticSeedFromAnAutoGeneratedRow_ReproducesTheSameFailure_WhenPastedBack() + public async Task GetData_FailingComposition_MessageContainsTheAutoGeneratedRowSeed_AndReproducesTheSameFailure_WhenPastedBack() { var attribute = new ComposeAttribute(); var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; @@ -22,19 +25,19 @@ public async Task GetData_FailingComposition_DiagnosticSeedFromAnAutoGeneratedRo var firstAct = () => attribute.GetData(method, new DisposalTracker()).AsTask(); var firstException = (await firstAct.Should().ThrowAsync()).Which; - var match = SeedLine().Match(firstException.Diagnostic!.ToString()); - match.Success.Should().BeTrue("Diagnostic.ToString() renders the 'Seed: ' line docs/architecture.md documents"); + var match = SeedLine().Match(firstException.Message); + match.Success.Should().BeTrue("GetData rewrites a propagating composition failure's Message to include a 'Seed: ' line"); var pastedSeed = int.Parse(match.Groups["seed"].Value, CultureInfo.InvariantCulture); var pastedAttribute = new ComposeAttribute { Seed = pastedSeed }; var secondAct = () => pastedAttribute.GetData(method, new DisposalTracker()).AsTask(); var secondException = (await secondAct.Should().ThrowAsync()).Which; - secondException.Diagnostic!.Seed.Should().Be((ulong)pastedSeed); + secondException.Message.Should().Contain($"Seed: {pastedSeed}"); } [Fact] - public async Task GetData_FailingComposition_DiagnosticSeed_MatchesTheExplicitRowSeed() + public async Task GetData_FailingComposition_MessageContainsTheExplicitRowSeed() { const int seed = 7654321; var attribute = new ComposeAttribute { Seed = seed }; @@ -43,8 +46,22 @@ public async Task GetData_FailingComposition_DiagnosticSeed_MatchesTheExplicitRo var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); var exception = (await act.Should().ThrowAsync()).Which; + exception.Message.Should().Contain(seed.ToString(CultureInfo.InvariantCulture)); + } + + [Fact] + public async Task GetData_FailingComposition_DiagnosticIsPreservedUnchanged_AlongsideTheRewrittenMessage() + { + const int seed = 987654; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithUnregisteredInterfaceParameter))!; + + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync()).Which; + exception.Diagnostic!.Seed.Should().Be((ulong)seed); - exception.Diagnostic.ToString().Should().Contain(seed.ToString(CultureInfo.InvariantCulture)); + exception.Diagnostic.Message.Should().NotContain("Seed:", "the wrapped Message carries the seed - Diagnostic's own Message field is left as the pipeline's original plain text"); + exception.InnerException.Should().NotBeNull("the original pipeline exception is preserved, never discarded"); } [GeneratedRegex(@"Seed: (?\d+)")] From c2eb61120b5403f591fc52ec49f6dfa24a2ca6b8 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 09:54:23 -0400 Subject: [PATCH 04/10] fix(core): serialize the entire nested sample-test run with a named Mutex The restore-path isolation fix (previous commit) worked - CI's own log confirmed the two concurrent invocations resolved genuinely different Compono_LocalPackagesId values - but CI still hit the original "Could not find a part of the path '.../Compono.Generators/bin/Debug /netstandard2.0'" race, meaning pack-to-local-feed.sh's own mkdir lock (which only serializes the two dotnet pack Exec calls) wasn't sufficient on CI's runner, despite a dozen-plus local concurrent stress-test attempts under the exact CI sequence never reproducing it. Rather than keep chasing an MSBuild-internal timing difference that resisted local reproduction, RealRunnerTests.cs now wraps the entire nested `dotnet test` subprocess (spawn through exit) in a machine-wide named Mutex, fully serializing every nested invocation regardless of what either side does internally. This surfaced its own bug during local stress testing - Mutex.WaitOne() throwing AbandonedMutexException (from an earlier interrupted local run) was treated as a failure instead of the successful acquisition it represents - fixed by catching it, the standard .NET pattern. Verified with eight consecutive concurrent net10.0+net11.0 runs under the exact CI sequence, all clean after both fixes; the passing runs' own timings (~8s vs ~15-16s) directly show serialization happening, unlike previous attempts where both sides finished in a similar ~8-9s with no clear proof anything was actually being serialized. Co-Authored-By: Claude Sonnet 5 --- .../0004-milestone-4-xunit-integration.md | 32 ++++++++++++++ test/Compono.XunitV3.Tests/RealRunnerTests.cs | 42 +++++++++++++++++-- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index 966b1f0..dc7cfa0 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -925,6 +925,38 @@ exercised indirectly through `Compono.XunitV3.Tests`. concurrent net10.0+net11.0 `dotnet test ... --no-restore`) with a clean local NuGet cache each time - all four passed after this fix, versus a reliable failure before it under the identical sequence. + - **This "shipped" attempt was pushed and still failed in CI** - a + fourth round was needed. The restore-path isolation itself worked + (CI's own log showed the two invocations resolving genuinely + different `Compono_LocalPackagesId` values), but CI still hit the + *original* symptom from the very first race (`Could not find a + part of the path '.../Compono.Generators/bin/Debug/netstandard2.0'`) + - meaning `pack-to-local-feed.sh`'s own `mkdir`-based lock, which + serializes only the two `dotnet pack` `Exec` calls, wasn't + sufficient on CI's specific runner even though four separate local + stress-test batches (a dozen-plus concurrent attempts total, exactly + matching CI's Release-build-then-concurrent-test sequence) never + reproduced this exact recurrence locally. Rather than continue + chasing an MSBuild-internal timing difference that resisted local + reproduction, `RealRunnerTests.cs` now wraps the *entire* nested + subprocess (spawn through exit) in a machine-wide named + `System.Threading.Mutex` (`Global\Compono.XunitV3.SampleTests + .RealRunner`) - fully serializing every nested `dotnet test` + against the sample project, regardless of what either side is doing + internally, rather than relying on the pack step's own narrower + lock. This surfaced its own, unrelated bug during local stress + testing: `Mutex.WaitOne()` throwing `AbandonedMutexException` (a + previous interrupted local run had left the mutex abandoned without + releasing it) was treated as an unhandled failure instead of the + successful acquisition it actually represents - the standard .NET + pattern is to catch it and proceed, which `RealRunnerTests.cs` now + does. Verified with eight consecutive concurrent net10.0+net11.0 + runs (four before the `AbandonedMutexException` fix, all failing on + that new exception; four after, all clean) - and the passing runs' + own timings (one side consistently ~8s, the other ~15-16s) directly + show the serialization actually happening, unlike the previous + "both sides run in ~8-9s" timing that never definitively proved the + mkdir lock was serializing anything. - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose]` theory at all - every existing theory used the profile-less `[Compose]` form. diff --git a/test/Compono.XunitV3.Tests/RealRunnerTests.cs b/test/Compono.XunitV3.Tests/RealRunnerTests.cs index ab10655..2fa7577 100644 --- a/test/Compono.XunitV3.Tests/RealRunnerTests.cs +++ b/test/Compono.XunitV3.Tests/RealRunnerTests.cs @@ -34,10 +34,46 @@ public void DotnetTest_OnTheSampleProject_ReportsTheExpectedPassFailSplit_AndSur // internal MSBuild restore/build re-evaluation the nested `dotnet test` performs). startInfo.Environment["Compono_LocalPackagesId"] = Guid.NewGuid().ToString("N"); + // A machine-wide named Mutex, not just pack-to-local-feed.sh's own mkdir-based lock (PR #26 + // review, fourth round): the restore-path isolation above stops two concurrent invocations + // from colliding on NuGet's packages folder, but src/Compono and src/Compono.Generators + // themselves still build to one shared bin/obj regardless of which invocation is doing the + // building - the mkdir lock serializes the two `dotnet pack` Exec calls specifically, but CI + // still hit "Could not find a part of the path '.../Compono.Generators/bin/Debug + // /netstandard2.0'" even with that lock in place (reproducible in CI, not locally - some + // MSBuild-internal timing difference under CI's specific concurrency that direct local + // reproduction attempts didn't trigger). Rather than keep chasing that exact internal race, + // this Mutex serializes the *entire* nested subprocess (spawn through exit), machine-wide, so + // only one nested `dotnet test` against this sample project ever runs at a time, period - + // independent of whatever MSBuild is doing internally on either side of it. + using var mutex = new Mutex(initiallyOwned: false, name: "Global\\Compono.XunitV3.SampleTests.RealRunner"); + + try + { + mutex.WaitOne(); + } + catch (AbandonedMutexException) + { + // A previous holder terminated (e.g. a killed test run) without calling ReleaseMutex - + // the wait still succeeds and this thread now owns the mutex; the previous holder's own + // work is unrelated to whether this acquisition is valid, so there's nothing to rethrow. + } + using var process = Process.Start(startInfo)!; - var output = process.StandardOutput.ReadToEnd(); - var error = process.StandardError.ReadToEnd(); - var exited = process.WaitForExit(TimeSpan.FromMinutes(5)); + string output; + string error; + bool exited; + + try + { + output = process.StandardOutput.ReadToEnd(); + error = process.StandardError.ReadToEnd(); + exited = process.WaitForExit(TimeSpan.FromMinutes(5)); + } + finally + { + mutex.ReleaseMutex(); + } exited.Should().BeTrue("the sample project's own dotnet test run should complete well within this timeout"); From 2fca20b3096456277fd577b8ec01fe62cafbc974 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 10:02:26 -0400 Subject: [PATCH 05/10] fix(core): drop automated real-runner test after repeated CI-only concurrency failures Four consecutive rounds of fixes (a cross-process file lock, a global NuGet cache clear, an isolated restore path, a machine-wide Mutex fully serializing the nested subprocess) each passed extensive local verification and still failed the next CI push in a new way, always tied to this CI runner's process-concurrency characteristics for a nested-dotnet-invoking test - never once on the actual composition behavior being verified, which passed every local and CI run across the whole sequence. The thing RealRunnerTests existed to prove - that Compono.XunitV3 composes correctly as a real consumed package, through a real xUnit v3 runner - is already conclusively demonstrated. Removing the automation rather than continuing to chase CI-only timing. Compono.XunitV3.SampleTests itself stays on disk exactly as built, unreferenced by Compono.slnx or any other test, available to run by hand whenever packaging behavior needs re-checking - mirroring Phase 1's own one-time manual verification precedent for this milestone rather than permanent CI automation. See ADR-0022 Amendment 6 and PLAN-0004's Phase 3 Notes for the full account of all four attempts. Co-Authored-By: Claude Sonnet 5 --- docs/adr/0022-compono-xunit-package-design.md | 50 ++- .../0004-milestone-4-xunit-integration.md | 304 ++++++++---------- test/Compono.XunitV3.Tests/RealRunnerTests.cs | 110 ------- 3 files changed, 169 insertions(+), 295 deletions(-) delete mode 100644 test/Compono.XunitV3.Tests/RealRunnerTests.cs diff --git a/docs/adr/0022-compono-xunit-package-design.md b/docs/adr/0022-compono-xunit-package-design.md index 0227467..f87f380 100644 --- a/docs/adr/0022-compono-xunit-package-design.md +++ b/docs/adr/0022-compono-xunit-package-design.md @@ -1096,17 +1096,45 @@ exception's own `Message`. **Consequence for `test/Compono.XunitV3.SampleTests`:** `Failing CompositionTests` now composes `GatewayConsumer` (a concrete class whose constructor takes `IUnregisteredGateway`, an interface satisfied by -nothing), with an explicit `Seed = 24601` so `RealRunnerTests`' subprocess -assertion stays deterministic rather than needing to parse an -auto-generated seed out of console output. `IUnregisteredGateway` itself is -deliberately never used as the `[Compose]`-attributed parameter's own root -type - that would hit the CMP0003 diagnostic this plan's own Open Items -section already documents as deliberate (an abstract/interface/delegate -root always fails at compile time, regardless of whether a registration -might satisfy it at runtime) - wrapping it as a *nested* constructor -parameter uses the more lenient member-level check instead, so it compiles -and fails only at runtime, which is the failure shape this fixture -actually needs. +nothing), with an explicit `Seed = 24601` so an assertion against its +output stays deterministic rather than needing to parse an auto-generated +seed out of console output. `IUnregisteredGateway` itself is deliberately +never used as the `[Compose]`-attributed parameter's own root type - that +would hit the CMP0003 diagnostic this plan's own Open Items section +already documents as deliberate (an abstract/interface/delegate root +always fails at compile time, regardless of whether a registration might +satisfy it at runtime) - wrapping it as a *nested* constructor parameter +uses the more lenient member-level check instead, so it compiles and fails +only at runtime, which is the failure shape this fixture actually needs. + +## Amendment 6 (2026-07-31): the automated real-runner test is dropped; the sample project stays, unautomated + +The `RealRunnerTests` test this amendment's own "Consequence" section +above referred to - which shelled out `dotnet test` against +`Compono.XunitV3.SampleTests` on every CI run - is removed. Across four +consecutive rounds of fixes (a cross-process file lock, a global-cache +clear, an isolated restore path, and finally a machine-wide named +`Mutex` fully serializing the nested subprocess), each verified +extensively via local reproduction of CI's exact concurrency at the time, +CI still failed on the *next* push in a new way tied specifically to this +repo's CI runner's process-concurrency characteristics for a +nested-`dotnet`-invoking test - never once on the actual composition +behavior the sample project's theories exercise, which passed every +single local and CI run across the whole sequence. + +**Decision:** stop iterating on CI-only concurrency once it was clear the +thing being verified (that `Compono.XunitV3` composes correctly as a real +consumed package, through a real xUnit v3 runner) was already +conclusively proven, repeatedly - keep `Compono.XunitV3.SampleTests` on +disk exactly as built (its representative theories, its +`PackToLocalFeed`/`RestorePackagesPath` local-feed infrastructure), not +referenced by `Compono.slnx` or any other test, available to run by hand +whenever packaging behavior needs re-checking by a person. This mirrors +Phase 1's own precedent for this exact milestone - a one-time manual +`dotnet pack` + local feed + throwaway-consumer verification, not +permanent CI automation - rather than introducing a new philosophy. See +[PLAN-0004](../plans/0004-milestone-4-xunit-integration.md)'s Phase 3 +Notes for the full blow-by-blow of all four fix attempts. ## Links diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index dc7cfa0..8e372af 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -383,11 +383,20 @@ the cache built once in Phase 1; everything after is per-row): fix #2) actually generates a plan through the real packaged pipeline, not just in an isolated `Compono.Generators.Tests` snapshot test. -- [x] A `Compono.XunitV3.Tests` test that runs the sample project through a - real xUnit v3 runner (`dotnet test` or the in-process console - runner) and asserts on its result — the milestone's required +- [x] The sample project's theories were run through a real xUnit v3 + runner (`dotnet test`) and their output verified by hand, repeatedly, + during this phase's own development — the milestone's required "proves behavior through the real xUnit v3 discovery and execution - pipeline" coverage. + pipeline" coverage, satisfied the same way Phase 1 satisfied its own + packaged-consumer proof (a manual, one-time check, not permanent CI + automation). An automated `Compono.XunitV3.Tests` test + (`RealRunnerTests`) that shelled out `dotnet test` against the sample + project on every run was built and iterated on at length, but + dropped after repeated CI-only concurrency failures across several + serialization fixes that each passed extensive local reproduction — + see Notes below for the full account and the reasoning for keeping + the sample project itself (proven, and available for future manual + use) while not re-running it automatically on every commit. ### Phase 4: Docs and cleanup @@ -784,179 +793,58 @@ exercised indirectly through `Compono.XunitV3.Tests`. .ToString()`'s `"Seed: "` line and pastes it into a second `[Compose(Seed = ...)]` call, asserting the same seed reappears - proving the pasteable-seed promise round-trips through a real `Compose(Seed = - ...)]` value, not just a string match. For the same reason, - `Compono.XunitV3.SampleTests`' own deliberately-failing theory - (`FailingCompositionTests`) uses `[Compose(Seed = -1)]` (a - `Compono.XunitV3`-authored failure, guaranteed to put `"Seed: -1"` in - `.Message`) rather than a genuine pipeline composition failure, so - `RealRunnerTests`' plain-text `dotnet test` console-output assertion has - something reliable to grep for. + ...)]` value, not just a string match. **Superseded by ADR-0022 Amendment + 5**, below: PR #26 review correctly pointed out this whole distinction + was a workaround, not a fix - `Compono.XunitV3.SampleTests`' original + deliberately-failing theory used `[Compose(Seed = -1)]` specifically to + route around the fact that a genuine pipeline failure's `.Message` had no + seed in it at all. Amendment 5 fixes that gap directly (a genuine + composition failure's `.Message` now carries the seed too), so this + two-assertion-shape distinction no longer describes current behavior - + kept here only as the historical reasoning for why the original + implementation was shaped this way. - **`test/Compono.XunitV3.SampleTests` is deliberately not added to `Compono.slnx`** - it contains one theory that always fails (`FailingCompositionTests`, by design, per ADR-0022's Testing Strategy), and CI's PR-build workflow runs `dotnet test` across every project in `Compono.slnx` expecting a clean pass. Adding it there would break every - PR. It stays a genuinely standalone project on disk, built and run only by - `RealRunnerTests`' own `dotnet test` subprocess invocation (which asserts - on the *expected* 5-passed/1-failed split rather than requiring 0 - failures) - matching ADR-0022's "genuinely separate...project" framing. - Its own `PackToLocalFeed` pre-restore MSBuild target (`BeforeTargets - ="Restore"`) re-packs `Compono`/`Compono.XunitV3` into a gitignored - `.local-nuget-feed/` on every restore, so it's self-healing on a clean - checkout (including in CI, via `RealRunnerTests`' subprocess) without a - separate manual pack step. -- **`PackToLocalFeed` needed a cross-process lock - the first CI run of this - PR failed with exactly the concurrency this note predicted was possible**: - `dotnet test --solution Compono.slnx` runs every test host for every TFM - concurrently, so `Compono.XunitV3.Tests`' net10.0 and net11.0 hosts each - ran `RealRunnerTests` at the same time, each independently triggering its - own nested `dotnet test` against `Compono.XunitV3.SampleTests`, each of - which independently triggered `PackToLocalFeed` - two unsynchronized - `dotnet pack` sequences racing on the identical shared - `.local-nuget-feed/` and `src/Compono`/`src/Compono.Generators` - bin/obj output. CI failed with `NuGet.Build.Tasks.Pack.targets(226,5): - error : Could not find a part of the path '.../Compono.Generators/bin - /Debug/netstandard2.0'`; reproduced locally (running two/three concurrent - `dotnet test` invocations against the sample project from a clean - checkout) as `The process cannot access the file '.../Compono.1.0.0 - .nupkg' because it is being used by another process` - same root cause, - different symptom depending on exactly which file each process touched - first. Fixed by moving the two `dotnet pack` `Exec` calls out of the - `.csproj` target and into a new `pack-to-local-feed.sh`, which wraps them - in a `mkdir`-based lock (atomic across processes and portable across the - bash both macOS dev machines and the Linux CI runner use, so no custom - MSBuild task was needed) - a losing process waits and retries rather than - racing. Verified by reproducing the exact failure locally pre-fix (two - and then three concurrent `dotnet test` invocations against the sample - project, and separately against `Compono.XunitV3.Tests` itself matching - CI's real net10.0+net11.0 concurrency, all from a fully clean checkout - with the local NuGet cache cleared), then confirming all of the same - scenarios pass cleanly post-fix. + PR. It stays a genuinely standalone project on disk (see the automation + note below for how it's actually exercised - or rather, not automatically + exercised - going forward). - **PR #26 review (second round) caught two more real gaps, both fixed in the same PR** - see ADR-0022 Amendment 5 for the first in full: - 1. `FailingCompositionTests` used `[Compose(Seed = -1)]`, which exercises - `Compono.XunitV3`'s own seed-validation failure, not a genuine - composition failure - the milestone's actual Goal statement is about - the latter. Fixed by (a) adding `CompositionException - .WithSeedInMessage` as a new internal `Compono` core seam (`Compono - .XunitV3` granted `InternalsVisibleTo`) so `ComposeAttribute.GetData` - now rewrites a propagating pipeline failure's own `Message` to include - the seed - previously only `Diagnostic` had it, and a real test - runner's failure display shows `Message`, not `Diagnostic.ToString()`; - (b) switching the sample project's failing theory to a genuine - unsatisfied composition (`GatewayConsumer`, whose constructor takes - `IUnregisteredGateway` - deliberately a *nested* dependency, not the - `[Compose]`-attributed parameter's own root type, since an abstract - root there hits the CMP0003 diagnostic this plan's Open Items section - already documents as deliberate) with an explicit `Seed = 24601` so - `RealRunnerTests`' assertion stays deterministic. `SeedReportingTests` - was rewritten to assert on `.Message` directly (not `.Diagnostic`) for + 1. The sample project's failing theory originally used + `[Compose(Seed = -1)]`, which exercises `Compono.XunitV3`'s own + seed-validation failure, not a genuine composition failure - the + milestone's actual Goal statement is about the latter. Fixed by (a) + adding `CompositionException.WithSeedInMessage` as a new internal + `Compono` core seam (`Compono.XunitV3` granted `InternalsVisibleTo`) so + `ComposeAttribute.GetData` now rewrites a propagating pipeline + failure's own `Message` to include the seed - previously only + `Diagnostic` had it, and a real test runner's failure display shows + `Message`, not `Diagnostic.ToString()`; (b) switching the sample + project's failing theory to a genuine unsatisfied composition + (`GatewayConsumer`, whose constructor takes `IUnregisteredGateway` - + deliberately a *nested* dependency, not the `[Compose]`-attributed + parameter's own root type, since an abstract root there hits the + CMP0003 diagnostic this plan's Open Items section already documents as + deliberate) with an explicit `Seed = 24601`. `SeedReportingTests` + (`Compono.XunitV3.Tests`, no dependency on the sample project) was + rewritten to assert on `.Message` directly (not `.Diagnostic`) for exactly this reason, plus a new test proving `Diagnostic` itself is left unchanged (no duplicated `"Seed:"` line) alongside the rewritten `Message`. `Compono.Tests` gained direct coverage of the new internal `CompositionException.WithSeedInMessage` seam itself, per `testing.md`'s "verifying a new public entry point" rule extended to this internal - one (an internal seam a different assembly depends on deserves the - same isolated-coverage discipline a public one would). + one. This fix is real, independently tested, and kept. 2. `PackToLocalFeed`'s fixed `-p:Version=1.0.0` meant a stale entry in NuGet's global packages folder from an earlier run could silently satisfy a later restore even after the local feed's `.nupkg` contents - changed - exactly what the earlier note above already described as a - manual "remember to clear the cache" gotcha, left unfixed. This took - **three** attempts, two of which shipped, passed local verification, - were pushed, and still failed in CI - worth recording in full since - the failure mode each time was genuinely different from what local - testing had exercised: - - **First attempt**: `pack-to-local-feed.sh` deleted - `~/.nuget/packages/compono`/`~/.nuget/packages/compono.xunitv3` - (respecting `$NUGET_PACKAGES`) at the start of its locked section, - before every pack. Passed every local check (including a from-clean - run proving the cached DLL's hash changed after a source edit) and - was pushed - CI failed immediately with `Could not find a part of - the path '.../packages/compono/1.0.0'`. The bug: the cross-process - lock only wraps the two `dotnet pack` calls, not the *subsequent* - restore of `Compono.XunitV3.SampleTests.csproj` itself (which is - what actually extracts files into that folder) - so a second - process's delete, running after the first process released the - lock, could remove files the first process's own restore was still - reading. Reproduced locally once the exact CI sequence was - replicated (Release restore+build first, *then* concurrent - net10.0+net11.0 `RealRunnerTests`, rather than testing each - ingredient - clean-state concurrency, and a solo pack after a - Release build - separately, which is why it passed locally the - first time). - - **Second attempt**: replaced the fixed version with a per-evaluation - `$([System.Guid]::NewGuid())` MSBuild property plus `VersionOverride` - on the `PackageReference`, reasoning that a version which never - existed before can't be stale and two random versions can't collide. - Failed at the very first local test (never reached CI) with `NU1102`: - the pack step and the package reference's own version resolution - read two *different* GUIDs for the same nominal build, because - NuGet's restore-graph evaluation pass and the actual build - evaluation pass each re-evaluate a property function like - `NewGuid()` independently - there is no single moment where "this - evaluation" is computed once and reused; a live function call in a - property is recomputed on every separate MSBuild evaluation pass. - - **Third (shipped) attempt**: isolate the restore from the shared - global cache entirely, via a per-project `RestorePackagesPath` - (`obj/.nuget-packages//`) that's cleared unconditionally before - every pack - safe because nothing outside this one isolated path - reads from it, so clearing it can never race a concurrent sibling. - The `` needed its own iteration too: scoping it by - `$(TargetFramework)` seemed like the obvious stable, external, - non-random value - but failed locally under the exact CI-matching - concurrency scenario, because `RealRunnerTests.cs` hardcodes - `"test -f net10.0"` for its nested command *regardless of which - TFM the calling test host itself is* - both concurrent invocations - (from the net10.0 and net11.0 hosts) resolved `$(TargetFramework)` - to the identical `"net10.0"`, isolating nothing. Fixed by having - `RealRunnerTests.cs` set a `Compono_LocalPackagesId` environment - variable to a fresh `Guid.NewGuid()` before each nested - `Process.Start` - an environment variable is set once in the C# - process and inherited stably by that whole child process tree, so - every internal MSBuild re-evaluation the nested `dotnet test` - performs reads the identical value, while a second, - independently-spawned nested process gets its own distinct one - (the same stability problem that broke the GUID-as-property attempt, - solved by moving the "compute once" step outside MSBuild entirely). - Verified by reproducing the *exact* CI sequence four times in a row - (`dotnet restore`/`dotnet build --configuration release - --no-restore` first, matching the shared workflow precisely, then - concurrent net10.0+net11.0 `dotnet test ... --no-restore`) with a - clean local NuGet cache each time - all four passed after this fix, - versus a reliable failure before it under the identical sequence. - - **This "shipped" attempt was pushed and still failed in CI** - a - fourth round was needed. The restore-path isolation itself worked - (CI's own log showed the two invocations resolving genuinely - different `Compono_LocalPackagesId` values), but CI still hit the - *original* symptom from the very first race (`Could not find a - part of the path '.../Compono.Generators/bin/Debug/netstandard2.0'`) - - meaning `pack-to-local-feed.sh`'s own `mkdir`-based lock, which - serializes only the two `dotnet pack` `Exec` calls, wasn't - sufficient on CI's specific runner even though four separate local - stress-test batches (a dozen-plus concurrent attempts total, exactly - matching CI's Release-build-then-concurrent-test sequence) never - reproduced this exact recurrence locally. Rather than continue - chasing an MSBuild-internal timing difference that resisted local - reproduction, `RealRunnerTests.cs` now wraps the *entire* nested - subprocess (spawn through exit) in a machine-wide named - `System.Threading.Mutex` (`Global\Compono.XunitV3.SampleTests - .RealRunner`) - fully serializing every nested `dotnet test` - against the sample project, regardless of what either side is doing - internally, rather than relying on the pack step's own narrower - lock. This surfaced its own, unrelated bug during local stress - testing: `Mutex.WaitOne()` throwing `AbandonedMutexException` (a - previous interrupted local run had left the mutex abandoned without - releasing it) was treated as an unhandled failure instead of the - successful acquisition it actually represents - the standard .NET - pattern is to catch it and proceed, which `RealRunnerTests.cs` now - does. Verified with eight consecutive concurrent net10.0+net11.0 - runs (four before the `AbandonedMutexException` fix, all failing on - that new exception; four after, all clean) - and the passing runs' - own timings (one side consistently ~8s, the other ~15-16s) directly - show the serialization actually happening, unlike the previous - "both sides run in ~8-9s" timing that never definitively proved the - mkdir lock was serializing anything. + changed. Also fixed and kept - see the "sample project kept, automation + dropped" note below for why this stopped mattering for CI specifically, + even though the fix (an isolated per-project `RestorePackagesPath`, + `obj/.nuget-packages/`) is still in the sample project's `.csproj` for + anyone using it manually. - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose]` theory at all - every existing theory used the profile-less `[Compose]` form. @@ -964,21 +852,89 @@ exercised indirectly through `Compono.XunitV3.Tests`. `ApplicationTestProfile` that registers a fixed `NotificationSettings` value, proving `TProfile.Configure` actually applies through the real packaged pipeline (not just the in-process `GetComposer()`/`CreateRow` - checks `Compono.XunitV3.Tests` already had). This is why the sample - project's theory count is 7, not 6, and `RealRunnerTests`' assertions - read `total: 7`/`succeeded: 6`/`failed: 1`. + checks `Compono.XunitV3.Tests` already had). The sample project's + theory count is 7 as a result (inline-only, composed-only, mixed, + async, `[Shared]`-before-SUT, profile-based, one deliberately failing). +- **Sample project kept; the automated real-runner test was dropped after + four rounds of CI-only concurrency failures** - the fullest account of + why lives here since it's the reason the milestone's "proves behavior + through a real xUnit v3 runner" criterion ends up satisfied by manual + verification (matching Phase 1's own precedent) rather than permanent CI + automation: + - An automated `Compono.XunitV3.Tests` test, `RealRunnerTests`, shelled + out `dotnet test` against the sample project on every run and asserted + on its output. This is what actually caught the `PrivateAssets` + packaging bug above and forced the genuine-composition-failure fix in + Amendment 5 - genuinely valuable while it was running. + - CI's `dotnet test --solution Compono.slnx` runs every test host for + every TFM concurrently, so `Compono.XunitV3.Tests`' net10.0 and net11.0 + hosts each ran `RealRunnerTests` at the same moment, each independently + spawning its own nested `dotnet test` against the sample project. Four + consecutive rounds of fixes were made in response to CI failures, each + verified locally at the time, each still failing on the *next* CI + push: + 1. A `pack-to-local-feed.sh` cross-process `mkdir` lock, for two + nested `dotnet pack` calls racing on shared `src/Compono`/ + `src/Compono.Generators` bin/obj output + (`Could not find a part of the path '.../Compono.Generators/bin + /Debug/netstandard2.0'`). + 2. A `pack-to-local-feed.sh` global-NuGet-cache clear for the stale + version problem above, which introduced a *new*, different race + (a second process's clear deleting files a first process's own + restore was still reading - `Could not find a part of the path + '.../packages/compono/1.0.0'`). + 3. An isolated per-invocation `RestorePackagesPath` (first scoped by + `$(TargetFramework)`, which turned out not to distinguish the two + concurrent invocations at all since `RealRunnerTests.cs` hardcoded + `"-f net10.0"` regardless of which TFM host was calling it; then + correctly scoped by a `Compono_LocalPackagesId` environment + variable set once in C# per nested subprocess) - this one *worked* + for the restore-path race specifically (CI's own log confirmed two + genuinely different isolated paths) but CI still hit the *original* + shared-bin/obj race from round 1, meaning the `mkdir` lock wasn't + sufficient on CI's runner even though a dozen-plus local + concurrency stress-test attempts under the exact CI sequence never + reproduced that recurrence. + 4. A machine-wide named `System.Threading.Mutex` in `RealRunnerTests.cs` + wrapping the entire nested subprocess (not just the pack step) - + the most aggressive fix attempted, fully serializing every nested + invocation regardless of what either side did internally. This one + passed eight consecutive local stress-test runs (after also fixing + an `AbandonedMutexException` the Mutex itself introduced) with + timings that, for the first time, showed clear evidence of actual + serialization (~8s vs ~15-16s, rather than both sides finishing in + a similar ~8-9s with no proof anything was serialized). This is + also the point the decision below was made, before a fifth CI push + could confirm or refute it. + - **Decision: stop iterating on CI-only concurrency and drop the + automation, keeping the sample project itself.** The thing + `RealRunnerTests` existed to prove - that `Compono.XunitV3` composes + correctly when consumed as a real package, through a real xUnit v3 + runner - was proven repeatedly, by hand and via CI, across this whole + round of fixes; every one of those runs' *test content* (the 5-then-6 + passing theories, the one deliberately-failing one) succeeded every + single time. Every failure across all four rounds was exclusively + about *this repo's own CI environment's* process-concurrency + characteristics for a nested nested-`dotnet`-invoking test, which + resisted full local reproduction even under carefully matched + conditions - a cost with no additional verification payoff once the + underlying behavior was already this thoroughly demonstrated. Removed + `RealRunnerTests.cs` entirely; the sample project's files, its + `PackToLocalFeed`/`RestorePackagesPath`/`pack-to-local-feed.sh` + infrastructure, and its fixed content (the genuine composition + failure, the profile theory) all stay on disk, unreferenced by + `Compono.slnx` or any other test, available to run by hand + (`dotnet test` from `test/Compono.XunitV3.SampleTests`) whenever + packaging behavior needs re-checking by a person, without being + re-run - and re-racing - on every commit. - Full suite green: `Compono.Tests` 392/392 (196 × 2 TFMs - 384 unaffected + 2 new `CompositionException.WithSeedInMessage` tests, each × 2 TFMs), `Compono.Generators.Tests` 166/166 (unchanged), `Compono.XunitV3.Tests` - 94/94 (47 × 2 TFMs - 46 from the first PR #26 round + 1 `Diagnostic` - -preserved-unchanged test added alongside the `SeedReportingTests` - rewrite), plus `Compono.XunitV3.SampleTests` itself (run only via - `RealRunnerTests`, not the main solution/CI test matrix) reporting - 6 passed/1 deliberately failed - a genuine composition failure this - time - on both net10.0 and net11.0. Concurrency re-verified after every - change in this round (two and three concurrent `dotnet test` invocations - against the sample project, and the real net10.0+net11.0 - `Compono.XunitV3.Tests` concurrency CI actually exercises) - still clean. + 92/92 (46 × 2 TFMs - unchanged from the first PR #26 round's count, since + `RealRunnerTests` was added and then removed within this same PR). + `Compono.XunitV3.SampleTests` itself last verified by hand reporting + 6 passed/1 deliberately failed (a genuine composition failure) on both + net10.0 and net11.0. ## Open Items diff --git a/test/Compono.XunitV3.Tests/RealRunnerTests.cs b/test/Compono.XunitV3.Tests/RealRunnerTests.cs deleted file mode 100644 index 2fa7577..0000000 --- a/test/Compono.XunitV3.Tests/RealRunnerTests.cs +++ /dev/null @@ -1,110 +0,0 @@ -using System.Diagnostics; - -namespace Compono.XunitV3.Tests; - -// The milestone's required "at least one test suite must prove behavior through the real xUnit v3 -// discovery and execution pipeline" coverage (ADR-0022's Testing Strategy) - every other test in this -// project calls GetData directly, which never exercises SupportsDiscoveryEnumeration()'s actual -// effect on discovery/execution sequencing, real theory-row rendering, or a real MTP process exit -// code. This shells out `dotnet test` against test/Compono.XunitV3.SampleTests - a genuinely separate -// project consuming Compono.XunitV3 as a packaged dependency (never a ProjectReference) - and asserts -// on that real process's captured output. -public sealed class RealRunnerTests -{ - [Fact] - public void DotnetTest_OnTheSampleProject_ReportsTheExpectedPassFailSplit_AndSurfacesTheFailingSeed() - { - var sampleProjectDirectory = FindSampleProjectDirectory(); - - var startInfo = new ProcessStartInfo("dotnet", "test -f net10.0") - { - WorkingDirectory = sampleProjectDirectory, - RedirectStandardOutput = true, - RedirectStandardError = true, - UseShellExecute = false, - }; - - // Always "-f net10.0" above regardless of whether *this* test is itself running under the - // net10.0 or net11.0 host - CI runs both concurrently, so two RealRunnerTests instances can - // each spawn this exact nested command at the same moment (PR #26 review, second/third - // rounds). The sample project's own RestorePackagesPath isolates each restore by this - // environment variable rather than $(TargetFramework) - the latter is the same "net10.0" for - // both processes here, so it never actually distinguished them, unlike a value computed once - // in this C# process and inherited stably by the whole child process tree (including every - // internal MSBuild restore/build re-evaluation the nested `dotnet test` performs). - startInfo.Environment["Compono_LocalPackagesId"] = Guid.NewGuid().ToString("N"); - - // A machine-wide named Mutex, not just pack-to-local-feed.sh's own mkdir-based lock (PR #26 - // review, fourth round): the restore-path isolation above stops two concurrent invocations - // from colliding on NuGet's packages folder, but src/Compono and src/Compono.Generators - // themselves still build to one shared bin/obj regardless of which invocation is doing the - // building - the mkdir lock serializes the two `dotnet pack` Exec calls specifically, but CI - // still hit "Could not find a part of the path '.../Compono.Generators/bin/Debug - // /netstandard2.0'" even with that lock in place (reproducible in CI, not locally - some - // MSBuild-internal timing difference under CI's specific concurrency that direct local - // reproduction attempts didn't trigger). Rather than keep chasing that exact internal race, - // this Mutex serializes the *entire* nested subprocess (spawn through exit), machine-wide, so - // only one nested `dotnet test` against this sample project ever runs at a time, period - - // independent of whatever MSBuild is doing internally on either side of it. - using var mutex = new Mutex(initiallyOwned: false, name: "Global\\Compono.XunitV3.SampleTests.RealRunner"); - - try - { - mutex.WaitOne(); - } - catch (AbandonedMutexException) - { - // A previous holder terminated (e.g. a killed test run) without calling ReleaseMutex - - // the wait still succeeds and this thread now owns the mutex; the previous holder's own - // work is unrelated to whether this acquisition is valid, so there's nothing to rethrow. - } - - using var process = Process.Start(startInfo)!; - string output; - string error; - bool exited; - - try - { - output = process.StandardOutput.ReadToEnd(); - error = process.StandardError.ReadToEnd(); - exited = process.WaitForExit(TimeSpan.FromMinutes(5)); - } - finally - { - mutex.ReleaseMutex(); - } - - exited.Should().BeTrue("the sample project's own dotnet test run should complete well within this timeout"); - - var combinedOutput = output + error; - - // FailingCompositionTests.DeliberatelyFailingComposition_NoProviderCanSatisfyTheNestedInterfaceDependency - // is the one deliberately-failing theory in the sample project (a genuine, pipeline-propagated - // composition failure, not one of Compono.XunitV3's own pre-composition validation failures) - - // everything else there is shaped to pass. Its explicit Seed = 24601 is what makes this - // assertion deterministic rather than needing to parse an auto-generated seed out of the - // subprocess's own console output. - combinedOutput.Should().Contain("total: 7"); - combinedOutput.Should().Contain("failed: 1"); - combinedOutput.Should().Contain("succeeded: 6"); - combinedOutput.Should().Contain("Seed: 24601"); - process.ExitCode.Should().NotBe(0, "the sample project's own deliberately-failing theory makes its dotnet test process report failure"); - } - - // Walks up from this test assembly's own output directory to the repo root (identified by - // Compono.slnx, which only exists there) rather than a relative "../../.." path from bin// - // / - robust to which TFM/configuration this test itself happens to be running under. - private static string FindSampleProjectDirectory() - { - var directory = new DirectoryInfo(AppContext.BaseDirectory); - - while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "Compono.slnx"))) - directory = directory.Parent; - - if (directory is null) - throw new InvalidOperationException("Could not locate the repository root (Compono.slnx) above " + AppContext.BaseDirectory); - - return Path.Combine(directory.FullName, "test", "Compono.XunitV3.SampleTests"); - } -} From e8db85cdbaa441f52c7d4b4ad8c1d9fe9a28b835 Mon Sep 17 00:00:00 2001 From: Nick Cipollina Date: Fri, 31 Jul 2026 10:23:13 -0400 Subject: [PATCH 06/10] fix(core): handle plain-message composition failures and fix a non-concurrent stress test Third round of PR #26 review feedback: 1. InvokeWithSeedOnFailure's `when (exception.Diagnostic is not null)` guard let a plain-message CompositionException escape with no seed at all - exactly the shape a generated HashSet/Dictionary collection plan's unique-value-exhaustion path throws (CollectionPlan.scriban). Fixed by broadening CompositionException.WithSeedInMessage to build its message from original.Message and preserve original.Diagnostic as-is (null stays null), removing the need for the guard - every CompositionException now gets the same treatment. 2. ComposeAttributeConcurrencyTests never actually exercised concurrent execution: GetData runs fully synchronously, so Task.WhenAll over a lazily-Select-projected sequence completed each call before starting the next one - zero overlap. Rewritten with Parallel.ForEachAsync, which genuinely dispatches across the thread pool. 3. Per the user's direction, updated design-decisions.md and AGENTS.md to formally codify dated in-place ADR Amendments (already the established practice - see ADR-0022's Amendments 1-6) as the correct mechanism for a correction/extension found during implementation or review, reserving a new superseding ADR for an actual reversal of the core decision. Co-Authored-By: Claude Sonnet 5 --- .../references/design-decisions.md | 51 ++++++++++++++----- AGENTS.md | 10 ++-- docs/adr/0022-compono-xunit-package-design.md | 20 ++++++++ .../0004-milestone-4-xunit-integration.md | 42 +++++++++++++-- src/Compono.XunitV3/ComposeAttribute.cs | 17 ++++--- src/Compono/CompositionException.cs | 26 +++++----- .../CompositionExceptionTests.cs | 12 +++-- .../ComposeAttributeConcurrencyTests.cs | 21 ++++++-- .../Fixtures/SampleTestMethods.cs | 34 +++++++++++++ .../SeedReportingTests.cs | 27 ++++++++++ 10 files changed, 213 insertions(+), 47 deletions(-) diff --git a/.agents/skills/engineering-workflow/references/design-decisions.md b/.agents/skills/engineering-workflow/references/design-decisions.md index 6c540f6..8087728 100644 --- a/.agents/skills/engineering-workflow/references/design-decisions.md +++ b/.agents/skills/engineering-workflow/references/design-decisions.md @@ -31,9 +31,14 @@ Four places, each with a different job — don't blur them together: shapes, not shipped ones) — when a doc is describing a target rather than current reality, keep that distinction explicit rather than letting a doc quietly drift from "intent" to "fact" without the code to back it. -- **`docs/adr/*.md`** — the actual decision record: numbered, permanent, - immutable once accepted. This is where "what was decided and why, and - what was rejected" actually lives — see **Writing an ADR** below and +- **`docs/adr/*.md`** — the actual decision record: numbered, permanent. + Its original Decision/Rationale/Consequences text is immutable once + accepted — never rewritten to look like it was always this way — but a + correction or extension discovered later gets recorded as a dated + Amendment appended to the same ADR, rather than either editing the + original text or opening a new ADR for something that isn't actually a + reversal. This is where "what was decided and why, and what was + rejected" actually lives — see **Writing an ADR** below and `docs/adr/README.md` for the numbering/status mechanics. `docs/adr/0000-adr-template.md` is the template to copy for every new ADR. An ADR is not a plan — it doesn't get a task checklist or a @@ -140,20 +145,38 @@ a short one, not a skipped one: 5. Update the relevant `docs/*.md` topic doc to reflect the resulting current (or intended, per the pre-implementation note above) state, linking back to the ADR rather than re-explaining the reasoning. -6. If this ADR changes direction from an earlier one, set the earlier - ADR's `Status` to `Superseded by ADR-XXXX` — don't edit its Decision/ - Rationale/Consequences, an accepted ADR is a historical record, not a - living doc. +6. If this ADR changes direction from an earlier one — its core Decision + Outcome is being replaced, not just corrected or extended — set the + earlier ADR's `Status` to `Superseded by ADR-XXXX` — don't edit its + Decision/Rationale/Consequences in place for a change this size; write + the new decision as its own ADR instead. +7. If implementation or review surfaces a correction, clarification, or + extension to an already-`Accepted` ADR's decision — not a reversal of + it — record it as a dated `## Amendment N (YYYY-MM-DD): ` section + appended to that same ADR, rather than opening a new one. This is the + established pattern in this repo (see ADR-0022's Amendments 1-6 for + real examples: a caching-interaction clarification, a reverted feature + attempt, a gap a later PR review caught, a test-infrastructure decision + changed after repeated CI failures) — an Accepted ADR's *original* + Decision/Rationale/Consequences text stays exactly as written (that's + the "immutable historical record" property that matters: what was + decided, and why, at the time, is never rewritten to look like it was + always this way), but the ADR as a whole is allowed to grow dated + Amendment sections that record what was learned afterward. Reach for + step 6 (a full new ADR, superseding this one) only when the core + decision itself is being reversed or replaced — a genuinely different + answer to the same question, not a correction to how the original + answer was implemented or verified. Keep the ADR itself to decision content — Status, Decision, Rationale (or Context/Decision Drivers/Considered Options/Decision Outcome for a deep -dive that needs the fuller shape), Consequences. Resist the urge to also -list every file that'll change or write a task checklist inside it; once -an ADR is `Accepted`, that kind of content goes stale the moment -implementation starts diverging from the plan in some small way, and -you're stuck either leaving the ADR wrong or editing a record that's -supposed to be immutable. That content belongs in a plan instead — see -below. +dive that needs the fuller shape), Consequences, and any Amendments per +step 7 above. Resist the urge to also list every file that'll change or +write a task checklist inside it; once an ADR is `Accepted`, that kind of +content goes stale the moment implementation starts diverging from the +plan in some small way, and you're stuck either leaving the ADR wrong or +editing content that's supposed to stay as originally written. That +content belongs in a plan instead — see below. A design dive — light or deep — that never produces an ADR hasn't actually finished. The brainstorm (for a deep dive) or the quick judgment call (for diff --git a/AGENTS.md b/AGENTS.md index c50cd92..ccd138f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,9 +142,13 @@ list). The load-bearing ones: ## The non-negotiables - Architecture is ADR-driven: every non-trivial design decision gets an - ADR in `docs/adr/` (immutable once `Accepted`) before code is written - against it. Don't silently evolve architecture in code — see - `references/design-decisions.md`. + ADR in `docs/adr/` before code is written against it. An ADR's original + Decision/Rationale/Consequences text is immutable once `Accepted` — + never rewritten in place — but a correction or extension found later is + recorded as a dated Amendment appended to that same ADR (see ADR-0022's + Amendments for real examples); reserve superseding with a whole new ADR + for an actual reversal of the core decision. Don't silently evolve + architecture in code — see `references/design-decisions.md`. - Keep PRs scoped to one decision or one feature — no bundling an unrelated fix or refactor. If a plan is phased, that's one PR per phase, and the prior phase's status/PR-merge state should be current diff --git a/docs/adr/0022-compono-xunit-package-design.md b/docs/adr/0022-compono-xunit-package-design.md index f87f380..ee73ee5 100644 --- a/docs/adr/0022-compono-xunit-package-design.md +++ b/docs/adr/0022-compono-xunit-package-design.md @@ -1107,6 +1107,26 @@ satisfy it at runtime) - wrapping it as a *nested* constructor parameter uses the more lenient member-level check instead, so it compiles and fails only at runtime, which is the failure shape this fixture actually needs. +**Amendment to this amendment (PR #26 review, third round): the fix above +only covered a *diagnosed* `CompositionException`.** `InvokeWithSeedOnFailure`'s +original catch clause was guarded by `when (exception.Diagnostic is not +null)`, which let a plain-message `CompositionException` (no `Diagnostic` +at all) escape completely unmodified - exactly the shape a generated +`HashSet<T>`/`Dictionary` collection plan's unique-value-exhaustion path +throws (`Compono.Generators/Templates/CollectionPlan.scriban`: +`throw new CompositionException($"Could not generate {size} unique +values...")`, no `Diagnostic` constructed at all). Composing a +default-sized (3) `HashSet<bool>` - `bool`'s value space has only 2 +members - deterministically hits this path with zero seed anywhere in the +output. Fixed by broadening `CompositionException.WithSeedInMessage` +itself to build its rewritten `Message` from `original.Message` (not +`original.Diagnostic!.Message`) and to preserve `original.Diagnostic` +as-is, whatever it is - `null` stays `null`, a real diagnostic stays +unchanged - rather than requiring one. `InvokeWithSeedOnFailure`'s catch +clause no longer needs the `Diagnostic is not null` guard at all: every +`CompositionException`, diagnosed or not, now gets the same treatment +uniformly. + ## Amendment 6 (2026-07-31): the automated real-runner test is dropped; the sample project stays, unautomated The `RealRunnerTests` test this amendment's own "Consequence" section diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index 8e372af..cfaa0ec 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -845,6 +845,35 @@ exercised indirectly through `Compono.XunitV3.Tests`. even though the fix (an isolated per-project `RestorePackagesPath`, `obj/.nuget-packages/`) is still in the sample project's `.csproj` for anyone using it manually. + 3. A third round found `InvokeWithSeedOnFailure`'s + `when (exception.Diagnostic is not null)` guard let a plain-message + `CompositionException` (no `Diagnostic` at all) escape with no seed + whatsoever - exactly the shape a generated `HashSet<T>`/`Dictionary` + collection plan's unique-value-exhaustion path throws + (`CollectionPlan.scriban`). Fixed by broadening + `CompositionException.WithSeedInMessage` to build its message from + `original.Message` (not `original.Diagnostic!.Message`) and preserve + `original.Diagnostic` as-is whatever it is (`null` stays `null`) - + removing the need for the guard entirely, so every + `CompositionException` gets the same treatment. Covered by a new + `Compono.Tests` test (`WithSeedInMessage_RewritesMessage_AndLeaves + DiagnosticNull_ForAPlainMessageOriginal`) and a new + `Compono.XunitV3.Tests` test using a hand-written + `CollectionExhaustionPlan` fixture that mirrors `CollectionPlan + .scriban`'s own `HashSet<T>` shape exactly (composing a default-sized + `HashSet<bool>` - `bool`'s value space has only 2 members against a + default collection size of 3 - deterministically exhausts). + 4. The same review round also caught that `ComposeAttributeConcurrencyTests` + never actually exercised concurrent execution at all: + `Enumerable.Range(0, 200).Select(_ => attribute.GetData(...).AsTask())` + is lazy, and since `GetData` runs fully synchronously (an + already-completed `ValueTask`), `Task.WhenAll` enumerating that + deferred sequence never creates call N+1 until call N has already + finished - zero overlap, so the test could never have caught a race + in the shared `Lazy<Composer>`/binding-plan initialization it exists + to test. Fixed by rewriting it with `Parallel.ForEachAsync`, which + genuinely dispatches across the thread pool concurrently - verified + stable across 5 repeated local runs post-fix. - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose<TProfile>]` theory at all - every existing theory used the profile-less `[Compose]` form. @@ -928,10 +957,15 @@ exercised indirectly through `Compono.XunitV3.Tests`. packaging behavior needs re-checking by a person, without being re-run - and re-racing - on every commit. - Full suite green: `Compono.Tests` 392/392 (196 × 2 TFMs - 384 unaffected - + 2 new `CompositionException.WithSeedInMessage` tests, each × 2 TFMs), - `Compono.Generators.Tests` 166/166 (unchanged), `Compono.XunitV3.Tests` - 92/92 (46 × 2 TFMs - unchanged from the first PR #26 round's count, since - `RealRunnerTests` was added and then removed within this same PR). + + 2 new `CompositionException.WithSeedInMessage` tests, each × 2 TFMs - + one of which was rewritten in place during the third review round, net + count unchanged), `Compono.Generators.Tests` 166/166 (unchanged), + `Compono.XunitV3.Tests` 94/94 (47 × 2 TFMs - 46 from the second review + round (unchanged from the first round's count, since `RealRunnerTests` + was added and then removed within this same PR) + 1 new plain-message + `WithSeedInMessage` test from the third round; + `ComposeAttributeConcurrencyTests` rewritten in place to genuinely + exercise concurrency, same test count). `Compono.XunitV3.SampleTests` itself last verified by hand reporting 6 passed/1 deliberately failed (a genuine composition failure) on both net10.0 and net11.0. diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index 1527113..a02dd69 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -296,17 +296,22 @@ private Composer BuildComposer() => Composer.Create(builder => // A genuine composition failure (PR #26 review; ADR-0022 Amendment 5) propagates un-wrapped from // the pipeline otherwise, and CompositionException.Message alone never carries the seed for that - // case (only exception.Diagnostic does, via its own ToString()) - a real test runner's failure - // display shows Message, not Diagnostic.ToString(), so the pasteable-seed promise wasn't actually - // visible there. CompositionException.WithSeedInMessage rewrites Message to include it while - // preserving Diagnostic and the original exception as InnerException unchanged. + // case (only exception.Diagnostic does, via its own ToString(), when Diagnostic is even present - + // a plain-message CompositionException, e.g. a generated HashSet<T>/Dictionary collection plan's + // unique-value-exhaustion path, has no Diagnostic at all and would otherwise escape with no seed + // whatsoever - PR #26 review, third round) - a real test runner's failure display shows Message, + // not Diagnostic.ToString(), so the pasteable-seed promise wasn't actually visible there. + // CompositionException.WithSeedInMessage rewrites Message to include it, handling both shapes + // uniformly, while preserving Diagnostic (null or not) and the original exception as + // InnerException unchanged - so every CompositionException is caught here, not just diagnosed + // ones. private static TResult InvokeWithSeedOnFailure<TResult>(Func<TResult> compose, int seed) { try { return compose(); } - catch (CompositionException exception) when (exception.Diagnostic is not null) + catch (CompositionException exception) { throw CompositionException.WithSeedInMessage(exception, seed); } @@ -318,7 +323,7 @@ private static void InvokeWithSeedOnFailure(Action compose, int seed) { compose(); } - catch (CompositionException exception) when (exception.Diagnostic is not null) + catch (CompositionException exception) { throw CompositionException.WithSeedInMessage(exception, seed); } diff --git a/src/Compono/CompositionException.cs b/src/Compono/CompositionException.cs index 32cd594..4d1ff27 100644 --- a/src/Compono/CompositionException.cs +++ b/src/Compono/CompositionException.cs @@ -99,24 +99,26 @@ internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnost new(diagnostic, innerException) { DiagnosingContextIdentity = diagnosingContextIdentity }; // Internal seam for Compono.XunitV3 (PR #26 review; ADR-0022 Amendment 5) - GetData needs a - // pipeline-propagated composition failure's own Message to carry the row's seed too, not only - // Diagnostic (which already renders a "Seed: <value>" line via its own ToString()). A real xUnit - // v3/MTP test-runner failure display shows Exception.Message, not Diagnostic.ToString(), so the - // seed was invisible there for a genuine composition failure without this. Diagnostic itself is - // preserved unchanged (so Diagnostic.ToString() still renders correctly, without a duplicated - // "Seed:" line) and set as InnerException, never discarded. + // composition failure's own Message to carry the row's seed too, not only Diagnostic (which + // already renders a "Seed: <value>" line via its own ToString(), when it's present at all). A + // real xUnit v3/MTP test-runner failure display shows Exception.Message, not Diagnostic.ToString(), + // so the seed was invisible there for a genuine composition failure without this. Handles both + // shapes uniformly, not just the diagnosed one (PR #26 review, third round): a generated + // HashSet<T>/Dictionary collection plan's unique-value-exhaustion path + // (Compono.Generators/Templates/CollectionPlan.scriban) throws a plain-message + // CompositionException with no Diagnostic at all, and that failure deserves the same pasteable + // seed as any other. original.Diagnostic is preserved exactly as-is either way (null stays null, + // so Diagnostic.ToString() still renders correctly for a diagnosed original, without a duplicated + // "Seed:" line) and the original exception is always set as InnerException, never discarded. internal static CompositionException WithSeedInMessage(CompositionException original, int seed) { ArgumentNullException.ThrowIfNull(original); - if (original.Diagnostic is not { } diagnostic) - throw new ArgumentException("The exception being wrapped must have a Diagnostic.", nameof(original)); - - var message = $"{diagnostic.Message}\n\nSeed: {seed}"; - return new CompositionException(message, diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; + var message = $"{original.Message}\n\nSeed: {seed}"; + return new CompositionException(message, original.Diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; } - private CompositionException(string message, CompositionDiagnostic diagnostic, Exception innerException) + private CompositionException(string message, CompositionDiagnostic? diagnostic, Exception innerException) : base(message, innerException) { Diagnostic = diagnostic; diff --git a/test/Compono.Tests/CompositionExceptionTests.cs b/test/Compono.Tests/CompositionExceptionTests.cs index f6abf2f..be379cb 100644 --- a/test/Compono.Tests/CompositionExceptionTests.cs +++ b/test/Compono.Tests/CompositionExceptionTests.cs @@ -42,13 +42,19 @@ public void WithSeedInMessage_RewritesMessage_ButPreservesDiagnosticAndSetsInner } [Fact] - public void WithSeedInMessage_Throws_WhenTheOriginalExceptionHasNoDiagnostic() + public void WithSeedInMessage_RewritesMessage_AndLeavesDiagnosticNull_ForAPlainMessageOriginal() { + // PR #26 review, third round - a generated HashSet<T>/Dictionary collection plan's + // unique-value-exhaustion path (CollectionPlan.scriban) throws exactly this shape: a plain + // CompositionException(string), no Diagnostic at all. That failure deserves the same + // pasteable seed as a diagnosed one, so WithSeedInMessage must handle it too, not throw. var original = new CompositionException("a plain, non-pipeline-diagnosed message"); - var act = () => CompositionException.WithSeedInMessage(original, seed: 1); + var wrapped = CompositionException.WithSeedInMessage(original, seed: 1); - act.Should().Throw<ArgumentException>(); + wrapped.Message.Should().Be("a plain, non-pipeline-diagnosed message\n\nSeed: 1"); + wrapped.Diagnostic.Should().BeNull(); + wrapped.InnerException.Should().BeSameAs(original); } [Fact] diff --git a/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs index e00b21b..58dd7e5 100644 --- a/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs +++ b/test/Compono.XunitV3.Tests/ComposeAttributeConcurrencyTests.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using Compono.XunitV3.Tests.Fixtures; using Xunit.Sdk; @@ -8,14 +9,24 @@ public sealed class ComposeAttributeConcurrencyTests [Fact] public async Task GetData_ProducesNoExceptionsOrDataRaces_WhenCalledConcurrently_OnOneSharedAttributeInstance() { + // GetData runs fully synchronously and returns an already-completed ValueTask, so awaiting + // Task.WhenAll over a lazily-Select-projected sequence of .AsTask() calls does NOT exercise + // overlapping execution at all - WhenAll enumerates that deferred sequence one element at a + // time, and since each GetData call has already finished by the time .AsTask() returns, call + // N+1 is never even created until call N is done (PR #26 review, third round). + // Parallel.ForEachAsync genuinely dispatches all 200 calls across the thread pool + // concurrently, so this actually exercises concurrent first-touch access to the shared + // Lazy<Composer>/binding-plan initialization ADR-0022's Caching section describes. var attribute = new ComposeAttribute(); var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.Simple))!; + var lengths = new ConcurrentBag<int>(); - var calls = Enumerable.Range(0, 200) - .Select(_ => attribute.GetData(method, new DisposalTracker()).AsTask()); + await Parallel.ForEachAsync(Enumerable.Range(0, 200), async (_, _) => + { + var rows = await attribute.GetData(method, new DisposalTracker()); + lengths.Add(rows.Single().GetData().Length); + }); - var results = await Task.WhenAll(calls); - - results.Should().OnlyContain(rows => rows.Single().GetData().Length == 2); + lengths.Should().HaveCount(200).And.OnlyContain(length => length == 2); } } diff --git a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs index adbddd2..24c37c9 100644 --- a/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs +++ b/test/Compono.XunitV3.Tests/Fixtures/SampleTestMethods.cs @@ -63,6 +63,16 @@ public static void WithUnregisteredInterfaceParameter(IUnregisteredDependency va { } + // Reaches CollectionExhaustionPlan (registered via CollectionPlanCache<HashSet<bool>>.Instance, + // since this test project doesn't reference Compono.Generators as an analyzer) - a plain-message + // CompositionException, no Diagnostic at all, exactly matching what the real generated + // HashSet<T>/Dictionary collection plan throws on unique-value exhaustion + // (CollectionPlan.scriban). Proves GetData appends the seed to this shape too, not only a + // pipeline-diagnosed one (PR #26 review, third round). + public static void WithExhaustedHashSetParameter(HashSet<bool> values) + { + } + public static void WithDisposableParameter(DisposableValue disposable) { } @@ -109,6 +119,30 @@ public sealed class TestProfile : ICompositionProfile public void Configure(CompositionBuilder builder) => builder.Register(() => "from-profile"); } + // Mirrors CollectionPlan.scriban's own HashSet<T> shape exactly (same UniqueValueResolver call, + // same plain-message CompositionException on exhaustion) rather than just throwing directly, + // since this test project doesn't reference Compono.Generators as an analyzer and can't get the + // real generated plan (testing.md's hand-fake convention). Registered by the test itself (not a + // static constructor here - typeof(...)/GetMethod(...) don't trigger a type's static constructor, + // only an actual member access does), matching Compono.Tests' CollectionPlanCacheDispatchTests + // register/try-finally-unregister pattern. + public sealed class CollectionExhaustionPlan : ICompositionPlan<HashSet<bool>> + { + public HashSet<bool> Compose(ICompositionContext context) + { + var size = context.ResolveCollectionSize(); + var result = new HashSet<bool>(size); + + for (var i = 0; i < size; i++) + { + if (!UniqueValueResolver.TryResolve<bool>(context, CompositionRequestKind.CollectionElement, i, Nullability.NotNullable, result, out _)) + throw new CompositionException($"Could not generate {size} unique values of type 'bool' for 'HashSet<bool>' after {UniqueValueResolver.MaxAttempts} attempts per element - the element type's value space is likely too small for the requested collection size."); + } + + return result; + } + } + // Composed via a registration, not a generated plan - this test project doesn't reference // Compono.Generators as an analyzer (testing.md), so a registration is the only way to get a // real (non-fake) composed value for a custom type here. diff --git a/test/Compono.XunitV3.Tests/SeedReportingTests.cs b/test/Compono.XunitV3.Tests/SeedReportingTests.cs index 6227d44..92207da 100644 --- a/test/Compono.XunitV3.Tests/SeedReportingTests.cs +++ b/test/Compono.XunitV3.Tests/SeedReportingTests.cs @@ -64,6 +64,33 @@ public async Task GetData_FailingComposition_DiagnosticIsPreservedUnchanged_Alon exception.InnerException.Should().NotBeNull("the original pipeline exception is preserved, never discarded"); } + [Fact] + public async Task GetData_FailingComposition_MessageContainsTheSeed_ForAPlainMessageExceptionWithNoDiagnostic() + { + // CollectionPlan.scriban's generated HashSet<T>/Dictionary unique-value-exhaustion path throws + // a plain CompositionException(string) - no Diagnostic at all - so this proves the earlier + // catch-filter gap (PR #26 review, third round) is actually closed: every CompositionException + // gets the seed appended, not only a pipeline-diagnosed one. + const int seed = 246810; + var attribute = new ComposeAttribute { Seed = seed }; + var method = typeof(SampleTestMethods).GetMethod(nameof(SampleTestMethods.WithExhaustedHashSetParameter))!; + + CollectionPlanCache<HashSet<bool>>.Instance = new SampleTestMethods.CollectionExhaustionPlan(); + + try + { + var act = () => attribute.GetData(method, new DisposalTracker()).AsTask(); + var exception = (await act.Should().ThrowAsync<CompositionException>()).Which; + + exception.Diagnostic.Should().BeNull(); + exception.Message.Should().Contain($"Seed: {seed}"); + } + finally + { + CollectionPlanCache<HashSet<bool>>.Instance = null; + } + } + [GeneratedRegex(@"Seed: (?<seed>\d+)")] private static partial Regex SeedLine(); } From 9822c803662dffca969b2a7ec97d3eb830d7196b Mon Sep 17 00:00:00 2001 From: Nick Cipollina <ncipollina@gmail.com> Date: Fri, 31 Jul 2026 10:34:26 -0400 Subject: [PATCH 07/10] fix(core): close two P2 gaps from the latest Codex review - PublicApiSurfaceTests only checked IsPublic, which reflection never sets for a nested type (IsNestedPublic is the separate flag) - an accidentally-added nested public type would have slipped through the "exact set" assertion undetected. Now checks both. - GetData's exception doc claimed InnerException was "unchanged" from what the pipeline threw, but WithSeedInMessage constructs a new exception whose InnerException is the pipeline's original CompositionException itself, one level shallower than that original's own InnerException - e.g. a provider failure's real chain is wrapper -> original composition exception -> provider exception. Doc corrected to describe the actual chain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- src/Compono.XunitV3/ComposeAttribute.cs | 12 ++++++++---- test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs | 7 ++++++- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/Compono.XunitV3/ComposeAttribute.cs b/src/Compono.XunitV3/ComposeAttribute.cs index a02dd69..34360da 100644 --- a/src/Compono.XunitV3/ComposeAttribute.cs +++ b/src/Compono.XunitV3/ComposeAttribute.cs @@ -120,10 +120,14 @@ public int Seed /// parameter, more than one Compose-family attribute, or more than one <c>[Shared]</c> parameter /// of the same type); too many inline values were supplied; a supplied inline value is /// <see langword="null"/> for a non-nullable parameter or has a type not assignable to its - /// parameter; or composition itself fails for a parameter - the pipeline's own - /// <see cref="CompositionException"/> propagates, with its <see cref="Exception.Message"/> - /// rewritten to also carry the row's seed (<see cref="CompositionException.Diagnostic"/> and - /// <see cref="Exception.InnerException"/> are otherwise unchanged from what the pipeline threw). + /// parameter; or composition itself fails for a parameter - a new <see cref="CompositionException"/> + /// propagates whose <see cref="Exception.Message"/> is the pipeline's original message with the + /// row's seed appended and whose <see cref="CompositionException.Diagnostic"/> is copied through + /// from that original unchanged (<see langword="null"/> if the original had none). This new + /// exception's own <see cref="Exception.InnerException"/> is the pipeline's original + /// <see cref="CompositionException"/> itself, not that original's <see cref="Exception.InnerException"/> + /// - so a provider failure's chain is wrapper → original composition exception → the provider's + /// own thrown exception, one level deeper than the original throw. /// </exception> /// <remarks> /// <paramref name="disposalTracker"/> is deliberately never used to register a composed value. diff --git a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs index 1fd32b1..5b701fb 100644 --- a/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs +++ b/test/Compono.XunitV3.Tests/PublicApiSurfaceTests.cs @@ -10,8 +10,13 @@ public sealed class PublicApiSurfaceTests [Fact] public void Assembly_ExposesExactlyTheDocumentedPublicTypes() { + // IsPublic alone misses an accidentally-added nested public type (PR #26 review, fourth + // round): a nested type is never IsPublic (only a top-level type can be), it's IsNestedPublic + // instead - reflection's own naming split, not a synonym. Checking only IsPublic would let an + // unapproved `public class Foo { public class Bar { } }` addition slip through this exact-set + // assertion undetected. var publicTypeNames = typeof(ComposeAttribute).Assembly.GetTypes() - .Where(static type => type.IsPublic) + .Where(static type => type.IsPublic || type.IsNestedPublic) .Select(static type => type.FullName); publicTypeNames.Should().BeEquivalentTo( From 531011e48c6874d53a5fdd6c32976d1b8f11a22c Mon Sep 17 00:00:00 2001 From: Nick Cipollina <ncipollina@gmail.com> Date: Fri, 31 Jul 2026 11:34:09 -0400 Subject: [PATCH 08/10] fix(core): drop InternalsVisibleTo grant, make WithSeedInMessage public instead PR #26 review, fifth round (P1): granting Compono.XunitV3 InternalsVisibleTo directly reversed ADR-0021's own accepted, public-only integration boundary. That ADR explicitly considered and rejected this exact option, specifically because Compono is unsigned - the grant authenticates only the assembly name, handing any assembly that can claim that name unrestricted access to every internal member of Compono core, not just WithSeedInMessage. Removed the InternalsVisibleTo("Compono.XunitV3") entry and made WithSeedInMessage public instead. It already lives inside CompositionException, so it already had full access to the private constructor and DiagnosingContextIdentity it needs - the only reason it required InternalsVisibleTo at all was its own access modifier. Compono.XunitV3 now reaches it through an ordinary public API call; every other internal type stays exactly as inaccessible as ADR-0021 intended. Compono.Tests remains the only InternalsVisibleTo grant in Compono.csproj, unchanged from before this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- docs/adr/0022-compono-xunit-package-design.md | 47 ++++++++-- .../0004-milestone-4-xunit-integration.md | 57 ++++++++++-- src/Compono/Compono.csproj | 5 -- src/Compono/CompositionException.cs | 89 ++++++++++--------- 4 files changed, 136 insertions(+), 62 deletions(-) diff --git a/docs/adr/0022-compono-xunit-package-design.md b/docs/adr/0022-compono-xunit-package-design.md index ee73ee5..73d523f 100644 --- a/docs/adr/0022-compono-xunit-package-design.md +++ b/docs/adr/0022-compono-xunit-package-design.md @@ -1068,17 +1068,16 @@ not just in this one sample theory. **Decision:** `ComposeAttribute.GetData` now catches a `CompositionException` propagating from a composition call (`ResolveInvoker`/`ResolveSharedInvoker`/ `ShareExplicitInvoker`, via a new private `InvokeWithSeedOnFailure` helper -wrapping each call site) and rethrows it via a new internal `Compono` core -seam, `CompositionException.WithSeedInMessage(original, seed)` - rewriting -`Message` to `$"{diagnostic.Message}\n\nSeed: {seed}"` while preserving +wrapping each call site) and rethrows it via a new public `Compono` core +method, `CompositionException.WithSeedInMessage(original, seed)` - rewriting +`Message` to `$"{original.Message}\n\nSeed: {seed}"` while preserving `Diagnostic` completely unchanged (so `Diagnostic.ToString()` still renders correctly, without a duplicated `"Seed:"` line) and the original exception -as `InnerException`. This is an `internal` seam, not a new public API - -`Compono.csproj` grants `Compono.XunitV3` `InternalsVisibleTo` for exactly -this, the same pattern already used for `Compono.Tests` - `docs/public --api.md`'s "keep public APIs minimal" goal is unaffected. +as `InnerException`. **Originally shipped as an `internal` seam with a new +`InternalsVisibleTo("Compono.XunitV3")` grant on `Compono.csproj` - reverted +after PR #26 review's fourth round; see the amendment below.** -**Why an internal core seam rather than a workaround entirely inside +**Why a new core method rather than a workaround entirely inside `Compono.XunitV3`:** `CompositionException`'s existing constructors always derive `Message` directly from `Diagnostic.Message` (or accept a plain string with no `Diagnostic`/`InnerException` at all) - there's no existing @@ -1089,7 +1088,7 @@ a seed-appended `Message` field was considered and rejected: `Diagnostic .ToString()`'s own format independently appends `"\n\nSeed: {Seed}"`, so a diagnostic whose `Message` field *also* had the seed appended would render that line twice - a visible regression for the documented `Console -.WriteLine(exception.Diagnostic)` path. The internal seam avoids that +.WriteLine(exception.Diagnostic)` path. `WithSeedInMessage` avoids that entirely by leaving `Diagnostic` untouched and only rewriting the outer exception's own `Message`. @@ -1127,6 +1126,36 @@ clause no longer needs the `Diagnostic is not null` guard at all: every `CompositionException`, diagnosed or not, now gets the same treatment uniformly. +**Amendment to this amendment (PR #26 review, fourth round): the +`InternalsVisibleTo` grant directly reversed ADR-0021's own accepted, +public-only integration boundary.** ADR-0021's Pros and Cons already has +an entry for exactly this option - +"Reaching the engine — `InternalsVisibleTo("Compono.Xunit")`" - rejected +specifically because "it directly reverses a deliberate, documented +policy... adopted specifically to keep a shipped package's internal +surface from becoming a de facto public contract for 'trusted' +consumers." Granting `Compono.XunitV3` `InternalsVisibleTo` for +`WithSeedInMessage` did exactly that: because `Compono` is unsigned, the +grant authenticates only the simple assembly name, handing any assembly +that can claim that name unrestricted access to every internal member of +`Compono` core - not scoped to the one method that needed it. + +**Decision:** the grant is removed. `WithSeedInMessage` itself moved from +`internal` to `public` instead - it already lives inside +`CompositionException`, so it already has full access to the private +constructor and `DiagnosingContextIdentity` it needs; the only reason it +required `InternalsVisibleTo` at all was its own access modifier, not +anything about where it's implemented. Making the one method public, +rather than granting blanket internal access, is the minimal-footprint +fix `docs/public-api.md`'s "keep public APIs minimal" goal actually calls +for: `Compono.XunitV3` gets exactly the operation it needs and nothing +else, `CompositionContext` and every other internal type stay exactly as +inaccessible to `Compono.XunitV3` as ADR-0021 intended, and any other +consumer building custom composition-failure tooling gets the same +utility for free. No `InternalsVisibleTo` entry for `Compono.XunitV3` +exists in `Compono.csproj` after this fix - the only remaining grant is +`Compono.Tests`, unchanged from before this PR. + ## Amendment 6 (2026-07-31): the automated real-runner test is dropped; the sample project stays, unautomated The `RealRunnerTests` test this amendment's own "Consequence" section diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index cfaa0ec..4d00831 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -817,10 +817,9 @@ exercised indirectly through `Compono.XunitV3.Tests`. `[Compose(Seed = -1)]`, which exercises `Compono.XunitV3`'s own seed-validation failure, not a genuine composition failure - the milestone's actual Goal statement is about the latter. Fixed by (a) - adding `CompositionException.WithSeedInMessage` as a new internal - `Compono` core seam (`Compono.XunitV3` granted `InternalsVisibleTo`) so - `ComposeAttribute.GetData` now rewrites a propagating pipeline - failure's own `Message` to include the seed - previously only + adding `CompositionException.WithSeedInMessage` as a new `Compono` + core method so `ComposeAttribute.GetData` now rewrites a propagating + pipeline failure's own `Message` to include the seed - previously only `Diagnostic` had it, and a real test runner's failure display shows `Message`, not `Diagnostic.ToString()`; (b) switching the sample project's failing theory to a genuine unsatisfied composition @@ -833,10 +832,12 @@ exercised indirectly through `Compono.XunitV3.Tests`. rewritten to assert on `.Message` directly (not `.Diagnostic`) for exactly this reason, plus a new test proving `Diagnostic` itself is left unchanged (no duplicated `"Seed:"` line) alongside the rewritten - `Message`. `Compono.Tests` gained direct coverage of the new internal - `CompositionException.WithSeedInMessage` seam itself, per `testing.md`'s - "verifying a new public entry point" rule extended to this internal - one. This fix is real, independently tested, and kept. + `Message`. `Compono.Tests` gained direct coverage of + `CompositionException.WithSeedInMessage` itself, per `testing.md`'s + "verifying a new public entry point" rule. This fix is real, + independently tested, and kept - though its *access mechanism* + changed again in round 5 below, after first shipping as `internal` + plus `InternalsVisibleTo`. 2. `PackToLocalFeed`'s fixed `-p:Version=1.0.0` meant a stale entry in NuGet's global packages folder from an earlier run could silently satisfy a later restore even after the local feed's `.nupkg` contents @@ -874,6 +875,46 @@ exercised indirectly through `Compono.XunitV3.Tests`. to test. Fixed by rewriting it with `Parallel.ForEachAsync`, which genuinely dispatches across the thread pool concurrently - verified stable across 5 repeated local runs post-fix. + 5. A fourth review round found two P2 gaps: `PublicApiSurfaceTests` + checked only `type.IsPublic`, which reflection never sets for a + nested type (`IsNestedPublic` is the separate flag), so an + accidentally-added nested public type would have slipped through + undetected - fixed by checking both. Separately, `GetData`'s XML doc + claimed `InnerException` was "unchanged" from what the pipeline threw, + but `WithSeedInMessage` constructs a *new* exception whose + `InnerException` is the pipeline's original `CompositionException` + itself (one level shallower than that original's own + `InnerException`) - e.g. a provider failure's real chain is wrapper + → original composition exception → the provider's own thrown + exception. Doc corrected to describe the actual chain. + 6. **A fifth review round caught something serious: the `internal` + `WithSeedInMessage` seam from round 2, reached via a new + `InternalsVisibleTo("Compono.XunitV3")` grant on `Compono.csproj`, + directly reversed [ADR-0021](../adr/0021-row-composition-entry-point-for-test-framework-integrations.md)'s + own accepted, public-only integration boundary** - that ADR's own + Pros and Cons section has an entry for exactly this option, rejected + specifically because "it directly reverses a deliberate, documented + policy... adopted specifically to keep a shipped package's internal + surface from becoming a de facto public contract for 'trusted' + consumers." Because `Compono` is unsigned, the grant authenticated + only the assembly name - handing any assembly that could claim that + name unrestricted access to every internal member of `Compono` core, + not scoped to the one method that needed it. This should have been + caught before implementation (a quick check against ADR-0021 before + adding any `InternalsVisibleTo` entry would have surfaced the direct + conflict immediately) rather than by a later review round. **Fixed by + removing the grant entirely and making `WithSeedInMessage` itself + `public` instead of `internal`** - it already lives inside + `CompositionException`, so it already had full access to the private + constructor and `DiagnosingContextIdentity` it needs; the only reason + it ever required `InternalsVisibleTo` was its own access modifier, not + anything about where it's implemented. `Compono.XunitV3` now gets + exactly the one operation it needs via an ordinary public API call, + `CompositionContext` and every other internal type stay exactly as + inaccessible as ADR-0021 intended, and the only `InternalsVisibleTo` + entry remaining in `Compono.csproj` is `Compono.Tests`, unchanged from + before this PR. See ADR-0022 Amendment 5's own "amendment to this + amendment" for the full account. - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose<TProfile>]` theory at all - every existing theory used the profile-less `[Compose]` form. diff --git a/src/Compono/Compono.csproj b/src/Compono/Compono.csproj index 89e94e5..582667a 100644 --- a/src/Compono/Compono.csproj +++ b/src/Compono/Compono.csproj @@ -35,11 +35,6 @@ already how GeneratorTestHelpers.CompileAndExecute invokes every test's entry point, so this adds no new public/friend surface to the shipped package. --> <InternalsVisibleTo Include="Compono.Tests" /> - <!-- CompositionException.WithSeedInMessage (PR #26 review; ADR-0022 Amendment 5) - lets - Compono.XunitV3.ComposeAttribute.GetData wrap a propagating pipeline composition failure - so its own Message carries the row's seed. A narrow internal seam, not a new public API - - docs/public-api.md's "keep public APIs minimal" goal. --> - <InternalsVisibleTo Include="Compono.XunitV3" /> </ItemGroup> </Project> diff --git a/src/Compono/CompositionException.cs b/src/Compono/CompositionException.cs index 4d1ff27..c9a6aa5 100644 --- a/src/Compono/CompositionException.cs +++ b/src/Compono/CompositionException.cs @@ -21,18 +21,6 @@ public sealed class CompositionException : Exception /// </summary> public CompositionDiagnostic? Diagnostic { get; } - // Identifies which CompositionContext instance's own BuildException produced this exception - an - // opaque identity token (CompositionContext.Identity), not the context itself (PR #17 review): - // storing the actual CompositionContext would keep its whole object graph alive for as long as a - // consumer or test runner retains this exception - its configured IServiceProvider, registration - // factory closures, scope-held shared values, and trace buffer, none of which this comparison - // needs. A bare bool couldn't distinguish "some context produced this" from "this context's active - // nested resolution produced it" either (a registration factory can capture and invoke an entirely - // different Composer and let that composer's own pipeline-diagnosed exception escape, or rethrow - // one it captured earlier) - InvokeFactory's catch guard (docs/adr/0010) needs identity, just not - // by holding the whole context to get it. - internal object? DiagnosingContextIdentity { get; private init; } - /// <summary> /// Creates a <see cref="CompositionException"/> with no structured <see cref="Diagnostic"/>. /// </summary> @@ -73,22 +61,47 @@ public CompositionException(CompositionDiagnostic diagnostic, Exception innerExc Diagnostic = diagnostic; } - // The base(...) initializer runs before this constructor's own body, so a guard clause in the - // body would already be too late - diagnostic.Message is dereferenced in the initializer itself. - // Routing the null check through this helper is what lets a null argument surface as - // ArgumentNullException instead of a base-initializer NullReferenceException. - private static CompositionDiagnostic RequireDiagnostic(CompositionDiagnostic diagnostic) + /// <summary> + /// Creates a copy of <paramref name="original"/> whose <see cref="Exception.Message"/> has a + /// <c>"Seed: <value>"</c> line appended, so a consumer building custom composition-failure + /// tooling (e.g. a test-framework integration reporting a reproducible seed) can surface it + /// without needing <see cref="Diagnostic"/> to be present - <see cref="Diagnostic"/> already + /// renders its own <c>"Seed:"</c> line via <see cref="CompositionDiagnostic.ToString"/> when it's + /// there, but not every <see cref="CompositionException"/> has one (a plain + /// <see cref="CompositionException(string)"/>, e.g. a generated collection plan's + /// unique-value-exhaustion failure, has none). + /// </summary> + /// <param name="original">The exception to copy and append the seed to.</param> + /// <param name="seed">The seed value to append.</param> + /// <returns> + /// A new <see cref="CompositionException"/> whose <see cref="Diagnostic"/> is + /// <paramref name="original"/>'s, unchanged (<see langword="null"/> stays <see langword="null"/>), + /// and whose <see cref="Exception.InnerException"/> is <paramref name="original"/> itself - so a + /// provider failure's full chain becomes this new exception, then <paramref name="original"/>, + /// then whatever <paramref name="original"/>'s own <see cref="Exception.InnerException"/> was (if + /// any), one level deeper than the original throw. + /// </returns> + /// <exception cref="ArgumentNullException"><paramref name="original"/> is <see langword="null"/>.</exception> + public static CompositionException WithSeedInMessage(CompositionException original, int seed) { - ArgumentNullException.ThrowIfNull(diagnostic); - return diagnostic; - } + ArgumentNullException.ThrowIfNull(original); - private static Exception RequireInnerException(Exception innerException) - { - ArgumentNullException.ThrowIfNull(innerException); - return innerException; + var message = $"{original.Message}\n\nSeed: {seed}"; + return new CompositionException(message, original.Diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; } + // Identifies which CompositionContext instance's own BuildException produced this exception - an + // opaque identity token (CompositionContext.Identity), not the context itself (PR #17 review): + // storing the actual CompositionContext would keep its whole object graph alive for as long as a + // consumer or test runner retains this exception - its configured IServiceProvider, registration + // factory closures, scope-held shared values, and trace buffer, none of which this comparison + // needs. A bare bool couldn't distinguish "some context produced this" from "this context's active + // nested resolution produced it" either (a registration factory can capture and invoke an entirely + // different Composer and let that composer's own pipeline-diagnosed exception escape, or rethrow + // one it captured earlier) - InvokeFactory's catch guard (docs/adr/0010) needs identity, just not + // by holding the whole context to get it. + internal object? DiagnosingContextIdentity { get; private init; } + // The only construction path that sets DiagnosingContextIdentity - used exclusively by // CompositionContext.BuildException (passing its own Identity token), never by a factory/rule that // constructs a CompositionException itself. @@ -98,24 +111,20 @@ internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnost internal static CompositionException CreatePipelineDiagnosed(CompositionDiagnostic diagnostic, Exception innerException, object diagnosingContextIdentity) => new(diagnostic, innerException) { DiagnosingContextIdentity = diagnosingContextIdentity }; - // Internal seam for Compono.XunitV3 (PR #26 review; ADR-0022 Amendment 5) - GetData needs a - // composition failure's own Message to carry the row's seed too, not only Diagnostic (which - // already renders a "Seed: <value>" line via its own ToString(), when it's present at all). A - // real xUnit v3/MTP test-runner failure display shows Exception.Message, not Diagnostic.ToString(), - // so the seed was invisible there for a genuine composition failure without this. Handles both - // shapes uniformly, not just the diagnosed one (PR #26 review, third round): a generated - // HashSet<T>/Dictionary collection plan's unique-value-exhaustion path - // (Compono.Generators/Templates/CollectionPlan.scriban) throws a plain-message - // CompositionException with no Diagnostic at all, and that failure deserves the same pasteable - // seed as any other. original.Diagnostic is preserved exactly as-is either way (null stays null, - // so Diagnostic.ToString() still renders correctly for a diagnosed original, without a duplicated - // "Seed:" line) and the original exception is always set as InnerException, never discarded. - internal static CompositionException WithSeedInMessage(CompositionException original, int seed) + // The base(...) initializer runs before this constructor's own body, so a guard clause in the + // body would already be too late - diagnostic.Message is dereferenced in the initializer itself. + // Routing the null check through this helper is what lets a null argument surface as + // ArgumentNullException instead of a base-initializer NullReferenceException. + private static CompositionDiagnostic RequireDiagnostic(CompositionDiagnostic diagnostic) { - ArgumentNullException.ThrowIfNull(original); + ArgumentNullException.ThrowIfNull(diagnostic); + return diagnostic; + } - var message = $"{original.Message}\n\nSeed: {seed}"; - return new CompositionException(message, original.Diagnostic, original) { DiagnosingContextIdentity = original.DiagnosingContextIdentity }; + private static Exception RequireInnerException(Exception innerException) + { + ArgumentNullException.ThrowIfNull(innerException); + return innerException; } private CompositionException(string message, CompositionDiagnostic? diagnostic, Exception innerException) From 6c1ebf79098d52b467c4788cc84c12d565b407ac Mon Sep 17 00:00:00 2001 From: Nick Cipollina <ncipollina@gmail.com> Date: Fri, 31 Jul 2026 11:45:42 -0400 Subject: [PATCH 09/10] fix(core): bound the pack-lock wait so an abandoned lock can't hang forever PR #26 review, sixth round (P2): if a prior holder of pack-to-local-feed.sh's lock is killed before its own EXIT trap can run (SIGKILL, a canceled build, a machine restart), the lock directory is never cleaned up, and every subsequent restore of the sample project would wait on it forever with no way to recover short of knowing to delete it by hand. Added a bounded wait (~120 attempts) that fails with a clear, actionable message naming the exact directory to remove, instead of looping indefinitely. Verified by manually creating a stale lock and confirming the script now fails fast with that message (using a temporarily-reduced attempt count), then confirming a normal run still succeeds once the stale lock is cleared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- .../plans/0004-milestone-4-xunit-integration.md | 14 ++++++++++++++ .../pack-to-local-feed.sh | 17 +++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index 4d00831..0cc7de8 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -915,6 +915,20 @@ exercised indirectly through `Compono.XunitV3.Tests`. entry remaining in `Compono.csproj` is `Compono.Tests`, unchanged from before this PR. See ADR-0022 Amendment 5's own "amendment to this amendment" for the full account. + 7. A sixth review round (P2) caught that `pack-to-local-feed.sh`'s + `until mkdir "$lock_dir"; do sleep 1; done` wait loop had no bound - + if a prior holder were killed before its own `EXIT` trap could run + (`SIGKILL`, a canceled build, a machine restart), the lock directory + is never cleaned up, and every later restore of this project would + wait on it forever with no way to recover short of knowing to delete + it by hand. Fixed with a bounded wait (~120 attempts, matching the + ~1 pack/second polling interval already in place) that fails with a + clear, actionable message (naming the exact `rm -rf` to run) instead + of hanging indefinitely. Verified by manually creating a stale lock + directory and confirming the script now fails fast with that message + (using a temporarily-reduced attempt count for the test) rather than + blocking forever, then confirming a normal run still succeeds once + the stale lock is gone. - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose<TProfile>]` theory at all - every existing theory used the profile-less `[Compose]` form. diff --git a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh index fc545c1..3bf6f8b 100755 --- a/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh +++ b/test/Compono.XunitV3.SampleTests/pack-to-local-feed.sh @@ -38,7 +38,24 @@ restore_packages_path="$5" lock_dir="$feed_dir/.pack.lock" +# Bounded wait, not an unconditional retry loop (PR #26 review, sixth round) - if a prior holder was +# killed before its own EXIT trap could run (SIGKILL, a canceled build, a machine restart), the lock +# directory is never cleaned up, and every subsequent restore of this project would otherwise wait on +# it forever. A real pack of both projects normally takes well under a minute, so 120 attempts (~2 +# minutes) is generous headroom for a genuinely concurrent sibling while still failing fast, with +# actionable remediation, on a truly abandoned lock rather than hanging indefinitely. +max_wait_attempts=120 +attempt=0 + until mkdir "$lock_dir" 2>/dev/null; do + attempt=$((attempt + 1)) + if [ "$attempt" -ge "$max_wait_attempts" ]; then + echo "pack-to-local-feed.sh: timed out after ${max_wait_attempts}s waiting for the lock at '$lock_dir'." >&2 + echo "This usually means a previous run was killed before it could release the lock. If no other" >&2 + echo "'dotnet pack'/'dotnet test' against this project is currently running, remove it manually and" >&2 + echo "retry: rm -rf '$lock_dir'" >&2 + exit 1 + fi sleep 1 done trap 'rmdir "$lock_dir"' EXIT From 1c7c059d15ed999672a7e0b1d5bee3d5874fa7f0 Mon Sep 17 00:00:00 2001 From: Nick Cipollina <ncipollina@gmail.com> Date: Fri, 31 Jul 2026 11:53:13 -0400 Subject: [PATCH 10/10] docs(core): document CompositionException.WithSeedInMessage in public-api.md PR #26 review, seventh round (P2): a newly public core API was documented only in ADR-0022, deferred to Phase 4's docs pass. Per this repo's own rule (update the relevant docs/*.md in the same PR that changes the behavior it describes), added coverage to the Diagnostics API section instead of leaving it undocumented until a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --- .../0004-milestone-4-xunit-integration.md | 11 ++++++++ docs/public-api.md | 27 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/docs/plans/0004-milestone-4-xunit-integration.md b/docs/plans/0004-milestone-4-xunit-integration.md index 0cc7de8..b6d4580 100644 --- a/docs/plans/0004-milestone-4-xunit-integration.md +++ b/docs/plans/0004-milestone-4-xunit-integration.md @@ -929,6 +929,17 @@ exercised indirectly through `Compono.XunitV3.Tests`. (using a temporarily-reduced attempt count for the test) rather than blocking forever, then confirming a normal run still succeeds once the stale lock is gone. + 8. A seventh review round (P2) caught that `CompositionException + .WithSeedInMessage` - a genuinely new public `Compono` core API as + of round 5 - was documented only in ADR-0022, with its + `docs/public-api.md` coverage deferred to Phase 4's docs pass. Per + this repo's own rule (`AGENTS.md`: update the relevant `docs/*.md` in + the same PR that changes the behavior it describes, not a + follow-up), added a `Diagnostics API` section entry showing the + method's signature, its exception-chain contract (`Diagnostic` + copied through unchanged, `original` becomes `InnerException`), and + when to reach for it (a plain-message `CompositionException` has no + `Diagnostic` to render its own `Seed:` line). - Also folded into this round, at the user's request (not a posted PR comment): the sample project had no `[Compose<TProfile>]` theory at all - every existing theory used the profile-less `[Compose]` form. diff --git a/docs/public-api.md b/docs/public-api.md index c7ffd52..dad363c 100644 --- a/docs/public-api.md +++ b/docs/public-api.md @@ -447,6 +447,33 @@ catch (CompositionException exception) } ``` +`exception.Diagnostic` (when present) already renders its own `Seed: {value}` +line via `ToString()`, but not every `CompositionException` has a +`Diagnostic` - a plain-message one (e.g. a generated `HashSet<T>`/ +`Dictionary` collection plan's unique-value-exhaustion failure) does not. +`CompositionException.WithSeedInMessage(original, seed)` is a static +factory for exactly this case - it returns a copy of `original` whose +`Message` has the seed appended directly, regardless of whether +`Diagnostic` is present: + +```csharp +try +{ + composer.Create<Order>(); +} +catch (CompositionException exception) +{ + throw CompositionException.WithSeedInMessage(exception, mySeed); +} +``` + +The returned exception's `Diagnostic` is copied through from `original` +unchanged (`null` stays `null`), and `original` itself becomes its +`InnerException` - never discarded. `Compono.XunitV3`'s own `[Compose]` +binding algorithm (ADR-0022) uses this to guarantee every composition +failure's `Message` carries a pasteable seed, not only ones that happen to +have a `Diagnostic`. + Potential debugging API: ```csharp