-
Notifications
You must be signed in to change notification settings - Fork 32
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
Added warning: Async test method must have non-void return type #69
Conversation
{ | ||
class TestCaseUsageAnalyzerTestsAnalyzeWhenReturnTypeIsAsyncVoid | ||
{ | ||
[Test] |
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.
note, this isn't a TestCase
, but a Test
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.
Perhaps we will - at some time - need a test for both cases. But for now I think it is fine
@@ -5,6 +5,7 @@ internal static class TestCaseUsageAnalyzerConstants | |||
internal const string ExpectedResultTypeMismatchMessage = "The ExpectedResult value cannot be assigned to the return type, {0}"; | |||
internal const string NotEnoughArgumentsMessage = "There are not enough arguments provided from the TestCaseAttribute for the method"; | |||
internal const string SpecifiedExpectedResultForVoidMethodMessage = "Cannot specify ExpectedResult when the method returns void"; | |||
internal const string AsyncVoidMessage = "Async test method must have non-void return type"; |
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.
Not sure if this should be in TestCaseUsageAnalyzerConstants
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 will need another class as most of the async validations will be for Test
and TestCase
attributes.
if (testCaseType.ContainingAssembly.Identity == attributeSymbol?.ContainingAssembly.Identity && | ||
NunitFrameworkConstants.NameOfTestCaseAttribute == attributeSymbol?.ContainingType.Name) | ||
(isTestCase = NunitFrameworkConstants.NameOfTestCaseAttribute == attributeSymbol?.ContainingType.Name) | ||
|| NunitFrameworkConstants.NameOfTestAttribute == attributeSymbol?.ContainingType.Name) |
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.
maybe this could be simplified,. note this is now checking for TestCase + Test attributes
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.
Personally, I don't like assignments within boolean expressions (can be that I've not programmed enough C/C++). The plan is to have another analyzer that will run for both Test
and TestCase
attributes.
} | ||
|
||
AnalyzeReturnType(context, methodSymbol); |
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.
note: this will run with Test+TestCase
context.CancellationToken.ThrowIfCancellationRequested(); | ||
TestCaseUsageAnalyzer.AnalyzeNamedArguments(context, | ||
attributeNamedArguments, methodSymbol); | ||
} |
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.
note: nothing changed in this block above (lines 66-98)
any opinion on this one? |
@@ -27,6 +27,7 @@ public static class NunitFrameworkConstants | |||
public const string FullNameOfTypeTestCaseAttribute = "NUnit.Framework.TestCaseAttribute"; | |||
|
|||
public const string NameOfTestCaseAttribute = "TestCaseAttribute"; | |||
public const string NameOfTestAttribute = "TestAttribute"; |
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've exactly the same line in my local changes 😄
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.
👍 nice!
@@ -5,6 +5,7 @@ internal static class TestCaseUsageAnalyzerConstants | |||
internal const string ExpectedResultTypeMismatchMessage = "The ExpectedResult value cannot be assigned to the return type, {0}"; | |||
internal const string NotEnoughArgumentsMessage = "There are not enough arguments provided from the TestCaseAttribute for the method"; | |||
internal const string SpecifiedExpectedResultForVoidMethodMessage = "Cannot specify ExpectedResult when the method returns void"; | |||
internal const string AsyncVoidMessage = "Async test method must have non-void return type"; |
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 will need another class as most of the async validations will be for Test
and TestCase
attributes.
if (testCaseType.ContainingAssembly.Identity == attributeSymbol?.ContainingAssembly.Identity && | ||
NunitFrameworkConstants.NameOfTestCaseAttribute == attributeSymbol?.ContainingType.Name) | ||
(isTestCase = NunitFrameworkConstants.NameOfTestCaseAttribute == attributeSymbol?.ContainingType.Name) | ||
|| NunitFrameworkConstants.NameOfTestAttribute == attributeSymbol?.ContainingType.Name) |
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.
Personally, I don't like assignments within boolean expressions (can be that I've not programmed enough C/C++). The plan is to have another analyzer that will run for both Test
and TestCase
attributes.
@@ -128,6 +136,22 @@ private static void AnalyzeAttribute(SyntaxNodeAnalysisContext context) | |||
} | |||
} | |||
} | |||
private static void AnalyzeReturnType(SyntaxNodeAnalysisContext context, IMethodSymbol methodSymbol) |
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.
Major nit pick. Insert empty line before method and remove the empty line beginning the statements within the if
statement below
{ | ||
context.ReportDiagnostic(Diagnostic.Create( | ||
TestCaseUsageAnalyzer.CreateDescriptor( | ||
TestCaseUsageAnalyzerConstants.AsyncVoidMessage), |
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.
Shouldn't this also be mentioned in SupportedDiagnostics
?
{ | ||
class TestCaseUsageAnalyzerTestsAnalyzeWhenReturnTypeIsAsyncVoid | ||
{ | ||
[Test] |
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.
Perhaps we will - at some time - need a test for both cases. But for now I think it is fine
Hi @304NotModified But my "plan" is as follows:
I've also added some minor comments in the code, but please don't spend time on them before I've pushed the skeleton for #48 |
No problem! It's the nature of open source IMO 😄
Could we create subclasses etc? e.g. a base class for the sharing functionality and the a subclass for Test attribute and and for testcase attribute?
If I need to rebase stuff, no problem. Just let me know |
@304NotModified Once more I've left this hanging for much too long, and I'm sorry about that. |
When I found time (i'm also very busy these days), I will create a new PR with the changes! |
Fixes #7