feat: emit nested-mock TODO across all migration rewrite sites#69
Merged
Conversation
NSubstitute and Moq fixers silently rewrite nested receivers like sub.Child.Foo() into sub.Child.Mock.Setup/Verify/Raise.Foo() chains, which NRE at runtime if the nested child isn't explicitly registered (Mockolate doesn't auto-mock recursively). The setup path already emitted a TODO comment warning the user; this extends the same warning to every other rewrite site that builds a nested chain: - NSubstitute: method verify, property verify (Got/Set), event raise. - Moq: Setup call, Setup property access, SetupProperty, Verify call, VerifyAdd/VerifyRemove event, VerifyGet/VerifySet property. Also fixes a trivia-clobbering bug in the Moq Callback rewrite: WithTriviaFrom(invocation) on the wrapping Do() expression was overwriting the leading trivia of the embedded migrated setup, dropping any TODO attached to it. Switched to WithTrailingTrivia only so the leading trivia (with the TODO) is preserved.
There was a problem hiding this comment.
Pull request overview
This PR extends the existing “nested mock chain” warning behavior so that when migrations produce nested .Mock.* chains (which can NRE unless the nested mocks are explicitly registered), the code fix emits a TODO comment at all relevant rewrite sites (Moq + NSubstitute). It also fixes a Moq callback-trivia issue that could drop that TODO during .Callback(...) → .Do(...) rewrites.
Changes:
- Emit a nested-mock TODO comment for additional nested-chain rewrite sites across both NSubstitute and Moq fixers (verify/get/set/raise/etc.).
- Fix Moq
.Callback(...)→.Do(...)rewrite trivia handling to preserve leading trivia on embedded migrated setups (including the TODO). - Expand/adjust test expectations and add coverage for the new TODO emission behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Tests/Mockolate.Migration.Tests/NSubstituteCodeFixProviderTests.NestedTests.cs | Adds test coverage for TODO emission on nested Received property got/set and nested event raise rewrites. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.VerifyTests.cs | Updates expected output to include nested-mock TODO for nested verify rewrite. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.VerifyEventTests.cs | Updates expected output to include nested-mock TODO for nested VerifyAdd/VerifyRemove rewrite. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.SetupTests.cs | Updates expected output to include nested-mock TODO for nested Setup/SetupProperty-related rewrites. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.PropertyTests.cs | Updates expected output to include nested-mock TODO for nested VerifyGet/VerifySet rewrite. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.NewMockTests.cs | Updates expected output to include nested-mock TODO for nested mock navigation migrations. |
| Tests/Mockolate.Migration.Tests/MoqCodeFixProviderTests.CallbackTests.cs | Adds a test ensuring nested-mock TODO trivia is preserved through the .Callback → .Do rewrite. |
| Source/Mockolate.Migration.Analyzers.CodeFixers/NSubstituteCodeFixProvider.cs | Extends TODO emission to nested verify/property/event-raise rewrite sites using the existing trivia/TODO infrastructure. |
| Source/Mockolate.Migration.Analyzers.CodeFixers/MoqCodeFixProvider.cs | Adds nested-mock TODO trivia builder and wires it into all nested rewrite sites; adjusts callback rewrite to preserve leading trivia on embedded migrated setups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Generated TODO comments now include the rule ID of the analyzer that triggered the codefix in brackets, so users can grep for or filter unresolved migration hints by source: // TODO(MockolateM001): register the nested 'mock.Child' chain ... // TODO(MockolateM002): review CallInfo usage manually ... Applies to every TODO emitted by NSubstituteCodeFixProvider (MockolateM002 — both the nested-mock and CallInfo TODOs) and MoqCodeFixProvider (MockolateM001 — nested-mock TODO).
- Rename ApplySetupTrivia → ApplyTodoTrivia and update its summary comment: the helper is no longer setup-specific, it is invoked from setup-call, setup-property, method verify, property verify, and event raise rewrite paths. - Simplify Moq DetectLineEnding to a LINQ Where/Select/FirstOrDefault expression (Sonar S3267).
|
|
This is addressed in release v0.6.0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NSubstitute and Moq fixers silently rewrite nested receivers like sub.Child.Foo() into sub.Child.Mock.Setup/Verify/Raise.Foo() chains, which NRE at runtime if the nested child isn't explicitly registered (Mockolate doesn't auto-mock recursively). The setup path already emitted a TODO comment warning the user; this extends the same warning to every other rewrite site that builds a nested chain:
Also fixes a trivia-clobbering bug in the Moq Callback rewrite: WithTriviaFrom(invocation) on the wrapping Do() expression was overwriting the leading trivia of the embedded migrated setup, dropping any TODO attached to it. Switched to WithTrailingTrivia only so the leading trivia (with the TODO) is preserved.