Use stylecop in Test project #1090

brianpopow · 2020-01-22T19:07:27Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This PR will introduce stylecop rules to the TestProject. The rules are less strict than in the main project. Those are the rules:

<!--https://github.com/dotnet/roslyn/issues/18117-->
<Rule Id="AD0001" Action="None" />

<!--Missing XML comment for publicly visible type or member-->
<Rule Id="CS1591" Action="None" />

<!--Call 'ConfigureAwait(false)'-->
<Rule Id="RCS1090" Action="None" />
    
<!--XML comment analysis is disabled due to project configuration-->
<Rule Id="SA0001" Action="None" />

<!--Using directive should appear within a namespace declaration-->
<Rule Id="SA1200" Action="None" />

<!--Elements should be documented.-->
<Rule Id="SA1600" Action="None" />

<!--A field should not follow a method-->
<Rule Id="SA1201" Action="None" />

<!--Elements must be ordered by access-->
<Rule Id="SA1202" Action="None" />

<!--Constants must appear before fields-->
<Rule Id="SA1203" Action="None" />

<!--Static members should appear before non-static members-->
<Rule Id="SA1204" Action="None" />

<!--Field names must not contain underscore-->
<Rule Id="SA1310" Action="None" />

<!--Field should be private-->
<Rule Id="SA1401" Action="None" />

<!--A C# code file contains more than one unique type.-->
<Rule Id="SA1402" Action="None" />

<!--The last statement in a multi-line C# initializer or list is missing a trailing comma.-->
<Rule Id="SA1413" Action="None" />

<!--Partial elements should be documented.-->
<Rule Id="SA1601" Action="None" />

<!--Comments should end with a period. I like this but there's 3000+ errors to fix in ImageSharp-->
<Rule Id="SA1629" Action="None" /> 
    
<!--The XML documentation header for a C# constructor does not contain the appropriate summary text.-->
<Rule Id="SA1642" Action="None" />
    
<!--Do not use regions.-->
<Rule Id="SA1124" Action="None" />
    
<!--Constructor is missing documentation for one or more of its parameters.-->
<Rule Id="SA1611" Action="None" />

<!--A C# code file is missing a standard file header.-->
<Rule Id="SA1633" Action="Warning" />

Note to @JimBobSquarePants: I have added SA1642, SA1124, SA1611

…ests # Conflicts: # tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpegParseStreamOnly.cs # tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg_ImageSpecific.cs # tests/ImageSharp.Benchmarks/Samplers/Crop.cs # tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.cs # tests/ImageSharp.Tests/TestUtilities/ImageComparison/PixelDifference.cs # tests/ImageSharp.Tests/TestUtilities/Tests/TestEnvironmentTests.cs

antonfirsov · 2020-01-23T00:31:22Z

I expect painful conflicts with #1089.

JimBobSquarePants · 2020-01-23T00:50:53Z

Note to @JimBobSquarePants: I have added SA1642, SA1124, SA1611

How were the rules added? Any additional rules should be added to

https://github.com/SixLabors/SharedInfrastructure/blob/master/SixLabors.Tests.ruleset

and the submodule updated.

Regarding the addition of rules.

I want to keep copyright header in every file - We can do that globally via ctrl + .
If a constructor has documentation it should be complete. If you think it's not required simply delete all the constructor documentation and any warnings will go away. Bad docs are worse than no docs.
Regions can absolutely do one. I hate them 🤢

antonfirsov

TBH I expected much worse from StyleCop here, but most code changes seem to be positive. With several exceptions of course.

antonfirsov · 2020-01-23T00:56:53Z

tests/ImageSharp.Benchmarks/General/PixelConversion/PixelConversion_Rgba32_To_Bgra32.cs

-        struct C
+        private struct C
        {
-            public uint tmp3, tmp6, tmp9, tmp12, tmp15, tmp18, tmp21, tmp24;


I don't like the strict enforcement of the multi-line rule in low level code 😕 Definitely does not help readability here (irrelevant pieces taking more space).

Personally I don't like grouped declarations. It's too easy to set the variable to the wrong type.

its back to a single line: d59ca98

antonfirsov · 2020-01-23T01:02:54Z

tests/ImageSharp.Tests/Drawing/DrawImageTests.cs


        [Theory]
-        [WithTestPatternImages(200, 200, PixelTypes.Rgba32 | PixelTypes.Bgra32)]
+        [WithTestPatternImage(200, 200, PixelTypes.Rgba32 | PixelTypes.Bgra32)]


The plural form might make sense here, since the attribute is feeding an image for each requested pixel type.

changed with: f897ab6

antonfirsov · 2020-01-23T01:15:15Z

tests/ImageSharp.Tests/Formats/Jpg/Utils/ReferenceImplementations.LLM_FloatingPoint_DCT.cs

-                c1 = _mm_load_ps(s, 32);
-                Vector4 t3 = (c0 + c1);
-                Vector4 t4 = (c0 - c1);
+                Vector4 c0 = Mm_load_ps(s, 0);


Nope, stylecop, you failed terribly again!

This code has been kept deliberately similar to the original. and _mm_load_ps refers to the corresponding C++ intrinsic to emphasize compatibility with that implementation.

Stylecop hasn't failed here. The rule is good, this is an edge case.

The fix was based on a lack of critical information.

I would add the exception inline with a justification describing why this is important.

Agree, just trying to be dramatic 😄

The point is that we need to be careful with the automatic application of the rules. If something needs to be suppressed too often, then re-evaluate the rule. Fortunately this doesn't seem to be the case with the current set.

Agree, the rules are not fixed and as soon as one causes significant issue we should definitely exclude it.

All I'm looking for by adding rules is a little bit more structure to our tests. A lot of different people have submitted significant parts of code now and it's become, for me, at least difficult to follow at times.

The less draconian the ruleset the better. Let's just be careful to not throw the baby out with the bathwater.

i have reverted it back, to be more like the original code again: 94dc54b

antonfirsov · 2020-01-23T01:24:46Z

tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg_ImageSpecific.cs

        [Params(
            TestImages.Jpeg.BenchmarkSuite.Lake_Small444YCbCr,
            TestImages.Jpeg.BenchmarkSuite.BadRstProgressive518_Large444YCbCr,
+            /* The scaled result for the large image "ExifGetString750Transform_Huge420YCbCr"


The /* */ comments are harder to work with in most C# IDE-s. Was the change necessary because of the rule? If yes, then I'm afraid that enforcing it may affect our test-code productivity. Or even worse: not placing comments where they should be present.

I'd have added the spaces.

There are two conflicting rules here:

SA1515 Single-line comment should be preceded by blank line

SA1115 The parameter should begin on the line after the previous parameter.

I have changed it back now as it was before with an exception for SA1115, see commit: d59ca98

antonfirsov · 2020-01-23T01:35:40Z

tests/ImageSharp.Tests/TestUtilities/ImageProviders/ITestImageProvider.cs

@@ -25,29 +27,32 @@ public interface ITestImageProvider
    /// <summary>
    /// Provides <see cref="Image{TPixel}" /> instances for parametric unit tests.
    /// </summary>
-    /// <typeparam name="TPixel">The pixel format of the image</typeparam>
+    /// <typeparam name="TPixel">The pixel format of the image.</typeparam>
    public abstract partial class TestImageProvider<TPixel> : ITestImageProvider


The file name is misleading now, we should either always move classes to a separate files, or change the rules.

In this instance, yes I'd probably move. The relaxing of the rule is designed to allow things like combining enums and classes in the same file

I have moved the interface to a separate file

antonfirsov · 2020-01-23T01:38:37Z

If a constructor has documentation it should be complete. If you think it's not required simply delete all the constructor documentation and any warnings will go away.

@JimBobSquarePants what do you mean by completeness?
For me, obvious <param > docs do not add any value. We can omit them while still documenting life-saving non-trivial information in <summary>.

JimBobSquarePants · 2020-01-23T02:12:51Z

@antonfirsov Constructor docs are traditionally.

Initializes a new instance of the <see cref="MyType" /> class/struct.

IMO the params are the important part.

antonfirsov · 2020-01-23T02:32:19Z

IMO the params are the important part.

In some cases yes. But mostly it becomes <param name="value">The value.</param>. Sebastian Aaltonen has a good point on why is this wrong. (One of the rare cases when I agree with a game developer actually.)

I wish that canonical text was not a thing with constructors :) It takes a whole sentence before the important stuff begins. This is wasted time for both doc writers and readers.

…tation

brianpopow

How were the rules added? Any additional rules should be added to

I missed that it is a submodule, i just edited SixLabors.Tests.ruleset 😕

Regarding the addition of rules.

I want to keep copyright header in every file - We can do that globally via ctrl + .

If a constructor has documentation it should be complete. If you think it's not required simply delete all the constructor documentation and any warnings will go away. Bad docs are worse than no docs.

Regions can absolutely do one. I hate them 🤢

I have removed the regions, see commit: 6c40593

For SA1642, the constructor documentation: I think its not much helpful if there is just a standard text for the constructor, its better if there is some meaningful info or nothing at all.

For SA1611, Constructor is missing parameter documentation: Im here on anton's side of the argument. Often its just as he said an meaningless "The value" comment. Besides that there are more than 4k warnings were i cannot add meaningful comment, because i have not written the tests.

I will change the submodule with the rules, once we agree on the rules to apply.

dlemstra · 2020-01-23T15:09:36Z

tests/ImageSharp.Benchmarks/Codecs/Jpeg/DecodeJpeg_ImageSpecific.cs

-             TestImages.Jpeg.BenchmarkSuite.ExifGetString750Transform_Huge420YCbCr,
-             */
+
+#pragma warning disable SA1115 // Parameter should follow comma


Should we not just disable these kind of rules instead?

SA1115 is now disabled

…ests # Conflicts: # tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Baseline.cs # tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Progressive.cs # tests/ImageSharp.Tests/Memory/Alocators/ArrayPoolMemoryAllocatorTests.cs # tests/ImageSharp.Tests/PixelFormats/PixelOperations/PixelOperationsTests.cs # tests/ImageSharp.Tests/TestUtilities/ImageProviders/TestImageProvider.cs

…eSharp.Tests.ProfilingSandbox

brianpopow · 2020-01-30T15:50:30Z

I have some trouble with the ProfilingSanbox project. There are some errors and alot of wanings im not sure how to resolve.

The errors look like this:
Error CS0121 The call is ambiguous between the following methods or properties: 'SixLabors.ImageSharp.Tests.TestUtils.IsEquivalentTo<TPixel>(SixLabors.ImageSharp.Image<TPixel>, SixLabors.ImageSharp.Image<TPixel>, bool)' and 'SixLabors.ImageSharp.Tests.TestUtils.IsEquivalentTo<TPixel>(SixLabors.ImageSharp.Image<TPixel>, SixLabors.ImageSharp.Image<TPixel>, bool)' ImageSharp.Tests.ProfilingSandbox (net472), ImageSharp.Tests.ProfilingSandbox (netcoreapp2.1), ImageSharp.Tests.ProfilingSandbox (netcoreapp3.1) E:\vsprojekte\ImageSharp\tests\ImageSharp.Tests\TestUtilities\Tests\TestUtilityExtensionsTests.cs 59

And the warnings:
Warning CS0436 The type 'ApproximateColorSpaceComparer' in 'E:\vsprojekte\ImageSharp\tests\ImageSharp.Tests\Colorspaces\Conversion\ApproximateColorspaceComparer.cs' conflicts with the imported type 'ApproximateColorSpaceComparer' in 'SixLabors.ImageSharp.Tests, Version=0.0.1.0, Culture=neutral, PublicKeyToken=null'. Using the type defined in 'E:\vsprojekte\ImageSharp\tests\ImageSharp.Tests\Colorspaces\Conversion\ApproximateColorspaceComparer.cs'. ImageSharp.Tests.ProfilingSandbox (net472), ImageSharp.Tests.ProfilingSandbox (netcoreapp2.1), ImageSharp.Tests.ProfilingSandbox (netcoreapp3.1) E:\vsprojekte\ImageSharp\tests\ImageSharp.Tests\Colorspaces\Conversion\RgbAndYCbCrConversionTest.cs 20

@JimBobSquarePants have you seen those errors here before? Any ideas how to resolve them would be very welcome.

JimBobSquarePants · 2020-01-31T01:23:17Z

src/Directory.Build.props

    <InternalsVisibleTo Include="ImageSharp.Benchmarks" />
-    <InternalsVisibleTo Include="SixLabors.ImageSharp.Sandbox46" />
    <InternalsVisibleTo Include="SixLabors.ImageSharp.Tests" />
+    <InternalsVisibleTo Include="ImageSharp.Tests.ProfilingSandbox" />


Isn't this your breaking change?

i have added this because i was getting more than 2k errors of Error CS0122 '...' is inaccessible due to its protection level

That's because you're including all the files from the test project in the sandbox.

https://github.com/SixLabors/ImageSharp/pull/1090/files#diff-791cc76ca5e2665ba784771043fd9123R17

We don't do that anymore. The sandbox project simply references the test one.

That's because you're including all the files from the test project in the sandbox.

https://github.com/SixLabors/ImageSharp/pull/1090/files#diff-791cc76ca5e2665ba784771043fd9123R17

We don't do that anymore. The sandbox project simply references the test one.

great, thank you! I was not aware of that. I have the removed the tests from the profiling sandbox, now it work works. See: commit: 0bce7af

… to ImageSharp.Tests.ProfilingSandbox" This reverts commit 9f181a3.

codecov · 2020-01-31T12:43:48Z

Codecov Report

Merging #1090 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1090   +/-   ##
=======================================
  Coverage   81.53%   81.53%           
=======================================
  Files         704      704           
  Lines       29384    29384           
  Branches     3290     3290           
=======================================
  Hits        23957    23957           
  Misses       4734     4734           
  Partials      693      693

Flag	Coverage Δ
#unittests	`81.53% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
src/ImageSharp/Formats/Gif/GifDecoderCore.cs	`82.83% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Gif/GifMetadata.cs	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bce7af...1f3311b. Read the comment docs.

src/ImageSharp/Primitives/ColorMatrix.cs

brianpopow added 3 commits January 22, 2020 16:57

Fix stylecop warnings in the test project

43cbdab

Use using declarations to reduce nesting

38454f8

antonfirsov reviewed Jan 23, 2020

View reviewed changes

brianpopow added 6 commits January 23, 2020 13:09

Remove regions

6c40593

Change WithTestPatternImageAttribute to WithTestPatternImagesAttribute

f897ab6

Rename method names again to be the same as in the reference implemen…

94dc54b

…tation

Defining the tmp vars in a single line again

715de4c

Move ITestImageProvider to a separate file

8a6797c

Revert comment as it was before, add exception for SA1115

d59ca98

brianpopow commented Jan 23, 2020

View reviewed changes

dlemstra reviewed Jan 23, 2020

View reviewed changes

brianpopow added 4 commits January 30, 2020 15:13

Update external for the changed test ruleset

076a2bc

Fix some leftover stylecop warnings

6f0e6e9

Change InternalsVisibleTo from SixLabors.ImageSharp.Sandbox46 to Imag…

9f181a3

…eSharp.Tests.ProfilingSandbox

JimBobSquarePants reviewed Jan 31, 2020

View reviewed changes

brianpopow and others added 3 commits January 31, 2020 13:25

Revert "Change InternalsVisibleTo from SixLabors.ImageSharp.Sandbox46…

5ff7bb7

… to ImageSharp.Tests.ProfilingSandbox" This reverts commit 9f181a3.

Remove including tests into the profiling sandbox

0bce7af

Merge branch 'master' into feature/stylecopTests

2c7e253

JimBobSquarePants reviewed Jan 31, 2020

View reviewed changes

src/ImageSharp/Primitives/ColorMatrix.cs Show resolved Hide resolved

Revert using declaration changes for until we establish guidelines.

15a7a55

Add more rule exemptions and prevent blanket using recommendations

1f3311b

JimBobSquarePants merged commit 2cf2e46 into SixLabors:master Feb 2, 2020

JimBobSquarePants added the codequality label Apr 24, 2020

JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 24, 2020

Uh oh!

Use stylecop in Test project #1090

Use stylecop in Test project #1090

Uh oh!

Conversation

brianpopow commented Jan 22, 2020

Prerequisites

Description

Uh oh!

antonfirsov commented Jan 23, 2020

Uh oh!

JimBobSquarePants commented Jan 23, 2020

Uh oh!

antonfirsov left a comment

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!

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!

antonfirsov Jan 23, 2020 • 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!

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!

antonfirsov commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimBobSquarePants commented Jan 23, 2020

Uh oh!

antonfirsov commented Jan 23, 2020

Uh oh!

brianpopow left a comment

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!

brianpopow commented Jan 30, 2020

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!

codecov bot commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

antonfirsov Jan 23, 2020 •

edited

Loading

antonfirsov commented Jan 23, 2020 •

edited

Loading

codecov bot commented Jan 31, 2020 •

edited

Loading