Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Comment |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
|
@copilot I have guidance for you. OverviewIssue #634 requires refactoring the This is prerequisite work for issue #637 (currently blocked in Future milestone), which will remove the string-based fallback entirely once we verify symbol-based detection is comprehensive. Related Issues:
Context & RationaleWhy This Matters
Current State (from PR #633)The
This was introduced in PR #633 as part of CRAP score reduction refactoring. All 1690 tests pass with this implementation. Similar Patterns to TrackString-based detection exists in other locations (potential follow-up work):
Investigation Findings from PR #770Problem DiscoveryInitial attempt to remove the string-based fallback revealed that current symbol-based detection is incomplete. Removing Root Cause AnalysisFailing Test Pattern: mockProvider.Setup(x => x.Submit()).Raises(x => x.StringEvent += null, "arg");Issue: According to Moq's source code,
These classes implement fluent interfaces that provide the Current Symbol Coverage✅ Currently Covered:
❌ Missing Coverage:
Previous Investigation (Commit 8b3909c)A similar investigation added Recommended Investigation Approach@rjmurillo's Preferred Method: Combination of static analysis and debugging (especially debugging if AI Agent is capable) To complete Phase A (symbol registry) and unblock issue #637, follow this multi-pronged investigation approach: Step 1: Static Analysis of Moq Source Code
Step 2: Runtime Debugging (PREFERRED for AI Agents)Create a diagnostic test to capture actual runtime types:
Step 3: Cross-Reference with MoqKnownSymbols
Step 4: Add Missing Symbols to MoqKnownSymbolsOnce interfaces are identified:
Expected OutcomeAfter completing Steps 1-4:
Current DecisionStatus: Investigation complete, escalated to @rjmurillo for guidance. Per project guidelines, work was stopped when:
No code changes committed as string-based fallback must remain until symbol-based detection is proven comprehensive across all test scenarios. Technical ApproachFiles to ModifyPrimary:
Validation:
Implementation StepsBased on investigation in PR #770, this work now requires two phases: Phase A: Complete Symbol Registry (PREREQUISITE)
Phase B: Remove String-Based Fallback (THIS GUIDE)
PHASE A: Complete Symbol Registry (START HERE)Prerequisites
Step A1: Static Analysis of Moq SourceIf you can access external repositories:
If you cannot clone repos, proceed to Step A2 (debugging). Step A2: Runtime Debugging (CRITICAL)Create a diagnostic test to capture actual symbol information:
Step A3: Update MoqKnownSymbols
Step A4: Validate Symbol Coverage
Step A5: Document and Commit
Step A6: Proceed to Phase BOnce Phase A is complete and validated:
Then and only then proceed to Phase B (Phase 1 below) to remove string-based fallback. PHASE B: Remove String-Based FallbackPhase 1: Prerequisites & Validation
Phase 2: Refactoring (Only After Phase 1 Complete)
Phase 3: Testing & Validation
Phase 4: Documentation
Success Criteria (Non-Negotiable)Must Pass Before PR
PR Evidence RequiredPer
Critical ConstraintsStop ConditionsYou MUST STOP and request expert guidance if:
Escalation PathIf blocked or uncertain:
Coordination with #637After completing this work:
Resource Constraints
ReferencesKey Files
Related Work
Documentation
NotesWork is now TWO-PHASE:
Current Status:
Remember:
Questions? Comment on #634, review PR #770, or ping @rjmurillo directly. Thank you for helping maintain code quality and consistency! 🚀 |
... The guidance message appears to be truncated (ends with "Th..."). Could you provide the complete guidance so I can proceed with the appropriate next steps? I've documented the investigation findings showing 20 test failures due to missing Moq interface symbols in |
|
@copilot I implemented offline with claudette-auto and Sonnet 4.5 in commit b47bbac Implementation Summary: Symbol-Based Raises DetectionIssue: Replace string-based Raises method detection with symbol-based detection Changes Made1. Added
|
| Pattern | Interface | Detected Via |
|---|---|---|
mock.Raise(x => x.Event += null, args) |
Mock<T> |
Direct method check |
mock.Setup(x => x.Method()).Raises(x => x.Event += null, args) |
IRaise<T> |
IRaise1Raises ✓ |
mock.Setup(x => x.Method()).RaisesAsync(x => x.Event += null, args) |
IRaise<T> |
IRaise1RaisesAsync ✓ |
Files Modified
src/Common/WellKnown/MoqKnownSymbols.cs- Added IRaise1 and method collectionssrc/Common/ISymbolExtensions.cs- Updated IsRaiseableMethod, removed string-based fallback
Migration Complete
✅ 100% symbol-based detection
✅ Zero string-based heuristics remaining
✅ All tests passing
✅ Build clean with PedanticMode
This implementation follows the repository's mandate: prefer symbol-based detection over string matching, ensuring type safety and resilience to code refactoring.
- Documents symbol-based detection as mandatory approach - Explains generic type handling with backtick notation - Describes Moq fluent API chain pattern and interface registration - Provides diagnostic investigation pattern for debugging - Documents context preservation with analysis contexts Related to #770
- Provides step-by-step debugging process for symbol detection issues - Includes example diagnostic test with SemanticModel.GetSymbolInfo() - Documents common symbol detection issues and resolutions - Helps developers debug when tests fail after symbol updates Related to #770
There was a problem hiding this comment.
Pull Request Overview
This PR replaces string-based method detection with comprehensive symbol-based detection for Moq's Raises methods. The primary issue was that IsMoqRaisesMethod used a mixed approach with string-based fallback, which was inconsistent with the codebase's established MoqKnownSymbols pattern.
Key Changes:
- Added missing
IRaise<T>interface and related setup interface symbols toMoqKnownSymbols - Removed string-based fallback methods from
ISymbolExtensions.cs - Enhanced semantic analysis to handle all Moq fluent interface patterns with symbol-based detection
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Common/WellKnown/MoqKnownSymbols.cs |
Added comprehensive symbol definitions for IRaise<T>, setup interfaces, and concrete setup phrase classes |
src/Common/SemanticModelExtensions.cs |
Updated IsRaisesInvocation with improved symbol resolution and conservative syntactic fallback for edge cases |
src/Common/ISymbolExtensions.cs |
Removed string-based fallback methods and added new symbol-based detection methods for setup interfaces |
- Adds symbol-based detection architecture explanation - Documents symbol registry pattern with code examples - Explains Moq fluent API chain with interface table - Provides debugging guide for symbol detection issues - Includes investigation pattern with example diagnostic test Related to #770
- Documents all 5 improvements with rationale and impact - Includes code examples for key patterns - Lists files modified with line counts - Captures learnings for future reference Related to #770
This reverts commit 80d4324.
- Eliminates fragile ToString().Contains() syntax analysis - Removes string.Equals() method name checking - Returns false for unresolved symbols, relying on complete symbol registry - Ensures 100% symbol-based detection with no heuristics Related to #770
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Problem
Issue #634 requests replacing string-based method name detection with comprehensive symbol-based detection in
IsMoqRaisesMethod. Initial investigation revealed that the current symbol-based detection was incomplete and removing the string-based fallback caused 20 test failures.Solution
Added missing
IRaise<T>interface symbol support toMoqKnownSymbolsto enable comprehensive symbol-based detection for all Moq Raises method patterns, including theSetup().Raises()fluent chain.Changes Made
1. Added
IRaise<T>Symbol SupportFile:
src/Common/WellKnown/MoqKnownSymbols.csAdded the missing
IRaise<T>interface to the known symbols registry:Why: The
Setup().Raises()fluent chain returnsIRaise<T>instances, which were not previously covered inMoqKnownSymbols.2. Updated Symbol Detection Logic
File:
src/Common/ISymbolExtensions.csIsLikelyMoqRaisesMethodByName)IsRaiseableMethodto check forIRaise<T>symbols3. Enhanced Semantic Analysis
File:
src/Common/SemanticModelExtensions.csImproved symbol resolution logic to handle all Moq fluent interface patterns.
Root Cause (Resolved)
The failing tests used this pattern:
According to Moq's source code,
Setup()returns concrete implementation classes (VoidSetupPhrase<T>andNonVoidSetupPhrase<T, TResult>) that implement theIRaise<T>interface, which provides theRaisesmethod. This interface was not previously registered inMoqKnownSymbols.Symbol Coverage (Complete)
MoqKnownSymbolsnow covers:ICallback,ICallback<T>,ICallback<TMock, TResult>Raises methodsIReturns,IReturns<T>,IReturns<TMock, TResult>Raises methodsIRaiseable.Raises,IRaiseableAsync.RaisesAsyncIRaise<T>.Raises,IRaise<T>.RaisesAsync(newly added)Testing
Related Issues
Status: Complete. All Moq Raises method patterns now use comprehensive symbol-based detection via
MoqKnownSymbols. String-based fallback has been removed.Fixes #634
Original prompt
This section details on the original issue you should resolve
<issue_title>Replace string-based method name detection with MoqKnownSymbols pattern in ISymbolExtensions</issue_title>
<issue_description>## Problem
The
IsMoqRaisesMethodimplementation insrc/Common/ISymbolExtensions.cscurrently uses a mixed approach that falls back to string-based method name checking when symbol-based detection fails. This goes against the established pattern of usingMoqKnownSymbolsfor proper symbol-based analysis throughout the codebase.Current Implementation Issues
The following methods use string-based detection as fallback:
IsRaisesMethodByName(lines ~284-294)IsRaisesMethodName(lines ~301-304)These methods check for method names "Raises" and "RaisesAsync" using string comparison, which is brittle and inconsistent with the codebase's symbol-based approach.
Expected Behavior
All Moq method detection should rely exclusively on the
MoqKnownSymbolspattern for:References
<agent_instructions>## Overview
Issue #634 requires refactoring the
IsMoqRaisesMethodimplementation insrc/Common/ISymbolExtensions.csto use only symbol-based detection via theMoqKnownSymbolspattern, removing the string-based fallback methods that are inconsistent with our established codebase patterns.This is prerequisite work for issue #637 (currently blocked in Future milestone), which will remove the string-based fallback entirely once we verify symbol-based detection is comprehensive.
Related Issues:
Context & Rationale
Why This Matters
Code Consistency: The codebase uses
MoqKnownSymbolspattern for type-safe, symbol-based method detection. The current mixed approach (symbol + string fallback) is inconsistent.Maintenance Burden: String-based detection is brittle and doesn't follow SOLID principles. It's harder to test and maintain.
Prerequisite Work: Issue Remove string-based fallback in IsMoqRaisesMethod once symbol-based detection is comprehensive #637 is blocked waiting for this refactor to ensure symbol-based detection is comprehensive before removing fallback.
Current State (from PR #633)
The
IsMoqRaisesMethodimplementation currently uses:IsKnownMoqRaisesMethod(symbol-based)IsLikelyMoqRaisesMethodByName(string-based)This was introduced in PR #633 as part of CRAP score reduction refactoring. All 1690 tests pass with this implementation.
Similar Patterns to Track
String-based detection exists in other locations (potential follow-up work):
EventSyntaxExtensions.cs:273(EventHandler name check)MethodSetupShouldSpecifyReturnValueAnalyzer.cs:157(Returns/Throws name checks)ISymbolExtensions.cs:302(Moq namespace check)Technical Approach
Files to Modify
Primary:
src/Common/ISymbolExtensions.cs(lines 205-310)IsMoqRaisesMethodmethodIsLikelyMoqRaisesMethodByNamemethod (lines 289-304)IsRaisesMethodNamehelper if unusedValidation:
tests/Moq.Analyzers.Test/IsRaisesMethodTests.csIsMoqRaisesMethod(7 locations found)Implementation Steps
Phase 1: Analysis & Validation Setup
Read Project Instructions (MANDATORY):
Review Current Implementation:
Locate All Usage:
Phase 2: Refactoring
Update
IsMoqRaisesMethod:|| method.IsLikelyMoqRaisesMethodByName()fallbackIsKnownMoqRaisesMethod(knownSymbols)Remove Obsolete Methods:
IsLikelyMoqRaisesMethodByName(lines 289-304)IsRaisesMethodNamehas no other callers, then deleteVerify Symbol Coverage:
MoqKnownSymbolshas complete cov...Fixes #634
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.