Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix S3431 FP: Don't raise if assertions are done in catch or finally #9615

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Fix S3431 FP: Don't raise if assertions are done in catch or finally #9615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46fba4e
Fix S3431 FP: Don't raise if assertions are done in catch or finally
sebastien-marichal Aug 7, 2024
ebad9e2
Address comments
sebastien-marichal Aug 8, 2024
9798b1c
Remove base.VisitInvocationExpression
sebastien-marichal Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/ExpectedExceptionAttributeShouldNotBeUsed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,52 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.Common.Walkers;

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class ExpectedExceptionAttributeShouldNotBeUsed : ExpectedExceptionAttributeShouldNotBeUsedBase<SyntaxKind>
{
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override bool HasMultiLineBody(SyntaxNode syntax)
protected override bool HasMultiLineBody(SyntaxNode node)
{
var declaration = (MethodDeclarationSyntax)syntax;
var declaration = (MethodDeclarationSyntax)node;
return declaration.ExpressionBody is null
&& declaration.Body?.Statements.Count > 1;
}

protected override bool AssertInCatchFinallyBlock(SyntaxNode node)
{
var walker = new CatchFinallyAssertion();
foreach (var x in node.DescendantNodes().Where(x => x.Kind() is SyntaxKind.CatchClause or SyntaxKind.FinallyClause))
{
if (!walker.HasAssertion)
{
walker.SafeVisit(x);
}
}
return walker.HasAssertion;
}

private sealed class CatchFinallyAssertion : SafeCSharpSyntaxWalker
{
public bool HasAssertion { get; set; }

public override void VisitInvocationExpression(InvocationExpressionSyntax node)
{
if (HasAssertion)
{
return;
}

HasAssertion = node.Expression
.ToString()
.SplitCamelCaseToWords()
.Intersect(UnitTestHelper.KnownAssertionMethodParts)
.Any();
}
}
}
}
4 changes: 3 additions & 1 deletion analyzers/src/SonarAnalyzer.Common/Rules/ExpectedExceptionAttributeShouldNotBeUsedBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ public abstract class ExpectedExceptionAttributeShouldNotBeUsedBase<TSyntaxKind>
{
internal const string DiagnosticId = "S3431";

protected abstract bool HasMultiLineBody(SyntaxNode syntax);
protected abstract bool HasMultiLineBody(SyntaxNode node);
protected abstract bool AssertInCatchFinallyBlock(SyntaxNode node);

protected override string MessageFormat => "Replace the 'ExpectedException' attribute with a throw assertion or a try/catch block.";

Expand All @@ -35,6 +36,7 @@ protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(Language.GeneratedCodeRecognizer, c =>
{
if (HasMultiLineBody(c.Node)
&& !AssertInCatchFinallyBlock(c.Node)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rule will go through every method in a project and do this check with syntax walkers. Even though methods marked with ExpectedException are quite rare.
I'd optimize this rule in two places (this can be done in a separate PR):

  1. Before calling AssertInCatchFinallyBlock() check if the method declaration has any attributes called ExpectedException. This can be done on the syntax level. We're going to get a couple of FNs/FPs for some exotic scenarios: aliased attribute, partial test methods. These are quite rare, so I think it's ok.
  2. Before you register the analyzer, check if the project references MsTest or NUnit (with version < 3.0.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9627

&& c.SemanticModel.GetDeclaredSymbol(c.Node) is { } methodSymbol
&& methodSymbol.GetAttributes(UnitTestHelper.KnownExpectedExceptionAttributes).FirstOrDefault() is { } attribute)
{
Expand Down
44 changes: 37 additions & 7 deletions analyzers/src/SonarAnalyzer.VisualBasic/Rules/ExpectedExceptionAttributeShouldNotBeUsed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,45 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.VisualBasic
namespace SonarAnalyzer.Rules.VisualBasic;

[DiagnosticAnalyzer(LanguageNames.VisualBasic)]
public sealed class ExpectedExceptionAttributeShouldNotBeUsed : ExpectedExceptionAttributeShouldNotBeUsedBase<SyntaxKind>
{
[DiagnosticAnalyzer(LanguageNames.VisualBasic)]
public sealed class ExpectedExceptionAttributeShouldNotBeUsed : ExpectedExceptionAttributeShouldNotBeUsedBase<SyntaxKind>
protected override ILanguageFacade<SyntaxKind> Language => VisualBasicFacade.Instance;

protected override bool HasMultiLineBody(SyntaxNode node) =>
node.Parent is MethodBlockSyntax { Statements.Count: > 1 };

protected override bool AssertInCatchFinallyBlock(SyntaxNode node)
{
protected override ILanguageFacade<SyntaxKind> Language => VisualBasicFacade.Instance;
var walker = new CatchFinallyAssertion();
foreach (var x in node.Parent.DescendantNodes().Where(x => x.Kind() is SyntaxKind.CatchBlock or SyntaxKind.FinallyBlock))
{
if (!walker.HasAssertion)
{
walker.SafeVisit(x);
}
}
return walker.HasAssertion;
}

private sealed class CatchFinallyAssertion : SafeVisualBasicSyntaxWalker
{
public bool HasAssertion { get; set; }

public override void VisitInvocationExpression(InvocationExpressionSyntax node)
{
if (HasAssertion)
zsolt-kolbay-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
return;
}

protected override bool HasMultiLineBody(SyntaxNode syntax) =>
syntax.Parent is MethodBlockSyntax methodBlockSyntax
&& methodBlockSyntax.Statements.Count > 1;
HasAssertion = node.Expression
.ToString()
.SplitCamelCaseToWords()
.Intersect(UnitTestHelper.KnownAssertionMethodParts)
.Any();
}
}
}
105 changes: 52 additions & 53 deletions analyzers/tests/SonarAnalyzer.Test/Rules/ExpectedExceptionAttributeShouldNotBeUsedTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,64 +21,63 @@
using CS = SonarAnalyzer.Rules.CSharp;
using VB = SonarAnalyzer.Rules.VisualBasic;

namespace SonarAnalyzer.Test.Rules
namespace SonarAnalyzer.Test.Rules;

[TestClass]
public class ExpectedExceptionAttributeShouldNotBeUsedTest
{
[TestClass]
public class ExpectedExceptionAttributeShouldNotBeUsedTest
{
private readonly VerifierBuilder builderCS = new VerifierBuilder<CS.ExpectedExceptionAttributeShouldNotBeUsed>();
private readonly VerifierBuilder builderVB = new VerifierBuilder<VB.ExpectedExceptionAttributeShouldNotBeUsed>();
private readonly VerifierBuilder builderCS = new VerifierBuilder<CS.ExpectedExceptionAttributeShouldNotBeUsed>();
private readonly VerifierBuilder builderVB = new VerifierBuilder<VB.ExpectedExceptionAttributeShouldNotBeUsed>();

[DataTestMethod]
[DataRow("1.1.11")]
[DataRow(Constants.NuGetLatestVersion)]
public void ExpectedExceptionAttributeShouldNotBeUsed_MsTest_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.MsTest.cs")
.AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion))
.Verify();
[DataTestMethod]
[DataRow("1.1.11")]
[DataRow(Constants.NuGetLatestVersion)]
public void ExpectedExceptionAttributeShouldNotBeUsed_MsTest_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.MsTest.cs")
.AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion))
.Verify();

[DataTestMethod]
[DataRow("2.5.7.10213")]
[DataRow("2.6.7")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.cs")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.Verify();
[DataTestMethod]
[DataRow("2.5.7.10213")] // Lowest NUnit version available
[DataRow("2.7.1")] // Latest version of NUnit that contains the attribute
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.cs")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.Verify();

[DataTestMethod]
[DataRow("3.0.0")]
[DataRow(Constants.NuGetLatestVersion)]
[Description("Starting with version 3.0.0 the attribute was removed.")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_NoIssue_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.cs")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.WithErrorBehavior(CompilationErrorBehavior.Ignore)
.VerifyNoIssues();
[DataTestMethod]
[DataRow("3.0.0")]
[DataRow(Constants.NuGetLatestVersion)]
[Description("Starting with version 3.0.0 the attribute was removed.")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_NoIssue_CS(string testFwkVersion) =>
builderCS.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.cs")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.WithErrorBehavior(CompilationErrorBehavior.Ignore)
.VerifyNoIssues();

[DataTestMethod]
[DataRow("1.1.11")]
[DataRow(Constants.NuGetLatestVersion)]
public void ExpectedExceptionAttributeShouldNotBeUsed_MsTest_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.MsTest.vb")
.AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion))
.Verify();
[DataTestMethod]
[DataRow("1.1.11")]
[DataRow(Constants.NuGetLatestVersion)]
public void ExpectedExceptionAttributeShouldNotBeUsed_MsTest_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.MsTest.vb")
.AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion))
.Verify();

[DataTestMethod]
[DataRow("2.5.7.10213")]
[DataRow("2.6.7")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.vb")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.Verify();
[DataTestMethod]
[DataRow("2.5.7.10213")]
[DataRow("2.6.7")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.vb")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.Verify();

[DataTestMethod]
[DataRow("3.0.0")]
[DataRow(Constants.NuGetLatestVersion)]
[Description("Starting with version 3.0.0 the attribute was removed.")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_NoIssue_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.vb")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.WithErrorBehavior(CompilationErrorBehavior.Ignore)
.VerifyNoIssues();
}
[DataTestMethod]
[DataRow("3.0.0")]
[DataRow(Constants.NuGetLatestVersion)]
[Description("Starting with version 3.0.0 the attribute was removed.")]
public void ExpectedExceptionAttributeShouldNotBeUsed_NUnit_NoIssue_VB(string testFwkVersion) =>
builderVB.AddPaths("ExpectedExceptionAttributeShouldNotBeUsed.NUnit.vb")
.AddReferences(NuGetMetadataReference.NUnit(testFwkVersion))
.WithErrorBehavior(CompilationErrorBehavior.Ignore)
.VerifyNoIssues();
}
Loading
Loading