fix(mocks): let one type be mocked regularly and wrapped in one compilation - #6835
Conversation
…lation
`Mock.Of<T>()` and `Mock.Wrap(instance)` produce two models for the same type
that differ only in `IsWrapMock`. Model equality includes that flag, so both
survive dedup — correctly, since each needs its own impl and factory — but both
emission paths then added the same hint names. A duplicate hint name aborts the
generator, so every mock in the compilation disappeared and the build failed
with a cascade of CS1061 errors, the cause visible only in a CS8785 warning.
The wrap impl and factory are already types of their own and are file-scoped, so
they just need a hint name of their own: they now emit as
`{name}_WrapMockImplFactory.g.cs`.
The setup and verification surface is not per mode. It describes the mocked
type, is byte-identical between the two models, and lives in an extension class
that is not file-scoped, so it has to be emitted exactly once. A new
`SharedMemberSurfaceResolver` step assigns that ownership across the collected
requests: the regular model emits it, since it also emits the static `Mock()`
entry point, and a wrap model emits it only when it is the type's only model.
The identity used to recognise one target reached in several modes is shared
with `GeneratedNameCollisionDetector`, which already had to tell that case apart
from a genuine #6505 name collision.
Four wrap snapshots are re-recorded: renaming the wrap hint changes where that
file sorts in the concatenated snapshot. The generated code is unchanged.
Fixes #6834
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe source generator now supports regular and wrap mocks for the same type. It assigns distinct implementation and factory hint names, emits shared member and event surfaces once, and adds generator and runtime regression coverage. ChangesDual-mode mock generation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant TestSource
participant MockGenerator
participant MockRegistry
participant MockEngine
TestSource->>MockGenerator: Generate regular and wrap mocks
MockGenerator->>MockRegistry: Register regular factory
MockGenerator->>MockRegistry: Register wrap factory
TestSource->>MockEngine: Configure shared setup
MockEngine-->>TestSource: Handle regular and wrapped calls
Merge Risk: ⚪ Minimal · up to The dual-mode mock generation change has no identified actionable risk and is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 7 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the mock files bright Comment |
Code reviewReviewed the fix for #6834 (duplicate hint name when a type is reached through both Summary of the change
This is a clean, well-targeted fix. It correctly recognizes that the wrap impl/factory are already One maintainability nit (not a live bug today):
|
|
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.68.4 to 1.69.0. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.69.0 <!-- Release notes generated using configuration in .github/release.yml at v1.69.0 --> ## What's Changed ### Other Changes * feat(templates): add enableDotCover flag (#6714) by @ForNeVeR in thomhurst/TUnit#6844 * fix: don't request semantic models for attribute syntax from other compilations (DevKit crash) by @thomhurst in thomhurst/TUnit#6855 * fix(ci): restore net472 PublicAPI tests on Windows by @thomhurst in thomhurst/TUnit#6857 * perf(html-report): stream report JSON through pooled chunks and overlap sidecar serialization by @thomhurst in thomhurst/TUnit#6860 * chore(renovate): cap Microsoft.Build packages below 18.10.0 by @thomhurst in thomhurst/TUnit#6863 * perf: shrink generated per-class test source static constructors (~40% less startup JIT) by @thomhurst in thomhurst/TUnit#6859 * refactor: remove unreachable decimal source-text path from GenerateAttributeInstantiation by @thomhurst in thomhurst/TUnit#6856 * perf: cut per-test allocations in discovery and execution (-61% at 10k tests) by @thomhurst in thomhurst/TUnit#6861 * perf: stop hashing per-test event receivers during registration (data-driven tests 2.9x faster at 10k) by @thomhurst in thomhurst/TUnit#6858 * perf(analyzers): cut TUnit analyzer build time ~60% on large test projects by @thomhurst in thomhurst/TUnit#6862 ### Dependencies * chore(deps): update opentelemetry to 1.19.0 by @thomhurst in thomhurst/TUnit#6838 * chore(deps): update dependency opentelemetry.instrumentation.runtime to 1.19.0 by @thomhurst in thomhurst/TUnit#6840 * chore(deps): update tunit to 1.68.17 by @thomhurst in thomhurst/TUnit#6839 * chore(deps): update verify to 33.1.0 by @thomhurst in thomhurst/TUnit#6843 * chore(deps): update verify to 33.1.1 by @thomhurst in thomhurst/TUnit#6847 * chore(deps): update opentelemetry to 1.19.1 by @thomhurst in thomhurst/TUnit#6850 * chore(deps): update dependency grpc.core.api to 2.84.0 by @thomhurst in thomhurst/TUnit#6851 * chore(deps): update dependency stackexchange.redis to 3.3.1 by @thomhurst in thomhurst/TUnit#6853 * chore(deps): update dependency polyfill to 11.4.0 by @thomhurst in thomhurst/TUnit#6841 * chore(deps): update dependency polyfill to 11.4.0 by @thomhurst in thomhurst/TUnit#6842 ## New Contributors * @ForNeVeR made their first contribution in thomhurst/TUnit#6844 **Full Changelog**: thomhurst/TUnit@v1.68.17...v1.69.0 ## 1.68.17 <!-- Release notes generated using configuration in .github/release.yml at v1.68.17 --> ## What's Changed ### Other Changes * fix(mocks): emit init accessors for init-only properties and indexers by @thomhurst in thomhurst/TUnit#6833 * fix(mocks): let one type be mocked regularly and wrapped in one compilation by @thomhurst in thomhurst/TUnit#6835 * fix(mocks): keep editors in sync with publicized project references (#6836) by @thomhurst in thomhurst/TUnit#6837 ### Dependencies * chore(deps): update tunit to 1.68.4 by @thomhurst in thomhurst/TUnit#6824 * chore(deps): update mstest to 4.4.1 by @thomhurst in thomhurst/TUnit#6825 * chore(deps): update microsoft.testing by @thomhurst in thomhurst/TUnit#6717 * chore(deps): update verify to v33 by @thomhurst in thomhurst/TUnit#6794 * chore(deps): update dependency stackexchange.redis to 3.2.15 by @thomhurst in thomhurst/TUnit#6827 * chore(deps): update dependency messagepack to 3.1.9 by @thomhurst in thomhurst/TUnit#6828 * chore(deps): update dependency stackexchange.redis to 3.3.0 by @thomhurst in thomhurst/TUnit#6831 * chore(deps): update opentelemetry to 1.19.0 by @thomhurst in thomhurst/TUnit#6832 **Full Changelog**: thomhurst/TUnit@v1.68.4...v1.68.17 Commits viewable in [compare view](thomhurst/TUnit@v1.68.4...v1.69.0). </details> Updated [TUnit.AspNetCore](https://github.com/thomhurst/TUnit) from 1.68.4 to 1.69.0. <details> <summary>Release notes</summary> _Sourced from [TUnit.AspNetCore's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.69.0 <!-- Release notes generated using configuration in .github/release.yml at v1.69.0 --> ## What's Changed ### Other Changes * feat(templates): add enableDotCover flag (#6714) by @ForNeVeR in thomhurst/TUnit#6844 * fix: don't request semantic models for attribute syntax from other compilations (DevKit crash) by @thomhurst in thomhurst/TUnit#6855 * fix(ci): restore net472 PublicAPI tests on Windows by @thomhurst in thomhurst/TUnit#6857 * perf(html-report): stream report JSON through pooled chunks and overlap sidecar serialization by @thomhurst in thomhurst/TUnit#6860 * chore(renovate): cap Microsoft.Build packages below 18.10.0 by @thomhurst in thomhurst/TUnit#6863 * perf: shrink generated per-class test source static constructors (~40% less startup JIT) by @thomhurst in thomhurst/TUnit#6859 * refactor: remove unreachable decimal source-text path from GenerateAttributeInstantiation by @thomhurst in thomhurst/TUnit#6856 * perf: cut per-test allocations in discovery and execution (-61% at 10k tests) by @thomhurst in thomhurst/TUnit#6861 * perf: stop hashing per-test event receivers during registration (data-driven tests 2.9x faster at 10k) by @thomhurst in thomhurst/TUnit#6858 * perf(analyzers): cut TUnit analyzer build time ~60% on large test projects by @thomhurst in thomhurst/TUnit#6862 ### Dependencies * chore(deps): update opentelemetry to 1.19.0 by @thomhurst in thomhurst/TUnit#6838 * chore(deps): update dependency opentelemetry.instrumentation.runtime to 1.19.0 by @thomhurst in thomhurst/TUnit#6840 * chore(deps): update tunit to 1.68.17 by @thomhurst in thomhurst/TUnit#6839 * chore(deps): update verify to 33.1.0 by @thomhurst in thomhurst/TUnit#6843 * chore(deps): update verify to 33.1.1 by @thomhurst in thomhurst/TUnit#6847 * chore(deps): update opentelemetry to 1.19.1 by @thomhurst in thomhurst/TUnit#6850 * chore(deps): update dependency grpc.core.api to 2.84.0 by @thomhurst in thomhurst/TUnit#6851 * chore(deps): update dependency stackexchange.redis to 3.3.1 by @thomhurst in thomhurst/TUnit#6853 * chore(deps): update dependency polyfill to 11.4.0 by @thomhurst in thomhurst/TUnit#6841 * chore(deps): update dependency polyfill to 11.4.0 by @thomhurst in thomhurst/TUnit#6842 ## New Contributors * @ForNeVeR made their first contribution in thomhurst/TUnit#6844 **Full Changelog**: thomhurst/TUnit@v1.68.17...v1.69.0 ## 1.68.17 <!-- Release notes generated using configuration in .github/release.yml at v1.68.17 --> ## What's Changed ### Other Changes * fix(mocks): emit init accessors for init-only properties and indexers by @thomhurst in thomhurst/TUnit#6833 * fix(mocks): let one type be mocked regularly and wrapped in one compilation by @thomhurst in thomhurst/TUnit#6835 * fix(mocks): keep editors in sync with publicized project references (#6836) by @thomhurst in thomhurst/TUnit#6837 ### Dependencies * chore(deps): update tunit to 1.68.4 by @thomhurst in thomhurst/TUnit#6824 * chore(deps): update mstest to 4.4.1 by @thomhurst in thomhurst/TUnit#6825 * chore(deps): update microsoft.testing by @thomhurst in thomhurst/TUnit#6717 * chore(deps): update verify to v33 by @thomhurst in thomhurst/TUnit#6794 * chore(deps): update dependency stackexchange.redis to 3.2.15 by @thomhurst in thomhurst/TUnit#6827 * chore(deps): update dependency messagepack to 3.1.9 by @thomhurst in thomhurst/TUnit#6828 * chore(deps): update dependency stackexchange.redis to 3.3.0 by @thomhurst in thomhurst/TUnit#6831 * chore(deps): update opentelemetry to 1.19.0 by @thomhurst in thomhurst/TUnit#6832 **Full Changelog**: thomhurst/TUnit@v1.68.4...v1.68.17 Commits viewable in [compare view](thomhurst/TUnit@v1.68.4...v1.69.0). </details> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Fixes #6834.
Problem
Reaching one type through both a regular mock and
Mock.Wrapin the same compilation aborts the generator:Because the generator aborts, every mock in the compilation disappears and the build fails with a cascade of CS1061 "does not contain a definition for ..." errors on unrelated mocks; the real cause is visible only in that one warning.
Mock.Of<T>()andMock.Wrap(instance)produce twoMockTypeModels differing only inIsWrapMock. Model equality includes that flag, so both survive dedup — correctly, since each needs its own impl and factory — butGenerateWrapMockandGenerateSingleTypeMockboth routed throughGenerateImplFactoryMembersAndEvents, adding the same_MockImplFactory.g.cs,_MockMembers.g.csand_MockEvents.g.cshint names.GeneratedNameCollisionDetectoralready recognised this situation — its own comment names "Mock.Of and Mock.Wrap of one type" as models sharing an identity — and deliberately does not report TM008 for it, but nothing downstream prevented the duplicate emission.Fix
The wrap impl and factory are already distinct types (
{name}WrapMockImpl/{name}WrapMockFactory, bothfile-scoped), so they only needed a hint name of their own:{name}_WrapMockImplFactory.g.cs.The setup and verification surface is a different matter. It describes the mocked type rather than the construction mode, is byte-identical between the two models, and its extension class is not
file-scoped — so it has to be emitted exactly once. A newSharedMemberSurfaceResolverstep runs over the collected requests and assigns ownership: the regular model emits it (it also emits the staticMock()entry point), and a wrap model emits it only when it is the type's only model.The identity used to recognise one target reached in several modes is now shared with
GeneratedNameCollisionDetector, which already had to tell that case apart from a genuine #6505 collision.Tests
tests/TUnit.Mocks.Tests/Issue6834Tests.cs— one type used both ways in one compilation: the regular mock, the wrap mock falling through to the real instance and then being configured, one setup surface serving both, and raise extensions emitted once and working for both.tests/TUnit.Mocks.SourceGenerator.Tests/Issue6834Tests.cs— a snapshot of the dual-mode output plus assertions that hint names are unique, that both impl/factory files are present, that_MockMembers.g.csis emitted exactly once, and that wrapping without a regular mock still emits the member surface.Four existing wrap snapshots are re-recorded. Renaming the wrap hint changes where that file sorts in the concatenated snapshot; the generated code is unchanged (verified by comparing the sorted contents).
Suites on net10.0: TUnit.Mocks.Tests 1309/1309, TUnit.Mocks.SourceGenerator.Tests 156/156, Analyzers 63/63, Http 58/58, Logging 31/31, InternalsAccess 29/29.
Note
#6833 works around this bug by using two separate types in its tests (
VirtualInitOnlyProperty/WrappableInitOnlyProperty). Once this merges that split is no longer needed and the comment there can go.Summary by CodeRabbit
Bug Fixes
Tests