-
Notifications
You must be signed in to change notification settings - Fork 290
Codefix for MSTEST0023 #6796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Codefix for MSTEST0023 #6796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a code fix for analyzer MSTEST0023, which suggests using the appropriate Assert method instead of negating boolean expressions in test assertions. For example, Assert.IsTrue(!condition) is replaced with Assert.IsFalse(condition).
Key Changes:
- Adds a new code fix provider
DoNotNegateBooleanAssertionFixerthat automatically corrects negated boolean assertions - Updates the analyzer to include diagnostic properties that guide the code fix
- Adds comprehensive test coverage for various negation scenarios including simple negations, double negations, complex expressions, and method calls
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Analyzers/MSTest.Analyzers.CodeFixes/DoNotNegateBooleanAssertionFixer.cs | New code fix provider that removes negations and swaps Assert.IsTrue/IsFalse methods |
| src/Analyzers/MSTest.Analyzers/DoNotNegateBooleanAssertionAnalyzer.cs | Updated analyzer to include proper assert method name in diagnostic properties |
| src/Analyzers/MSTest.Analyzers/Resources.resx | Added localization strings for code fix titles |
| src/Analyzers/MSTest.Analyzers/xlf/Resources.*.xlf | Added localization entries for all supported languages |
| test/UnitTests/MSTest.Analyzers.UnitTests/DoNotNegateBooleanAssertionAnalyzerTests.cs | Added comprehensive test cases for code fix functionality |
src/Analyzers/MSTest.Analyzers.CodeFixes/DoNotNegateBooleanAssertionFixer.cs
Outdated
Show resolved
Hide resolved
Evangelink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace all calls to VerifyAnalyzerAsync from existing tests please
src/Analyzers/MSTest.Analyzers.CodeFixes/DoNotNegateBooleanAssertionFixer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers.CodeFixes/DoNotNegateBooleanAssertionFixer.cs
Outdated
Show resolved
Hide resolved
src/Analyzers/MSTest.Analyzers.CodeFixes/DoNotNegateBooleanAssertionFixer.cs
Outdated
Show resolved
Hide resolved
…ertionFixer.cs Co-authored-by: Copilot <[email protected]>
you want me to replace all |
|
Ideally everything should always use the code fix verifier API. This helps with ensuring we test as many cases as possible. |
All the test cases has been updated to use |
Evangelink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have had a few issues with trivias on our code fixers, I'd be great to add a few tests like:
Assert.IsTrue(!false,
"some explanation");
Assert.IsTrue(/* some comment */ !false /* some other comment */);| return expression; | ||
| } | ||
|
|
||
| private sealed class FixAll : DocumentBasedFixAllProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unused and can be removed.
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task WhenAssertIsTrueWithNegation_Diagnostic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test case can be dropped as it's already covered above.
| } | ||
|
|
||
| [TestMethod] | ||
| public async Task WhenAssertIsFalseWithNegation_Diagnostic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark for this test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
| public override FixAllProvider GetFixAllProvider() | ||
| => WellKnownFixAllProviders.BatchFixer; |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GetFixAllProvider method returns WellKnownFixAllProviders.BatchFixer, but there's a complete FixAll class implementation (lines 235-367) that is never used. Either the method should return new FixAll() to use the custom implementation, or the unused FixAll class should be removed if BatchFixer is sufficient for this scenario.
| // Get the actual argument from the invocation | ||
| conditionArgument = invocation.ArgumentList.Arguments[conditionArgumentIndex]; | ||
|
|
||
| string title = string.Format(System.Globalization.CultureInfo.InvariantCulture, Resources.DoNotNegateBooleanAssertionFix, properAssertMethodName); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a using directive for System.Globalization instead of fully qualifying CultureInfo.InvariantCulture. This aligns with the coding guidelines that prefer using directives and is consistent with other code fixers in the codebase.
| private sealed class FixAll : DocumentBasedFixAllProvider | ||
| { | ||
| public static readonly FixAll Instance = new(); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire FixAll class (lines 235-367) is defined but never instantiated or used since GetFixAllProvider() returns WellKnownFixAllProviders.BatchFixer. This adds 133 lines of dead code. Remove this class unless you intend to use it by changing GetFixAllProvider() to return new FixAll().
This PR fixes #6585
This Implementation is to modify the
DoNotNegateBooleanAnalyzer code for to suggest better and cleaner code when developers negate boolean arguments.For example;
When a code is wrriten as
Assert.IsTrue(!condition), then the Analyzer would suggestAssert.IsFalse(condition)Assert.IsFalse(!condition)Analyzer would suggestAssert.IsTrue(condition).other edge cases were also considered, for example;
Assert.IsTrue(!(a && b))Analyzer would suggestAssert.IsFalse(a && b)Assert.IsFalse(!(x == y)), Analyzer would suggestAssert.IsTrue(x == y)Assert.IsTrue(!!condition)Analyzer would suggestAssert.IsFalse(condition)