Fixes false positives with public methods which are not allowed #82

4creators · 2017-09-22T17:17:26Z

as test methods in xunit: "public static" and "public abstract"

PublicMethodShouldBeMarkedAsTest greedily issues warning for 'public static' and 'public abstract'
methods although they can not be used as a test method in xunit framework. Introduced code changes
verify if public method is abstract or is static and if detected skip further checks.

Fixes xunit/xunit#1466

…d in xunit: static and abstract PublicMethodShouldBeMarkedAsTest greedily issues warning for 'public static' and 'public abstract' methods although they can not be used as a test method in xunit framework. Introduced code changes verify if public method is abstract or is static and if detected skip further checks.

dnfclas · 2017-09-22T17:17:30Z

@4creators,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

4creators · 2017-09-22T17:18:24Z

PTAL @marcind

bradwilson · 2017-09-22T17:38:44Z

src/xunit.analyzers.vsix/source.extension.vsixmanifest

+    <Prerequisites>
+        <Prerequisite Id="Microsoft.VisualStudio.Component.CoreEditor" Version="[15.0.26208.0,16.0)" DisplayName="Visual Studio core editor" />
+    </Prerequisites>
 </PackageManifest>


Please do not reformat files. The entire file looks changed because of spacing differences.

bradwilson · 2017-09-22T17:40:37Z

src/xunit.analyzers/PublicMethodShouldBeMarkedAsTest.cs


                    var method = (IMethodSymbol)member;
-                    if (method.MethodKind != MethodKind.Ordinary)
+                    if (method.MethodKind != MethodKind.Ordinary || method.IsStatic || method.IsAbstract)


xUnit.net does support public static test methods.

Yeah, I have tested initially and xunit failed to detect that but using xunit.analyzers.tests I got it as You said. My wrong

…anges in vsix manifest

marcind · 2017-09-22T20:40:11Z

src/xunit.analyzers.vsix/source.extension.vsixmanifest

 <PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011">
  <Metadata>
-    <Identity Id="xunit.analyzers..d09124d1-9c5e-40de-b1de-35120196c3e0" Version="1.0" Language="en-US" Publisher="Marcin Dobosz"/>
+    <Identity Id="xunit.analyzers..d09124d1-9c5e-40de-b1de-35120196c3e0" Version="1.0.1" Language="en-US" Publisher="Marcin Dobosz"/>


Why is this file changed at all? Please undo.

Thought that xunit has semantic versioning. Change in error/warning detection syntax changes behaviour so I have incremented build version change.

Obviously will change it as requested.

4creators · 2017-09-22T20:58:12Z

Addressed all review remarks
@bradwilson @marcind

jamesqo · 2017-09-25T14:34:55Z

src/xunit.analyzers/PublicMethodShouldBeMarkedAsTest.cs


                    var method = (IMethodSymbol)member;
-                    if (method.MethodKind != MethodKind.Ordinary)
+                    if (method.MethodKind != MethodKind.Ordinary || method.IsAbstract)


Is this ever true? We returned earlier if type.IsAbstract was true, so logically this must be a concrete type and none of its members can be abstract.

Maybe the best fix would be to remove the type.IsAbstract check above and keep this as-is.

Sorry for late replay, I was stuck with other work for a while.
This way it is set it can skip diagnostics in partially invalid code during code editing i.e. concrete class with abstract method defined or skip analysis in abstract class for public concrete methods.

OK, it's fine to leave this as-is then, but can you add a comment about it?

jamesqo · 2017-09-25T14:41:27Z

test/xunit.analyzers.tests/PublicMethodShouldBeMarkedAsTestTests.cs

+        {
+            var diagnostics = await CodeAnalyzerHelper.GetDiagnosticsAsync(analyzer,
+@"public abstract class TestClass {
+    [Xunit.Fact] public void TestMethod() { }


Nit: Rename to AbstractMethod, since it isn't a test method and it doesn't raise an error.

jamesqo · 2017-09-25T14:41:54Z

test/xunit.analyzers.tests/PublicMethodShouldBeMarkedAsTestTests.cs

+}");
+
+            Assert.Empty(diagnostics);
+        }


Would be good to also add this test:

[Fact] public async void DoesNotFindErrorForPublicAbstractMethodMarkedWithFact() { var diagnostics = await CodeAnalyzerHelper.GetDiagnosticsAsync(analyzer, @"public abstract class TestClass { [Xunit.Fact] public void TestMethod() { } [Xunit.Fact] public abstract void AbstractTestMethod(); }"); Assert.Empty(diagnostics); }

I can't test right now but that should work fine at runtime, since the [Fact] attribute is inherited on overriding members.

OK it makes sense

…o fixpublicmethod

4creators · 2017-10-15T13:44:50Z

PTAL
@bradwilson @marcind @jamesqo
I left one unresolved problem to decide see: #82 (comment)

jamesqo · 2017-10-15T16:38:49Z

After you leave a comment LGTM

bradwilson

👍 once the requested comment has been made

…tics

4creators · 2017-10-15T17:57:47Z

OK All feedback addressed 😄
PTAL @bradwilson

marcind · 2017-10-15T19:03:34Z

Thanks for the submission

Bump test project dependencies - Castle.Core, Moq, and xUnit versions were all out of date - hold xunit.runner.visualstudio version back in .NET SDK test projects - can be moved to latest version after we stop testing on netcoreapp2.1 - add missing xunit.analyzers reference to System.Web.Razor.Test project - move all test projects to .NET v4.6.2 (a supported framework) - remove `netstandard` reference in System.Net.Http.Formatting.Test; not needed w/ new TFM & updated references - further separate build of Microsoft.TestCommon project when invoked from NetCore.Test project - special case `RestorePackages` for this case - add System.Net.Http references to avoid conflicting versions e.g. src/ and test/ TFMs differ React to changed xUnit APIs - adjust Microsoft.TestCommon code - nit: use `Array.Empty<byte>()` in `TranscodingStreamTests` - `TranscodingStream` `internal`s can be `private` instead Resolve xUnit issues new analyzers find - address xUnit2000 warnings - pass expected values to `Assert.Equal(...)` as correct (left) argument - make generic method types explicit to avoid `error CS0121: The call is ambiguous between the following methods or properties: ...` - note: cannot remove unnecessary xUnit1013 suppression - related bug (xunit/xunit#1466) apparently not fixed in 1.0.0 analyzers package - was xunit/xunit.analyzers#82 fix (in 2017) insufficient? React to new Moq changes - avoid `ProtectedMock\`1.ThrowIfPublicMethod(...)` `throw`ing - use new overloads introduced in the Moq 4.11.0 release - adjust to Moq hiding the `ObjectProxy` type better - update `ControllerContext` mocking to avoid NREs - setting `HttpContext.User` on the `Object` left `get` value `null` (did nothing) - nit: use `SetupGet(...)` for another property

dnfclas added the cla-already-signed label Sep 22, 2017

bradwilson requested changes Sep 22, 2017

View reviewed changes

Fixes addressing review, support for public static, no white space ch…

4b59d5b

…anges in vsix manifest

marcind reviewed Sep 22, 2017

View reviewed changes

Revert vsix version change as requested in code review

50037e9

jamesqo reviewed Sep 25, 2017

View reviewed changes

4creators added 2 commits October 15, 2017 15:36

Merge branch 'master' of https://github.com/xunit/xunit.analyzers int…

45ef7c6

…o fixpublicmethod

Address code review feedback, add test, rebase to master HEAD

22ba739

bradwilson approved these changes Oct 15, 2017

View reviewed changes

Add comment explaining redundant usage of IsAbstract check in diagnos…

9ccd153

…tics

marcind merged commit cdf2f86 into xunit:master Oct 15, 2017

dougbu mentioned this pull request Jan 30, 2023

Bump test project dependencies aspnet/AspNetWebStack#383

Merged

Fixes false positives with public methods which are not allowed #82

Fixes false positives with public methods which are not allowed #82

Uh oh!

Conversation

4creators commented Sep 22, 2017

Uh oh!

dnfclas commented Sep 22, 2017

Uh oh!

4creators commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesqo Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

4creators commented Oct 15, 2017

Uh oh!

jamesqo commented Oct 15, 2017

Uh oh!

bradwilson left a comment

Choose a reason for hiding this comment

Uh oh!

4creators commented Oct 15, 2017

Uh oh!

marcind commented Oct 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jamesqo Sep 25, 2017 •

edited

Loading

4creators Oct 15, 2017 •

edited

Loading