Uh oh!
There was an error while loading. Please reload this page.
feat(mstest): add Compono.MSTest package (ADR-0057) - #120
Conversation
…est; accept Compono.MSTest (ADR-0057) - RESEARCH-0015: disposal/ownership semantics — Outcome C, post-1.0 additive feature, no 1.0 API change required. - RESEARCH-0016: async composition viability — Outcome C, fully additive post-1.0 capability. - RESEARCH-0017: MSTest integration viability — Outcome A, add Compono.MSTest before 1.0 (includes VSTest/MTP discovery-execution spike findings and MSTest version-floor capability matrix). - ADR-0057: Compono.MSTest package design, Accepted — ITestDataSource-based [Compose] integration reusing CompositionRow/RowInvokerRegistry, no new composition engine. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the Compono.MSTest integration package: [TestMethod] + [Compose]/ [Compose<TProfile>]/[Compose<TProfile, TConfig>] as ITestDataSource attributes, [Shared] parameter sharing, and generator discovery support, matching Compono.XunitV3/Compono.TUnit's existing attribute family and semantics. Implementation evidence forced two ADR-0057 amendments: MSTest.TestFramework 3.x and 4.x ship under different, binary-incompatible assembly identities, so the supported floor is 4.0.0, not 3.0.0 (Amendment 1); and GetDisplayName is a discovery/listing-time MSTest hook, not visible during ordinary test execution under either MTP or the classic VSTest adapter (Amendment 2). A related Native AOT/trimming gap in Compono.XunitV3's own ConfigProfileBinder was found incidentally (ADR-0041 Amendment 4) and is tracked separately as #119, out of scope here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:bed19c48ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…and-adr-0057 # Conflicts: # skills/compono-evals/evals.json
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
ncipollina
left a comment
There was a problem hiding this comment.
PR #120 review — ADR/PLAN-0057 Compono.MSTest
Reviewed PR head b44fc6912dc48d4e17dee3cb34a01e1e524f1c0c against base 481854f704287547dd6be4cedcd7634ef3cc58b6 using the engineering-workflow PR review process.
Verdict
Request changes: no remaining product/architecture blockers found in the MSTest implementation itself, but the PR's current GitHub package-validation check is failing and must be green before merge.
Findings
BLOCKER
None.
HIGH
- Failing package-validation CI at the current PR head
- Evidence:
gh pr checks 120reportspackage-validationfailed for run33651896016, job100320715892. - Failed step:
Inspect packed .nupkg contents. - Log excerpt:
FAIL: Compono.TUnit's .nuspec dependency on TUnit.Core is '[1.65.63, 2.0.0)', expected the intended tested range '[1.65.38, 2.0.0)'. - Impact: The package-readiness gate is red. Even though the failing package is
Compono.TUnit, the current PR head cannot be accepted/merged with package validation failing. - Recommended correction: Either make the packed
Compono.TUnitnuspec emit the intended[1.65.38, 2.0.0)range again, or update.github/scripts/inspect-packed-nupkgs.sh/the relevant ADR-plan documentation if[1.65.63, 2.0.0)is now the true tested lower bound. Re-run package-validation to green on this PR head.
- Evidence:
MEDIUM
None found in the current PR head. Previously identified gaps for MSTest 4.0.0 floor evidence, GetDisplayName documentation, [DynamicData] executable coverage, and external packaged-consumer validation evidence appear addressed in the current plan/docs/tests.
LOW
- Stale test comment references removed GetData logging hook
- Location:
test/Compono.MSTest.Tests/RealRunnerRowIdentityTests.cs, class cleanup comment. - Evidence: The comment says GetData counts were captured via
COMPONO_MSTEST_GETDATA_LOGand says “see ComposeAttribute.cs”; currentsrc/Compono.MSTest/ComposeAttribute.csonly incrementsGetDataCallCountand has no environment-variable logging hook. - Impact: Documentation-only drift in a test comment; it can mislead the next reviewer/debugger but does not affect runtime behavior.
- Recommended correction: Reword the comment to say the log hook was temporary/implementation-time evidence recorded in PLAN-0057, or remove the
see ComposeAttribute.csclaim.
- Location:
Local verification performed
git status --shortbefore review-file creation: clean.dotnet build --no-restore: passed locally, 0 warnings/0 errors.dotnet test --no-build: started; the harness timed it out after MSTest and many other projects passed, so I do not count this as a full-suite pass.dotnet test test/Compono.MSTest.SampleTests/Compono.MSTest.SampleTests.csproj --no-restore --no-build -f net10.0: passed, 5/5.- GitHub checks inspected with
gh pr checks 120; package-validation failed as above.
Notes
- No implementation files were modified as part of this review.
- This summary file is the requested local durable review artifact.
ncipollina
commented
Sep 2, 2026
CI update after my review comment: |
…tch Directory.Packages.props Dependabot PR #121 (merged to main after this branch's last merge) bumped TUnit.Core/TUnit from 1.65.38 to 1.65.63 in Directory.Packages.props. This script hardcodes the expected packed-nuspec dependency range as a literal string per package, which dependabot has no way to know to update - main itself independently fails this same assertion as of #121, unrelated to PLAN-0057's own scope. Smallest correct fix: update the literal to match the now-current floor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GMQEjFrUVGYkctE3ECfRuA
ncipollina
commented
Sep 2, 2026
Re: #120 (comment) — Root cause confirmed for both:
All checks ( |
ncipollina
left a comment
There was a problem hiding this comment.
🎉 Follow-up review of the corrective delta from b44fc691 to 5cfe446: the prior CI blockers are resolved. The TUnit.Core package-validation mismatch was a stale expected range after main/Dependabot moved Directory.Packages.props to [1.65.63, 2.0.0), and the validation script now matches the packed nuspec; current package-validation is green. Current build / build is also green, including the Test Code step. The earlier Codex documentation findings remain corrected, all review threads are resolved, and I found no new merge-blocking issues in the corrective delta. Recommendation: READY TO MERGE.
🚀 Pull Request
📋 Summary
Adds
Compono.MSTest, the third test-framework integration package alongsideCompono.XunitV3/Compono.TUnit:[TestMethod]+[Compose]/[Compose<TProfile>]/[Compose<TProfile, TConfig>]asITestDataSourceattributes,[Shared]parameter sharing, and generator discovery — the same Compono-facing attribute family and semantics as the existing packages, adapted to MSTest's own synchronous, non-owningITestDataSourcecontract. Implements ADR-0057 / PLAN-0057, both nowAccepted/Done.Two real findings during implementation forced ADR amendments rather than being papered over:
MSTest.TestFramework3.x and 4.x ship under different, binary-incompatible assembly identities, so the supported floor is 4.0.0, not 3.0.0 (ADR-0057 Amendment 1); andGetDisplayNameis a discovery/listing-time MSTest hook only, never called during ordinary test execution under either runner (ADR-0057 Amendment 2). A related, pre-existing Native AOT/trimming gap was found incidentally inCompono.XunitV3's ownConfigProfileBinder— tracked separately as #119, explicitly out of scope here (noCompono.XunitV3source touched).📝 Changes
Compono.MSTestpackage (src/Compono.MSTest)ComposeAttribute : Attribute, ITestDataSource,ComposeAttribute<TProfile>,ComposeAttribute<TProfile, TConfig>,SharedAttribute— the frozen public API from ADR-0057 §6.Binding/(BindingPlan,ParameterBindingPlan,PositionalArgumentBinder,ConfigProfileBinder,RowInvokers), ported fromCompono.XunitV3's pattern (MSTest hands the sameMethodInfo/ParameterInfoshape xUnit v3 does).RowInvokersdispatches through the existing, unchanged coreRowInvokerRegistry/CompositionRowfrom its first commit — no throwaway reflection-based dispatch.GetData/GetDisplayNameshare no MSTest-provided context object, aConditionalWeakTable<object?[], object>keyed by the exact row-array instance carries the seed across the two calls — verified correct under both runners' discovery/listing output.MSTest.TestFrameworkonly (not theMSTestumbrella package), floor4.0.0(ADR-0057 Amendment 1).ConfigProfileBinder'sTConfig/TProfileconstruction needed the sameDynamicallyAccessedMembers(PublicConstructors)fixCompono.TUnitalready required (ADR-0041 Amendment 1) — found via a realdotnet publish -p:PublishAot=truefailure, fixed, and re-verified with zeroCompono.MSTest-attributable trim warnings.Generator discovery (
src/Compono.Generators)Compono.MSTest.ComposeAttribute/`1/`2) feeding the existing, already attribute-family-agnosticComposeMethodDiscovery— additive only, 299/299 existing generator tests unaffected. New snapshot test proves a type reachable only through aCompono.MSTest-attributed parameter gets a generated plan +RowInvokerRegistryregistration.Tests
test/Compono.MSTest.Tests— binding/signature validation, inline values,[Shared], profile/config binding, negative-seed ordering,[DataRow]and[DynamicData]+[Compose]independent-row coexistence, a real-runner row-identity proof (RealRunnerRowIdentityTests), a reflection-pattern source guard, and the public-API-surface lock.test/Compono.MSTest.SampleTests— the full attribute family through the real packagedCompono.MSTest → Componodependency chain (neverProjectReference), includingCompono.NSubstituteintegration.test/Compono.MSTest.AotSmokeTest— realdotnet publish -p:PublishAot=true+ run against the packaged dependency chain.Docs/skill/eval — new
docs/packages/compono-mstest.md,skills/compono/references/mstest.md;README.md/docs/index.md/docs/packages/index.md/docs/public-api.md/docs/concepts/shared-values.md/docs/getting-started/installation.md/docs/roadmap/future-packages.mdsynced (also caughtdocs/index.mdalready missingCompono.Http/Compono.Logging, fixed alongside); regenerateddocs/reference/api/Compono.MSTest/; two new evals + a scored benchmark inskills/compono-evals/.CI/packaging —
docs.yml,package-validation.yaml,inspect-packed-nupkgs.sh,generate-api-reference.shall updated and exercised for real (a fresh pack of all ten publishable packages passed everyinspect-packed-nupkgs.shassertion forCompono.MSTest, including confirming no embeddedCompono.Generators.dll).External packaged-consumer validation — a purpose-built, disposable external fixture (not a real application, not retained) validated via
scripts/dogfood-validate.shwith no script changes needed. This is explicitly consumer validation, not genuine dogfooding — no real MSTest consumer exists in any LayeredCraft/ncipollina repo yet. Full reproducible spec (exact layout, csproj, command, resolved versions, result) is recorded in PLAN-0057's Notes.🧪 Validation
dotnet build Compono.slnx— 0 errors.dotnet test Compono.slnx— 964/964 passing, zero regressions in any existing package.project.assets.jsoninspection (not requested versions):MSTestfamily4.0.0(floor)MSTestfamily4.3.3(current)dotnet publish -c Release -p:PublishAot=true -r osx-arm64 --self-contained true+ run, both[Compose]and[Compose<TProfile, TConfig>], zeroCompono.MSTest-attributable trim warnings (-p:TrimmerSingleWarn=falseconfirmed).Compono.Generators.Tests299/299 (298 existing + 1 new snapshot).test/Compono.MSTest.SampleTests5/5 through the real NuGet dependency chain, both runners.scripts/dogfood-validate.sh— PASS, 9/9, exit 0, fixture git tree byte-identical before/after.4.3.3-resolved dependencies, not a genuine4.0.0leg) and three MEDIUM findings, all resolved with real re-verification (see PLAN-0057 Notes' "Adversarial review response" entry for the full account) rather than narrowed claims.🧩 Related Issues
Relates to #119 (
Compono.XunitV3.ConfigProfileBinder's own Native AOT gap, found incidentally, tracked and scoped separately — noCompono.XunitV3source changed here).📦 Release Notes
Adds
Compono.MSTest(prerelease,MSTest.TestFramework4.0.0+ required) alongsideCompono.XunitV3/Compono.TUnit.💬 Notes for Reviewers
GetDisplayNamediscovery-only timing) — both are architectural corrections, not just doc tweaks, and are the most load-bearing part of this diff to review.Compono.XunitV3behavior changes.test/Compono.MSTest.SampleTests/test/Compono.MSTest.AotSmokeTestare deliberately not inCompono.slnx, matching the existingCompono.TUnit.SampleTests/Compono.TUnit.AotSmokeTestconvention (manual/local-feed-driven proofs).