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.

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

Add support for WorkItemAttribute analyzer #2180

Closed

Conversation

cvpoienaru
Copy link
Member

Add support for WorkItemAttribute analyzer

@cvpoienaru cvpoienaru enabled auto-merge (squash) January 26, 2024 16:18
Copy link
Member

@Evangelink Evangelink left a comment

Choose a reason for hiding this comment

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

I'll fix MSTEST0007 and merge this one right after. Thanks for the work here @cvpoienaru.

@@ -198,7 +198,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MSTest.Performance.Runner",
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Playground", "samples\Playground\Playground.csproj", "{8A41B37E-0732-4F28-B214-A44233B447FE}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "MSTest.Acceptance.IntegrationTests", "test\IntegrationTests\MSTest.Acceptance.IntegrationTests\MSTest.Acceptance.IntegrationTests.csproj", "{BCB42780-C559-40B6-8C4A-85EBC464AAA8}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "MSTest.Acceptance.IntegrationTests", "test\IntegrationTests\MSTest.Acceptance.IntegrationTests\MSTest.Acceptance.IntegrationTests.csproj", "{BCB42780-C559-40B6-8C4A-85EBC464AAA8}"
Copy link
Member

Choose a reason for hiding this comment

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

Was it some automatic change?


Rule ID | Category | Severity | Notes
--------|----------|----------|-------
MSTEST0010 | Usage | Info | WorkItemAttributeOnTestMethodAnalyzer
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this one 8 as it will be the first I merge (after #2176). Also, it seems that the rule is also not handling the notes part correctly so we will need to fix it by hand copying the rule description.

@@ -11,4 +11,5 @@ internal static class DiagnosticIds
public const string PublicTypeShouldBeTestClassRuleId = "MSTEST0004";
public const string TestContextShouldBeValidRuleId = "MSTEST0005";
public const string AvoidExpectedExceptionAttributeRuleId = "MSTEST0006";
public const string WorkItemAttributeOnTestMethodRuleId = "MSTEST0010";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public const string WorkItemAttributeOnTestMethodRuleId = "MSTEST0010";
public const string WorkItemAttributeOnTestMethodRuleId = "MSTEST0008";

<value>[WorkItem] can only be set on methods marked with [TestMethod]</value>
</data>
<data name="WorkItemAttributeOnTestMethodAnalyzerTitle" xml:space="preserve">
<value>WorkItemAttribute should be set on TestMethod</value>
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking that maybe we want to go with Invalid usage of [WorkItem].

{
if (methodAttribute.AttributeClass.Inherits(testMethodAttributeSymbol))
{
hasTestMethodAttribute = true;
Copy link
Member

Choose a reason for hiding this comment

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

Probably worth adding a continue; here so we don't also do the next check.

@Evangelink
Copy link
Member

@cvpoienary Can you please rebase to use the helper from #2176 (and use naming similar to what I did)

@Evangelink
Copy link
Member

As discussed, we will close this rule and provide a single rule that would check all the attributes that should only be set on test methods.

Thanks for the contribution @cvpoienaru

@Evangelink Evangelink closed this Jan 30, 2024
auto-merge was automatically disabled January 30, 2024 18:16

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants