Analyzers do not work with GlobalUsings

[bart-vmware]

Description

When using global usings, analyzers no longer report any diagnostics.

Expected behavior:

Be able to use global usings, either:

• From a source file GlobalUsings.cs that contains:

  global using Xunit;
  global using FluentAssertions;
  

  with the next snippet in project file:

  <ItemGroup>
    <Compile Include="GlobalUsings.cs" />
  </ItemGroup>
  

• From project file directly:

  <ItemGroup>
    <Using Include="FluentAssertions" />
    <Using Include="Xunit" />
  </ItemGroup>
  

Versions

• Fluent Assertions: 6.7.0
• Fluent Assertions Analyzers: 0.17.2
• .NET 6

[Meir017]

So basically if the global using file contains the testing library we should not perform the check on the namespace anymore. I think this should be implemented using some sort of a caching/singleton object in order to not scan the entire project each time

[bart-vmware]

I don't think it should ever look at namespace imports. For example, it won't work on the next file:

// No using

[Xunit.Fact]
public void SomeTest()
{
}

And I suspect the same applies to the following:

using XUnit.Fact as XFact;

[XFact]
public void SomeTest()
{
}

The GlobalUsings.cs is just a source file like any other, so it may contain constructs like #if/#endif, using static etc.

The above are perhaps corner cases. But with the introduction of global usings, the assumption that a namespace import must exist in the file no longer holds.

[bart-vmware]

In my case, the namespace imports are in a .props file, which is conditionally imported from another props file, which is imported from the .csproj.

[bart-vmware]

And to complicate it some more, I've once had to use:

<PropertyGroup>
  <ImplicitUsings>enable</ImplicitUsings>
</PropertyGroup>

<ItemGroup>
  <Using Remove="System.Threading" />
</ItemGroup>

to avoid numerous namespace clashes with my own Task class.

[Meir017]

@bart-vmware part of the goals of this package is to encourage best practices when writing tests. so for the example:

using XUnit.Fact as XFact;

[XFact]
public void SomeTest()
{

the solution would be to use

using XUnit;

[Fact]
public void SomeTest()
{

The csproj example will probably not be supported here as well

[bart-vmware]

Isn't the goal to promote the use of FluentAssertions over Xunit/MSTest assertions?

The using ... as construct is a C# notation unrelated to that, so it should not matter. There are numerous analyzers out there to promote general best practices (and I've written many myself), but I've never seen one that suggests to avoid namespace aliases. They are simply unavoidable in some cases. The C# language supports aliases, so the analyzer should just be able to deal with them.

A correct namespace check implementation would look at the symbol level, which abstracts these things away. But that's more expensive than syntax analysis. However, running on every method body (which is what the analyzer does today) is quite expensive too.

A more efficient analyzer would register AnalyzeSyntaxNode with node type ExpressionStatementSyntax, instead of RegisterCodeBlockAction. Then there's no need to check for namespace imports, wherever they come from. Such an analyzer could cheaply walk up the parent node tree to find the containing method declaration (and handle expression-bodied members), then inspect its attributes to find [Fact], [Theory], [InlineData] etc. and bail out early. Another approach would be to check the involved types in the expression, such as XUnit.Assert and bail out early on that.

[Meir017]

A more efficient analyzer would register AnalyzeSyntaxNode with node type ExpressionStatementSyntax, instead of RegisterCodeBlockAction. Then there's no need to check for namespace imports, wherever they come from. Such an analyzer could cheaply walk up the parent node tree to find the containing method declaration (and handle expression-bodied members), then inspect its attributes to find [Fact], [Theory], [InlineData] etc. and bail out early. Another approach would be to check the involved types in the expression, such as XUnit.Assert and bail out early on that.

This is a great point! started #167

[bart-vmware]

Thinking about this some more... walking up the syntax tree won't be very reliable either. The next example wouldn't work:

[Fact]
public void TestRequestAB()
{
    var response = ExecuteGetRequest("/a?b=1");
    AssertSuccess(response);
}

[Fact]
public void TestRequestCD()
{
    var response = ExecuteGetRequest("/c?d=2");
    AssertSuccess(response);
}

private void AssertSuccess(HttpReponseMessage response)
{
    // shared assertions here, goes undetected...
    Assert.NotNull(response);
}

What probably works best is walking up the tree (constrained within the method body) and find an invocation of XUnit.Debug.*. In other words: inspecting the compile-time types of outer method calls.

XUnit itself ships with built-in analyzers, so perhaps you get some ideas from their code?

[bart-vmware]

Another source for inspiration: The Sonar analyzer for S2699: Tests should include assertions, which supports xUnit and FluentAssertions. These analyzers are widely-used and optimized for high performance in large codebases. Source code at https://github.com/SonarSource/sonar-dotnet/blob/18eb2f6d6c4655f8b259744ef210347d12ce8b94/analyzers/src/SonarAnalyzer.CSharp/Rules/TestMethodShouldContainAssertion.cs.

[bart-vmware]

Thanks for the fix, this is working correctly now.