Skip to content

Commit

Permalink
Modify S3431: Promote C# rule to SonarWay (#4127)
Browse files Browse the repository at this point in the history
  • Loading branch information
sebastien-marichal authored Aug 9, 2024
1 parent 8f7fcf7 commit 716a7aa
Show file tree
Hide file tree
Showing 14 changed files with 220 additions and 85 deletions.
33 changes: 33 additions & 0 deletions rules/S3431/csharp/flow-example.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
[source,csharp]
----
[TestMethod]
[ExpectedException(typeof(InvalidOperationException))]
public void UsingTest()
{
Console.ForegroundColor = ConsoleColor.Black;
try
{
using var _ = new ConsoleAlert();
Assert.AreEqual(ConsoleColor.Red, Console.ForegroundColor);
throw new InvalidOperationException();
}
finally
{
Assert.AreEqual(ConsoleColor.Black, Console.ForegroundColor); // The exception itself is not relevant for the test.
}
}
public sealed class ConsoleAlert : IDisposable
{
private readonly ConsoleColor previous;
public ConsoleAlert()
{
previous = Console.ForegroundColor;
Console.ForegroundColor = ConsoleColor.Red;
}
public void Dispose() =>
Console.ForegroundColor = previous;
}
----
30 changes: 30 additions & 0 deletions rules/S3431/csharp/how-mstest.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
== How to fix it in MSTest

Remove the `ExpectedException` attribute in favor of using the https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception[Assert.ThrowsException] assertion.

=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=1,diff-type=noncompliant]
----
[TestMethod]
[ExpectedException(typeof(ArgumentNullException))] // Noncompliant
public void Method_NullParam()
{
var sut = new MyService();
sut.Method(null);
}
----

==== Compliant solution

[source,csharp,diff-id=1,diff-type=compliant]
----
[TestMethod]
public void Method_NullParam()
{
var sut = new MyService();
Assert.ThrowsException<ArgumentNullException>(() => sut.Method(null));
}
----
30 changes: 30 additions & 0 deletions rules/S3431/csharp/how-nunit.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
== How to fix it in NUnit

Remove the `ExpectedException` attribute in favor of using the https://docs.nunit.org/articles/nunit/writing-tests/assertions/classic-assertions/Assert.Throws.html[Assert.Throws] assertion.

=== Code examples

==== Noncompliant code example

[source,csharp,diff-id=2,diff-type=noncompliant]
----
[Test]
[ExpectedException(typeof(ArgumentNullException))] // Noncompliant
public void Method_NullParam()
{
var sut = new MyService();
sut.Method(null);
}
----

==== Compliant solution

[source,csharp,diff-id=2,diff-type=compliant]
----
[Test]
public void Method_NullParam()
{
var sut = new MyService();
Assert.Throws<ArgumentNullException>(() => sut.Method(null));
}
----
52 changes: 9 additions & 43 deletions rules/S3431/csharp/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,52 +1,18 @@
include::../../../shared_content/dotnet/csharp_dictionary.adoc[]
:language: csharp

== Why is this an issue?

include::../description.adoc[]

=== Noncompliant code example

[source,csharp]
----
[TestMethod]
[ExpectedException(typeof(ArgumentNullException))] // Noncompliant
public void TestNullArg()
{
//...
}
----

=== Compliant solution

[source,csharp]
----
[TestMethod]
public void TestNullArg()
{
bool callFailed = false;
try
{
//...
}
catch (ArgumentNullException)
{
callFailed = true;
}
Assert.IsTrue(callFailed, "Expected call to MyMethod to fail with ArgumentNullException");
}
----

or

[source,csharp]
----
[TestMethod]
public void TestNullArg()
{
Assert.ThrowsException<ArgumentNullException>(() => /*...*/);
}
----

include::../exceptions.adoc[]

include::./how-mstest.adoc[]

include::./how-nunit.adoc[]

include::../resources.adoc[]

ifdef::env-github,rspecator-view[]

'''
Expand Down
2 changes: 1 addition & 1 deletion rules/S3431/description.adoc
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
It should be clear to a casual reader what code a test is testing and what results are expected. Unfortunately, that's not usually the case with the `ExpectedException` attribute since an exception could be thrown from almost any line in the method.

This rule detects MSTest and NUnit `ExpectedException` attribute.
This rule detects MSTest and NUnit `ExpectedException` attribute.
7 changes: 6 additions & 1 deletion rules/S3431/exceptions.adoc
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
=== Exceptions

This rule ignores one-line test methods, since it is obvious in such methods where the exception is expected to be thrown.
This rule ignores:

* single-line tests, since it is obvious in such methods where the exception is expected to be thrown
* tests when it tests control flow and assertion are present in either a `{keyword_catch}` or `{keyword_finally}` clause
include::{language}/flow-example.adoc[]
2 changes: 1 addition & 1 deletion rules/S3431/metadata.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"sqKey": "S3431",
"scope": "Tests",
"defaultQualityProfiles": [

"Sonar way"
],
"quickfix": "unknown"
}
8 changes: 8 additions & 0 deletions rules/S3431/resources.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
== Resources

=== Documentation

* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception[Assert.ThrowsException Method]
* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.expectedexceptionattribute[ExpectedExceptionAttribute Class]
* NUnit - https://docs.nunit.org/articles/nunit/writing-tests/assertions/classic-assertions/Assert.Throws.html[Assert.Throws]
* NUnit - https://docs.nunit.org/2.4/exception.html[ExpectedExceptionAttribute]
31 changes: 31 additions & 0 deletions rules/S3431/vbnet/flow-example.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
[source,vbnet]
----
<TestMethod>
<ExpectedException(GetType(InvalidOperationException))>
Public Sub UsingTest()
Console.ForegroundColor = ConsoleColor.Black
Try
Using alert As New ConsoleAlert()
Assert.AreEqual(ConsoleColor.Red, Console.ForegroundColor)
Throw New InvalidOperationException()
End Using
Finally
Assert.AreEqual(ConsoleColor.Black, Console.ForegroundColor) ' The exception itself is not relevant for the test.
End Try
End Sub
Public NotInheritable Class ConsoleAlert
Implements IDisposable
Private ReadOnly previous As ConsoleColor
Public Sub New()
previous = Console.ForegroundColor
Console.ForegroundColor = ConsoleColor.Red
End Sub
Public Sub Dispose() Implements IDisposable.Dispose
Console.ForegroundColor = previous
End Sub
End Class
----
28 changes: 28 additions & 0 deletions rules/S3431/vbnet/how-mstest.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
== How to fix it in MSTest

Remove the `ExpectedException` attribute in favor of using the https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception[Assert.ThrowsException] assertion.

=== Code examples

==== Noncompliant code example

[source,vbnet,diff-id=1,diff-type=noncompliant]
----
<TestMethod>
<ExpectedException(GetType(ArgumentNullException))> ' Noncompliant
Public Sub Method_NullParam()
Dim sut As New MyService()
sut.Method(Nothing)
End Sub
----

==== Compliant solution

[source,vbnet,diff-id=1,diff-type=compliant]
----
<TestMethod>
Public Sub Method_NullParam()
Dim sut As New MyService()
Assert.ThrowsException(Of ArgumentNullException)(Sub() sut.Method(Nothing))
End Sub
----
28 changes: 28 additions & 0 deletions rules/S3431/vbnet/how-nunit.adoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
== How to fix it in NUnit

Remove the `ExpectedException` attribute in favor of using the https://docs.nunit.org/articles/nunit/writing-tests/assertions/classic-assertions/Assert.Throws.html[Assert.Throws] assertion.

=== Code examples

==== Noncompliant code example

[source,vbnet,diff-id=2,diff-type=noncompliant]
----
<Test>
<ExpectedException(GetType(ArgumentNullException))> ' Noncompliant
Public Sub Method_NullParam()
Dim sut As New MyService()
sut.Method(Nothing)
End Sub
----

==== Compliant solution

[source,vbnet,diff-id=2,diff-type=compliant]
----
<Test>
Public Sub Method_NullParam()
Dim sut As New MyService()
Assert.Throws(Of ArgumentNullException)(Sub() sut.Method(Nothing))
End Sub
----
46 changes: 9 additions & 37 deletions rules/S3431/vbnet/rule.adoc
Original file line number Diff line number Diff line change
@@ -1,46 +1,18 @@
include::../../../shared_content/dotnet/vbnet_dictionary.adoc[]
:language: vbnet

== Why is this an issue?

include::../description.adoc[]

=== Noncompliant code example

[source,vbnet]
----
<TestMethod>
<ExpectedException(GetType(ArgumentNullException))> ' Noncompliant
Public Sub TestNullArg()
'...
End Sub
----

=== Compliant solution

[source,vbnet]
----
<TestMethod>
Public Sub TestNullArg()
Dim CallFailed As Boolean = False
Try
' ...
Catch ex As Exception
CallFailed = true
End Try
Assert.IsTrue(CallFailed, "Expected call to MyMethod to fail with ArgumentNullException")
End Sub
----

or

[source,vbnet]
----
<TestMethod>
Public Sub TestNullArg()
Assert.ThrowsException(Of ArgumentNullException)(Sub() ... )
End Sub
----

include::../exceptions.adoc[]

include::./how-mstest.adoc[]

include::./how-nunit.adoc[]

include::../resources.adoc[]

ifdef::env-github,rspecator-view[]
'''
== Comments And Links
Expand Down
4 changes: 3 additions & 1 deletion shared_content/dotnet/csharp_dictionary.adoc
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
:keyword_null: null
:keyword_async: async
:keyword_catch: catch
:keyword_finally: finally
:concept_method: method
:typeparameter_TResult: <TResult>
:typeparameter_TResult: <TResult>
4 changes: 3 additions & 1 deletion shared_content/dotnet/vbnet_dictionary.adoc
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
:keyword_null: Nothing
:keyword_async: Async
:keyword_catch: Catch
:keyword_finally: Finally
:concept_method: procedure
:typeparameter_TResult: (Of TResult)
:typeparameter_TResult: (Of TResult)

0 comments on commit 716a7aa

Please sign in to comment.