Autofixture: Fix code analysis configuration

[sshushliapin]

I was about to push the issue #420 fix for xUnit.net library but it appeared that the build fails if I run build.cmd (everything is OK in VS).

After some research I found out that the Code Analysis rules are enabled on the xUnit.net project:

... but disabled on the xUnit.net2:

Before moving forward with the #420 fixes, I'd like to fix the code analysis configuration. There are couple of questions:

1. Does it make sense to enable code analysis on build (now disabled) so that it reports the same set of errors in VS and Console
   
2. There are two AutoFixture.ruleset files, both created about 5 years ago:
   
   
   
   • SrcAutoFixture.ruleset
   
   
   • Src\AutoFixtureAutoFixture.ruleset
   
   

The first one is almost empty and used in the AutoNSubstitute project only. The second one contains full set of rules configured and used everywhere (except xUnit.net2 ;)). I'd nuke the first one.

[sshushliapin]

In order to fix issue #420 for xUnit.net (PR #599) I had to change the CustomizeAttributeComparer a bit to avoid the following code analysis error:

  1) Building S:\autofixture\Src\AutoFixture.xUnit.net.sln failed with exitcode 1.
  2) CA1800: S:\autofixture\Src\AutoFixture.xUnit.net\CustomizeAttributeComparer.cs(14,0): Microsoft.Performance : 'x', a parameter, is cast to type 'FrozenAttribute' multiple times in method 'CustomizeAttributeComparer.Compare(CustomizeAttribute, CustomizeAttribute)'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction.
  3) CA1800: S:\autofixture\Src\AutoFixture.xUnit.net\CustomizeAttributeComparer.cs(14,0): Microsoft.Performance : 'y', a parameter, is cast to type 'FrozenAttribute' multiple times in method 'CustomizeAttributeComparer.Compare(CustomizeAttribute, CustomizeAttribute)'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction.

I tried to configure the xUnit.net2 library in the same way but for some reason it never fails with the rule validation error... Just wondering why... The only difference I can see at the moment is the Target Framework version (xUnit - 4; xUnit2 - 4.5), but I'm not sure if that might be the reason.

[sshushliapin]

That might sound strange, but removing and re-adding all the projects in the AutoFixture.xUnit.net2.sln has solved the problem. Now build.cmd reports the following 10 (!) issues for xUnit2:

  1) Building S:\autofixture\Src\AutoFixture.xUnit.net2.sln failed with exitcode 1.
  2) CA1704: (-1,0): Microsoft.Naming : Correct the spelling of 'Ploeh' in namespace name 'Ploeh.AutoFixture.Xunit2'.
  3) CA1704: (-1,0): Microsoft.Naming : Correct the spelling of 'Xunit' in namespace name 'Ploeh.AutoFixture.Xunit2'.
  4) CA1704: (-1,0): Microsoft.Naming : Correct the spelling of 'Ploeh' in assembly name 'Ploeh.AutoFixture.Xunit2.dll'.
  5) CA1704: (-1,0): Microsoft.Naming : Correct the spelling of 'Xunit' in assembly name 'Ploeh.AutoFixture.Xunit2.dll'.
  6) CA1725: S:\autofixture\Src\AutoFixture.xUnit.net2\AutoDataAttribute.cs(92,0): Microsoft.Naming : In member 'AutoDataAttribute.GetData(MethodInfo)', change parameter name 'methodUnderTest' to 'testMethod' in order to match the identifier as it has been declared in 'DataAttribute.GetData(MethodInfo)'.
  7) CA1725: S:\autofixture\Src\AutoFixture.xUnit.net2\CompositeDataAttribute.cs(69,0): Microsoft.Naming : In member 'CompositeDataAttribute.GetData(MethodInfo)', change parameter name 'methodUnderTest' to 'testMethod' in order to match the identifier as it has been declared in 'DataAttribute.GetData(MethodInfo)'.
  8) CA1800: S:\autofixture\Src\AutoFixture.xUnit.net2\CustomizeAttributeComparer.cs(14,0): Microsoft.Performance : 'x', a parameter, is cast to type 'FrozenAttribute' multiple times in method 'CustomizeAttributeComparer.Compare(CustomizeAttribute, CustomizeAttribute)'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction.
  9) CA1800: S:\autofixture\Src\AutoFixture.xUnit.net2\CustomizeAttributeComparer.cs(14,0): Microsoft.Performance : 'y', a parameter, is cast to type 'FrozenAttribute' multiple times in method 'CustomizeAttributeComparer.Compare(CustomizeAttribute, CustomizeAttribute)'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction.
 10) CA1704: (-1,0): Microsoft.Naming : Correct the spelling of 'Enumerables' in type name 'FavorEnumerablesAttribute'.

[ploeh]

A couple of years ago, 'something' happened to some of the code analysis settings that messed up things. IIRC, it was triggered by a change in Visual Studio from one version to the next.

We scrambled to correct the settings, but apparently we missed some. They ought to all use the same code analysis rules file, except the F# projects.

[ploeh]

Is there more work to be done, or can we now close this issue?

[sshushliapin]

I've fixed the code analysis configuration for all the test framework glue libraries. There might be more work that probably also needs to be done.

For instance, in the AutoFakeItEasy project there is no Verify build configuration at all.. I'm ok with fixing this (and all the other solutions if necessary).

I also noticed that the Verify build configuration is not configured for projects with unit tests. Is that by design? Shouldn't the test project have the same configuration as the production ones?

[ploeh]

I'm ok with fixing this (and all the other solutions if necessary).

:+1:

I also noticed that the Verify build configuration is not configured for projects with unit tests. Is that by design? Shouldn't the test project have the same configuration as the production ones?

They could have, but it isn't important. Perhaps a brief summary is in order.

The reason we have the Verify build configuration is that we use static code analysis. It's turned off in Debug mode, though, because it's _slow_.

Why not turn it on in Release mode, then? Aesthetics, purely. When static code analysis is turned on, all the suppression information gets compiled into the executables, and I just thought it was 'ugly' if you were to ever decompile the assemblies (with e.g. Reflector, or a similar tool).

Why you'd want to decompile the assemblies of a piece of open source software, I don't know, but back when I started AutoFixture, OSS wasn't the norm, and I was so used to thinking in terms of decompilation that it bothered me if the suppression metadata was included in the published assemblies. Therefore, static code analysis is also turned off in Release mode.

That's the reason I created the Verify mode. It's a copy of the Release build configuration, but with static code analysis turned on.

While we review the unit tests, we've never subjected them to code analysis, so there's no reason to have a separate build configuration for these projects.

Perhaps we should consider running code analysis on the test code as well. It is, after all, also code that we have to maintain. That's a slightly different discussion, though.

Unit test projects should use the Release build configuration in the Verify mode, unless we decide to turn on code analysis for them. I'm afraid that rule isn't consistently applied, however.

[sshushliapin]

Thanks for the detailed explanation, @ploeh! Couple of weeks ago I was about to ask what's the purpose of the Verify configuration but then I found some explanation that satisfied me here :)

Unit test projects should use the Release build configuration in the Verify mode

Could you please elaborate on this?

[ploeh]

Each project (.csproj) comes with a set of build configurations.

The intent is that all AutoFixture 'production' projects come with three configurations:

• _Debug_
• _Verify_
• _Release_

On the other hand, since we (currently) don't do static code analysis on unit tests, these projects need only define the two default configurations:

• _Debug_
• _Release_

Additionally, solutions (.sln) come with build configurations, which are named combination of project build configuration profiles. By default, Visual Studio defines two build configurations: _Debug_ and _Release_. In the _Debug_ solution configuration, all projects are built according to their _Debug_ configurations; in the _Release_ solution configurations, all projects are built with their _Release_ configurations.

When defining a custom solution build configuration like _Verify_, you must define which project build configuration to use for each project. For 'production' projects, we obviously choose the _Verify_ build configuration, but for the test projects, because there's no _Verify_ project build configuration, we choose _Release_:

The _Release_ configuration is the correct profile to choose, because _Verify_ ought to be a copy of _Release_ anyway, with the sole difference that static code analysis is turned on.

For unit tests, when static code analysis isn't turned on, there's no reason to have a _Verify_ configuration. It'd be an identical copy of the _Release_ configuration, and there's no reason to maintain two identical configurations.

[ploeh]

Is this work item done now, or is there any remaining work?

[sshushliapin]

Done.