Conversation
📝 WalkthroughWalkthroughThe test suite for Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.cs`: I need your help tracking down and fixing some bugs that have been reported in this codebase.
|
![qlty-cloud-legacy[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR corrects the namespace used when detecting ValueTask return types in the analyzer and adds regression tests to ensure ValueTask-based setups are handled correctly.
- Fix string comparison for
ValueTaskinIsMethodReturnTypeTask - Add test methods and analyzer tests for
ValueTaskandValueTask<T>scenarios - Extend
AsyncClienttest stub withValueTask-returning members
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Moq.Analyzers.Test/SetupShouldNotIncludeAsyncResultAnalyzerTests.cs | Added tests for ValueTaskAsync(), GenericValueTaskAsync() and .Result diagnostics |
| src/Common/ISymbolExtensions.cs | Corrected the namespace comparison from System.Threading.ValueTask to System.Threading.Tasks.ValueTask |
Comments suppressed due to low confidence (2)
src/Common/ISymbolExtensions.cs:101
- Relying on
ToDisplayStringand string comparisons for matchingTask/ValueTaskis fragile. Consider using the symbol model (e.g.,methodSymbol.ReturnType.OriginalDefinition) orSymbolEqualityComparerto compare against the actualTaskandValueTasktypes.
string type = methodSymbol.ToDisplayString();
tests/Moq.Analyzers.Test/SetupShouldNotIncludeAsyncResultAnalyzerTests.cs:16
- [nitpick] Consider adding a test case for
.ThrowsAsync(...)onValueTask-returning methods (e.g.,Setup(c => c.GenericValueTaskAsync()).ThrowsAsync(new Exception())) to ensure exception-based setups are analyzed correctly.
["""new Mock<AsyncClient>().Setup(c => c.ValueTaskAsync());"""]
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/Common/ISymbolExtensions.cs(1 hunks)tests/Moq.Analyzers.Test/SetupShouldNotIncludeAsyncResultAnalyzerTests.cs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.cs`: I need your help tracking down and fixing some bugs that have been reported in this codebase.
I suspect the bugs are related to:
- Incorrect handling of edge cases
- O...
**/*.cs: I need your help tracking down and fixing some bugs that have been reported in this codebase.I suspect the bugs are related to:
- Incorrect handling of edge cases
- Off-by-one errors in loops or array indexing
- Unexpected data types
- Uncaught exceptions
- Concurrency issues
- Improper configuration settings
To diagnose:
- Review the code carefully and systematically
- Trace the relevant code paths
- Consider boundary conditions and potential error states
- Look for antipatterns that tend to cause bugs
- Run the code mentally with example inputs
- Think about interactions between components
When you find potential bugs, for each one provide:
- File path and line number(s)
- Description of the issue and why it's a bug
- Example input that would trigger the bug
- Suggestions for how to fix it
After analysis, please update the code with your proposed fixes. Try to match the existing code style. Add regression tests if possible to prevent the bugs from recurring.
I appreciate your diligence and attention to detail! Let me know if you need any clarification on the intended behavior of the code.
src/Common/ISymbolExtensions.cstests/Moq.Analyzers.Test/SetupShouldNotIncludeAsyncResultAnalyzerTests.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-24.04-arm)
🔇 Additional comments (4)
src/Common/ISymbolExtensions.cs (1)
103-103: LGTM! Namespace fix is correct.The namespace correction from
"System.Threading.ValueTask"to"System.Threading.Tasks.ValueTask"is accurate and aligns with the correct .NET namespace for ValueTask.tests/Moq.Analyzers.Test/SetupShouldNotIncludeAsyncResultAnalyzerTests.cs (3)
16-18: LGTM! Well-structured test additions for ValueTask scenarios.The new test cases properly cover ValueTask scenarios and follow the same patterns as the existing Task tests. The test data includes both basic ValueTask setup and generic ValueTask with Returns pattern.
23-23: LGTM! Proper test coverage for ValueTask Result property access.This test case correctly validates that accessing
.Resulton aValueTask<T>triggers theMoq1201diagnostic, providing good regression test coverage for the fixed ValueTask detection logic.
41-43: LGTM! Well-implemented test helper methods.The new virtual methods in the
AsyncClientclass are properly implemented:
ValueTaskAsync()returnsValueTask.CompletedTaskGenericValueTaskAsync()returnsValueTask.FromResult(string.Empty)These follow the same patterns as the existing Task methods and provide the necessary test infrastructure for the new ValueTask test cases.
- Enhanced `copilot-instructions.md` with repository description and contribution guidelines. - Updated package versions in `Packages.props` for `Meziantou.Analyzer`, `Polyfill`, and `Verify.Xunit`. - Modified `test.runsettings` to add exclusions for code coverage. - Added summary comment and new method `IsTaskOrValueResultProperty` in `SetupShouldBeUsedOnlyForOverridableMembersAnalyzer.cs`. - Introduced `IsConstructor` method in `ISymbolExtensions.cs` and removed `IsMethodReturnTypeTask`. - Updated `SquiggleCop.Baseline.yaml` to include new warnings and adjust existing entries.
|
Code Climate has analyzed commit ac0d3ce and detected 0 issues on this pull request. View more on Code Climate. |
ValueTaskstring comparison for task-return detectionValueTasksetup analysisSummary by CodeRabbit