Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
…llback Investigation shows that removing the string-based fallback causes 20 test failures, indicating symbol-based detection is not yet comprehensive. Added ISetupGetter and ISetupSetter symbols but tests still fail. The fallback is currently necessary and requires further investigation to identify missing Moq interface symbols. Co-authored-by: rjmurillo <6811113+rjmurillo@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR documents and investigates why the string-based fallback method in IsMoqRaisesMethod cannot yet be safely removed, addressing issue #634. The investigation revealed that symbol-based detection is incomplete, causing 20 test failures when the fallback is removed.
- Enhanced documentation with comprehensive investigation findings and root cause analysis
- Added new Moq interface symbols (
ISetupGetterandISetupSetter) to improve symbol coverage - Kept the fallback method in place to maintain test stability
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Common/WellKnown/MoqKnownSymbols.cs | Added ISetupGetter and ISetupSetter interface symbols with their Raises methods |
| src/Common/ISymbolExtensions.cs | Enhanced TODO comment with detailed investigation findings and added new symbol checks |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
rjmurillo
deleted the
copilot/remove-string-fallback-in-ismoqraisesmethod
branch
Problem
Issue #634 requested removal of the string-based fallback method
IsLikelyMoqRaisesMethodByNamefromIsMoqRaisesMethod, as it was originally added as a temporary safety net in PR #633. The TODO comment indicated this should be removed once symbol-based detection is comprehensive.Investigation
I attempted to remove the fallback by relying solely on symbol-based detection through
IsKnownMoqRaisesMethod. However, this revealed that the symbol-based detection is not yet comprehensive enough to safely remove the fallback.Test Results
Removing the fallback causes 20 test failures in
RaisesEventArgumentsShouldMatchEventSignatureAnalyzerTestsacross both:The failing tests involve detecting
Raisescalls in chains like:Attempted Fixes
I attempted to resolve the issue by adding missing Moq interface symbols to
MoqKnownSymbols:ISetupGetter<TMock, TProperty>- Interface for property getter setups that haveRaisesmethodsISetupSetter<TMock, TProperty>- Interface for property setter setups that haveRaisesmethodsHowever, even with these additions, the tests still fail, indicating the root cause is more complex.
Current Symbol Coverage
The symbol-based detection now checks 11 different Moq interface method symbols:
ICallback.Raises,ICallback<T>.Raises,ICallback<TMock, TResult>.RaisesIReturns.Raises,IReturns<T>.Raises,IReturns<TMock, TResult>.RaisesISetupGetter<TMock, TProperty>.Raises,ISetupSetter<TMock, TProperty>.RaisesIRaiseable.Raises,IRaiseableAsync.RaisesAsyncDespite this comprehensive coverage, the symbol resolution for chained method calls like
Setup().Raises()is not matching these interface definitions correctly.Root Cause Analysis
The issue appears to be related to how Roslyn resolves method symbols for chained fluent API calls. When analyzing
mock.Setup(...).Raises(...), the method symbol forRaisesmay not resolve to the specific interface definitions we're checking against viaIsInstanceOf, possibly due to:Changes Made
Added new symbols to
MoqKnownSymbols.cs:ISetupGetterandISetupSetter(useful for future improvements even though they don't fully resolve the current issue)Enhanced TODO comment with comprehensive investigation documentation:
Kept the fallback in place to maintain test stability and prevent regressions
Recommended Next Steps
To safely remove the fallback, the following investigation is needed:
IsInstanceOfis the right approach, or if alternative symbol matching strategies are neededTesting
✅ All 1,874 tests pass
✅ Code formatted with
dotnet format✅ Build succeeds without warnings
✅ No regressions introduced
Related
Note: This PR documents the investigation findings and improves the codebase by adding useful symbols and comprehensive documentation. However, it does not remove the fallback as originally requested, because doing so would break 20 tests. The issue should remain open until the symbol-based detection can be made comprehensive enough to safely remove the fallback.
Original prompt
This section details on the original issue you should resolve
<issue_title>Remove string-based fallback in IsMoqRaisesMethod once symbol-based detection is comprehensive</issue_title>
<issue_description>## Description
A TODO comment in
src/Common/ISymbolExtensions.csindicates that a string-based fallback method should be removed once symbol-based detection for Moq Raises methods is complete.Location
src/Common/ISymbolExtensions.csIsMoqRaisesMethodCurrent Implementation
The method currently uses both:
IsKnownMoqRaisesMethodIsLikelyMoqRaisesMethodByNameTask
Remove the fallback method
IsLikelyMoqRaisesMethodByNameonce the symbol-based detection is verified to handle all Moq Raises method scenarios comprehensively.Context
This was flagged during security scanning as a TODO that may indicate incomplete functionality.
References
Requested by: @rjmurillo</issue_description>
<agent_instructions>User Impact & Necessity
Frequency & Real-World Usage:
The Moq Raises and RaisesAsync methods are documented as core event-handling features in Moq's quickstart guide
My research shows active test coverage with dedicated test suites (IsRaisesMethodTests.cs, RaiseEventArgumentsShouldMatchEventSignatureAnalyzerTests.cs, RaisesEventArgumentsShouldMatchEventSignatureAnalyzerTests.cs)
The method is used in 7 locations across the codebase for event-related analyzer detection
Issue #434 specifically tracks Moq quickstart coverage, confirming event patterns (including Raises) are priority features
Current Pain Points:
The string-based fallback (IsLikelyMoqRaisesMethodByName) was intentionally introduced as a "temporary safety net" in PR #633
GitHub Advanced Security flagged this as a TODO indicating "incomplete functionality"
The implementation contradicts established patterns: All other Moq method detection (Setup, Verify, Returns, Callback) use only symbol-based detection via MoqKnownSymbols
The fallback checks namespace.StartsWith("Moq") which could match unintended namespaces (false positives) and miss methods where symbol resolution temporarily fails
Evidence of Issue:
Cursor bot comment on PR #633 identified a namespace detection bug where the logic changed from checking Contains("Moq.Language") to StartsWith("Moq"), introducing both false positives and negatives
Issue #634 (also created from PR #633) specifically requests removal of string-based detection in favor of MoqKnownSymbols pattern
Issue #549 (closed) successfully removed similar string-based fallbacks for consistency
Implementation & Maintenance
Complexity:
Low complexity: The fix involves removing ~15 lines of fallback code (IsLikelyMoqRaisesMethodByName)
Symbol-based detection is already comprehensive: IsKnownMoqRaisesMethod checks 8 different Moq interface method symbols (ICallback, IReturns, IRaiseable variants)
Tests exist and pass with current implementation
Investigation Required:
The key question is: Does the fallback ever actually trigger?
From the code review:
The primary symbol-based path checks: ICallbackRaises, ICallback1Raises, ICallback2Raises, IReturnsRaises, IReturns1Raises, IReturns2Raises, IRaiseableRaises, IRaiseableAsyncRaisesAsync
These cover all documented Moq Raises patterns per the quickstart
If the fallback were needed, tests would fail when it's removed
Recommended Approach:
Remove the fallback method
Run full test suite (including IsRaisesMethodTests, all event-related analyzer tests)
If tests pass → fallback was unnecessary safety theatre
If tests fail → identify which scenarios the symbol-based detection misses and add those symbols to MoqKnownSymbols
Alignment with Project Goals
Strong Alignment:
Code quality: Removing technical debt and inconsistency
Maintainability: One less code path to maintain, reduced complexity
Type safety: Symbol-based detection is more robust than string matching
Performance: Eliminates string operations in hot path
Consistency: Aligns with patterns used in IsMoqSetupMethod, IsMoqReturnsMethod, IsMoqCallbackMethod, etc.
Project Priority:
This is directly related to Issue #434 (Moq quickstart coverage), which has 14/15 sub-issues completed and is marked with "triage" label
The removal supports the broader goal of ensuring all Moq patterns have proper symbol-based detection
The codebase shows active investment in event handling (multiple analyzers: Moq1202, Moq1204, Moq1205)
Trade-offs & Risks
Benefits:
✅ Eliminates false positive risk (matching non-Moq "Raises" methods in any namespace starting with "Moq")
✅ Eliminates false negative risk (missing Moq methods when namespace check fails)
✅ Reduces cognitive load (one clear detection path instead of two)
✅ Aligns with established codebase p...
Fixes #637
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.