Use reference assemblies instead of implementation assemblies

[silkfire]

The current way RoslynTestKit is adding core metadata references to the fixture results in the compilation engine always emitting compilation errors. This can be seen if the overridable property ThrowsWhenInputDocumentContainsError is set to true.

RoslynTestKit.RoslynTestKitException
Input document contains errors:
   TestDocument(7,9): error CS0012: The type 'Object' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
   TestDocument(7,1): error CS0012: The type 'Decimal' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Runtime, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
   TestDocument(9,31): error CS0012: The type 'Object' is defined in an assembly that is not referenced. You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'.

The reason this happens is because the wrong assemblies are being referenced. Only reference assemblies should be used in the compilation context, not implementation assemblies. In other words, System.Private.CoreLib.dll should never be referenced directly as that is not the way it is meant to be used.

The correct way is to first add <PreserveCompilationContext>true</PreserveCompilationContext> to your executable (that is, the test project that is referencing RoslynTestKit) and then use the NuGet package Microsoft.Extensions.DependencyModel to load all the relevant reference assemblies. This will make sure to not involve any implementation assemblies in the metadata references.

https://github.com/dotnet/core/issues/2082#issuecomment-442713181 https://stackoverflow.com/a/68121419/633098 https://www.nuget.org/packages/Microsoft.Extensions.DependencyModel/

I also took the freedom of upgrading the library dependencies to their respective latest versions and fixed some issues in the project file related to NuGet to avoid warnings.

[cezarypiatek]

Hi @silkfire

Thanks for sending this PR. I will review it later - I will need to run my existing test suites with this fix and verify the new behavior. This will probably solve the most annoying issue 👍

[cezarypiatek]

Some of my tests that rely on System.Collections.Generic.CollectionExtensions are failing. I need to investigate it.

[silkfire]

Okay let me know if I can be of any assistance to you.

[cezarypiatek]

I was thinking about your proposal. Using PreserveCompilationContext along with Microsoft.Extensions.DependencyModel results in loading all test project dependencies into a compilation context which is something that violates test isolation.

I'm not able to reproduce your original issues although I remember that I came across that in the past. I know this could be annoying but may I ask you to provide a minimal project that reproduces this issue? Here's a project that I've just created, there's a sample test and everything is working according to the expectations, no issues reported regarding the missing dependencies https://github.com/cezarypiatek/SampleRecordAnalyzer

[silkfire]

I was a bit of the same opinion as you, perhaps one can have some kind of smart filtering to not load all references at once... but not sure exactly how.

I'll look into creating a minimal repro sometime next week. Thanks for taking your time to look at this PR.

[silkfire]

I cloned your sample project but am getting some weird errors when running the test:

System.Reflection.ReflectionTypeLoadException : Unable to load one or more of the requested types.
Method 'get_CodeFixCategory' in type 'Microsoft.CodeAnalysis.PopulateSwitch.PopulateSwitchCodeFixProvider' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'RemoveDocumentAsync' in type 'Microsoft.CodeAnalysis.SolutionCrawler.AggregateIncrementalAnalyzer' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'RemoveDocumentAsync' in type 'Microsoft.CodeAnalysis.SolutionCrawler.IncrementalAnalyzerBase' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'DetermineDocumentsToSearchAsync' in type 'Microsoft.CodeAnalysis.ChangeSignature.DelegateInvokeMethodReferenceFinder' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'get_CodeFixCategory' in type 'Microsoft.CodeAnalysis.UseThrowExpression.UseThrowExpressionCodeFixProvider' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'get_CodeFixCategory' in type 'Microsoft.CodeAnalysis.UseExplicitTupleName.UseExplicitTupleNameCodeFixProvider' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'get_CodeFixCategory' in type 'Microsoft.CodeAnalysis.UseCoalesceExpression.UseCoalesceExpressionForNullableCodeFixProvider' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
Method 'get_CodeFixCategory' in type 'Microsoft.CodeAnalysis.UseCoalesceExpression.UseCoalesceExpressionCodeFixProvider' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.

[cezarypiatek]

What version of VisualStudio and dotnet core do you have installed?

[silkfire]

I'm using Visual Studio 17 Preview 4 and am running .NET 5 as is defined in the project.

The error stack starts with:

  ----> System.TypeLoadException : Method 'RemoveDocumentAsync' in type 'Microsoft.CodeAnalysis.Diagnostics.EngineV2.DiagnosticIncrementalAnalyzer' from assembly 'Microsoft.CodeAnalysis.Features, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.
   at System.Reflection.RuntimeModule.GetTypes(RuntimeModule module)
   at System.Reflection.RuntimeAssembly.get_DefinedTypes()
   at System.Composition.Hosting.ContainerConfiguration.<>c.<WithAssemblies>b__16_0(Assembly a)
   at System.Linq.Enumerable.SelectManySingleSelectorIterator`2.MoveNext()
   at System.Composition.TypedParts.TypedPartExportDescriptorProvider..ctor(IEnumerable`1 types, AttributedModelProvider attributeContext)
   at System.Composition.Hosting.ContainerConfiguration.CreateContainer()
   at Microsoft.CodeAnalysis.Host.Mef.MefHostServices.Create(IEnumerable`1 assemblies)
   at Microsoft.CodeAnalysis.Host.Mef.MefHostServices.get_DefaultHost()
   at Microsoft.CodeAnalysis.AdhocWorkspace..ctor()
   at RoslynTestKit.Utils.MarkupHelper.GetDocumentFromCode(String code, String languageName, IReadOnlyCollection`1 references, String projectName, String documentName) in D:\a\RoslynTestKit\RoslynTestKit\src\RoslynTestKit\Utils\MarkupHelper.cs:line 25
   at RoslynTestKit.Utils.MarkupHelper.GetDocumentFromMarkup(String markup, String languageName, IReadOnlyCollection`1 references, String projectName, String documentName) in D:\a\RoslynTestKit\RoslynTestKit\src\RoslynTestKit\Utils\MarkupHelper.cs:line 16
   at RoslynTestKit.AnalyzerTestFixture.HasDiagnostic(String markupCode, String diagnosticId) in D:\a\RoslynTestKit\RoslynTestKit\src\RoslynTestKit\AnalyzerTestFixture.cs:line 55
   at RecordAnalyzers.Test.SampleRecordAnalyzerTests.should_report_issue_for_record_equals_when_something() in D:\Projects\2021\SampleRecordAnalyzer\RecordAnalyzers.Test\SampleRecordAnalyzerTests.cs:line 38

[cezarypiatek]

This is totally unexpected and I have no idea what's going on.

Can you try to add the following property to the test class?

protected override IReadOnlyCollection<MetadataReference> References {get;} = new[]
{
        MetadataReference.CreateFromFile(Assembly.Load("System.Runtime").Location)
 };

[silkfire]

Hello, sorry for the late reply, I must've completely missed the update in my inbox. I tried adding the System.Runtime reference but it made no difference. It is strange though that I'm not getting this error in my other project. Maybe it's because this test project doesn't use the updated RoslynTestKit dependency from this PR?

Regarding your thoughts on test isolation, I agree with you. Is there some smarter way of filtering out the dependencies that actually are required?

Were you able to fix your failing System.Collections.Generic.CollectionExtensions tests?

[silkfire]

@cezarypiatek Any thoughts on my comments?

[cezarypiatek]

Sorry, I didn't have time to deal with it. I've just got a new clean setup of my dev env, maybe I managed to reproduce your issue. I will take a look at that on weekend.

[cezarypiatek]

Ok, I was able to reproduce and fix the issue. It was caused by some breaking changes between Roslyn major versions. I updated roslyn dependencies to the newest version and released a new version of RoslynTestKit. I've also updated the sample project. Can you try it now?

[silkfire]

It worked, thanks! I had to upgrade BuildTools package to v17 to get it to work because I'm using VS2022.

I was testing out my changes to RoslynTestKit the other day and realized it would be useful to have more control over how the sample code is packaged into a solution (AdhocWorkspace, etc). E.g. I'm in need of having Code A in one project, and Code B in another project, and that project is referenced by the first project. Also each individual project needs its own references. It's out of scope for this PR of course but I'm realizing that there goes a lot of work into building a proper test framework for Roslyn.

Also I discovered that they had made the completion test service provider internal in new version of CodeAnalysis because they're going the LSP route, so that one I had to remove from my project...

[cezarypiatek]

I will try to expose an extension point that allows taking control of how the solution is created as the current implementation handles only very simple scenarios.

[cezarypiatek]

I've release a new version with slightly re-organized internals. Now you can take full control of how the compilation is created by overriding CreateDocumentFromCode method. The default implementation looks as follows:

/// <summary>
///     Should create the compilation and return a document that represents the provided code
/// </summary>
protected virtual Document CreateDocumentFromCode(string code, string languageName, IReadOnlyCollection<MetadataReference> extraReferences)
{
    var frameworkReferences = CreateFrameworkMetadataReferences();

    var compilationOptions = GetCompilationOptions(languageName);

    return new AdhocWorkspace()
        .AddProject("TestProject", languageName)
        .WithCompilationOptions(compilationOptions)
        .AddMetadataReferences(frameworkReferences)
        .AddMetadataReferences(extraReferences)
        .AddDocument("TestDocument", code);
}

You can also change the default set of references by overriding CreateFrameworkMetadataReferences()

I would like to see your overrides, that could help me to improve this test kit.

[silkfire]

I've release a new version with slightly re-organized internals. Now you can take full control of how the compilation is created by overriding CreateDocumentFromCode method. The default implementation looks as follows:

/// <summary>
///     Should create the compilation and return a document that represents the provided code
/// </summary>
protected virtual Document CreateDocumentFromCode(string code, string languageName, IReadOnlyCollection<MetadataReference> extraReferences)
{
    var frameworkReferences = CreateFrameworkMetadataReferences();

    var compilationOptions = GetCompilationOptions(languageName);

    return new AdhocWorkspace()
        .AddProject("TestProject", languageName)
        .WithCompilationOptions(compilationOptions)
        .AddMetadataReferences(frameworkReferences)
        .AddMetadataReferences(extraReferences)
        .AddDocument("TestDocument", code);
}

You can also change the default set of references by overriding CreateFrameworkMetadataReferences()

I would like to see your overrides, that could help me to improve this test kit.

I'm not sure this signature would be useful as I need to be able to specify specific references per unique project.

[cezarypiatek]

By overriding this method you can create as many projects as you want with a separate set of dependencies for each one. If you provide an example structure I can try to help you to rewrite it into a specific setup code.

[silkfire]

Okay, but I'd need to implement my own fixture for that?

I was thinking something along these lines, but I believe the signature would have to be more flexible:

public static Document GetDocumentFromCodeReferencingAnotherProject(string code, string referencedCode, string languageName, IReadOnlyCollection<MetadataReference> references, IReadOnlyCollection<MetadataReference> referencedProjectReferences)
{
   var metadataReferences = CreateMetadataReferences(references);
   var referencedProjectMetadataReferences = CreateMetadataReferences(referencedProjectReferences);
   var compilationOptions = GetCompilationOptions(languageName);

   var workspace = new AdhocWorkspace();

   var referencedProject = workspace.AddProject("TestProject2", languageName)
                                    .WithCompilationOptions(compilationOptions)
                                    .AddMetadataReferences(referencedProjectMetadataReferences)
                                    .AddDocument("TestDocument2", referencedCode)
                                    .Project;

   return workspace.AddProject("TestProject", languageName)
                   .WithCompilationOptions(compilationOptions)
                   .AddProjectReference(new ProjectReference(referencedProject.Id))
                   .AddMetadataReferences(metadataReferences)
                   .AddDocument("TestDocument", code);
}

This is something I just added on the internal MarkupHelper by the way.

[cezarypiatek]

With the current design, you always need to inherit from the fixture class. This is something that I would like to get rid of in the future or at least provide wrappers for fixture classes to avoid the need for the inheritance - I'm already testing this approach in one of my projects and it works like a charm.

Regarding your example: you added code to both projects and you used referencedCode as a project name, I doubt it's what you really want to do...

[silkfire]

Yea sorry, was a bit too quick there. Updated it now.

Okay, that sounds awesome. I rather prefer static methods, completely agree with you there.

[cezarypiatek]

Static methods - definitely no. I was talking rather about something like that

RefactoringFixture inherits from CodeRefactoringTestFixtureand allowing to override different settings during the instantiation. This eliminates the need to inherit from CodeRefactoringTestFixture every time.

[silkfire]

Looks interesting. So the options approach will basically let the consumer construct their own Solution with any projects and respective documents they wish?

[cezarypiatek]

Correct. I even think about creating a DSL to facilitate that

[silkfire]

@cezarypiatek I was reading the README on the front page and realized that a few methods mentioned there don't exist in the API. Is this an oversight on your part? E.g. CreateDocumentFromMarkup and GetMarkerLocation.

[cezarypiatek]

Looks like the doc is a little bit outdated. Currently, there's a class called MarkupCode (internal) which is responsible for parsing markup (code with markers) and returning code and marker position https://github.com/cezarypiatek/RoslynTestKit/blob/5933dafadcf9585c61324afd610ed58190a62254/src/RoslynTestKit/CodeMarkup.cs#L7

Then you can create a document by calling CreateDocumentFromCode from the BaseTestFixture class.

[silkfire]

Right. It's just that the overload of HasDiagnostic that takes a Document instance requires a TextSpan and I don't know how to produce that without CodeMarkup.

[cezarypiatek]

Just use void HasDiagnostic(string markupCode, string diagnosticId). It will automatically search for the marker [||] in a provided markup

https://github.com/cezarypiatek/RoslynTestKit/blob/5933dafadcf9585c61324afd610ed58190a62254/src/RoslynTestKit/AnalyzerTestFixture.cs#L60

[silkfire]

I thought I needed that overload so I can supply a custom AdHocWorkspace with my own project structure as we discussed earlier?

[cezarypiatek]

In order to use custom Workspace, you had to override CreateDocumentFromCode but you don't need to call it directly - it's called by the test kit under the hood.

[silkfire]

Okay, but what do I pass to HasDiagnostic after overriding the CreateDocumentFromCode method?

[cezarypiatek]

As a markup you pass a code with a marker [| |] around a section where do you expect the reported diagnostic, something like that:

 public class TestMapper
 {
        IDataReader [|_reader|];
}

In this example you expect the diagnostic reported for span containing _reader

[silkfire]

I think you're missing my point here. I already perfectly know how that method works. It just doesn't seem to have any correlation with Document. Could you please provide an example? I'm still not able to achieve what I've described.

[cezarypiatek]

Under the hood, the test kit strips the markers from the markup and passes it to the CreateDocumentFromCode (which you should override if you want to use a custom workspace). Please inspect the implementation, it should help you to understand how it works.

[silkfire]

I understand that. I've built a custom method to populate a Document field that then is returned by CreateDocumentFromCode. But in that method I already provide the test code. Why do I need to provide the same test code again in void HasDiagnostic(string markupCode, string diagnosticId)?

[cezarypiatek]

You don't provide code via CreateDocumentFromCode. You should create there a document from the input parameter code.

protected virtual Document CreateDocumentFromCode(string code, string languageName, IReadOnlyCollection<MetadataReference> extraReferences)

[silkfire]

That doesn't make any sense. Could you provide an example? My tests are inside the test fixture with the overridden CreateDocumentFromCode.

[cezarypiatek]

The only example, that I have, it's a default implementation. Can you show a sample test class that you are trying to implement (with that duplication)?

[silkfire]

Ok, here's my code:

public class TestClass : AnalyzerTestFixture {
   private Document _sourceDocument;

   protected override string LanguageName => LanguageNames.CSharp;

   protected override DiagnosticAnalyzer CreateAnalyzer() => new MyAnalyzer();

   private void SetSourceDocument(string mainProjectName, string mainProjectDocumentName, string mainProjectDocumentCode, MetadataReference[] mainProjectReferences, string referencedProjectName, string referencedProjectDocumentName, string referencedProjectDocumentCode, MetadataReference[] referencedProjectReferences)
   {
      var compilationOptions = new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary);

      var workspace = new AdhocWorkspace();

      var referencedProject = workspace.AddProject(referencedProjectName, LanguageName)
                                       .WithCompilationOptions(compilationOptions)
                                       .AddMetadataReferences(referencedProjectReferences)
                                       .AddDocument(referencedProjectDocumentName, referencedProjectDocumentCode)
                                       .Project;

      _sourceDocument = workspace.AddProject(mainProjectName, LanguageName)
                                 .WithCompilationOptions(compilationOptions)
                                 .AddProjectReference(new ProjectReference(referencedProject.Id))
                                 .AddMetadataReferences(mainProjectReferences)
                                 .AddDocument(mainProjectDocumentName, mainProjectDocumentCode);
   }

   protected override Document CreateDocumentFromCode(string code, string languageName, IReadOnlyCollection<MetadataReference> extraReferences)
   {
      if (_sourceDocument == null) throw new InvalidOperationException($"The source document must be set with {nameof(SetSourceDocument)}() before executing tests.");

      return _sourceDocument;
   }

   [Fact]
   public void Test_method_should_trigger_diagnostic()
   {
      const string testDocument = @"
var variable = Math.[|Round|](50.23);";

      const string referencedDocument = @"";

      SetSourceDocument("TestProject", "TestDocument", testDocument, Array.Empty<MetadataReference>(), "ReferencedProject", "ReferencedDocumentClass", referencedDocument, Array.Empty<MetadataReference>());

      // Duplication of code here? testDocument is already provided in the SetSourceDocument method
      HasDiagnostic(testDocument, DiagnosticIds.SomeDiagnosticId);
   }
}

[cezarypiatek]

I would try something like that

public class TestClass : AnalyzerTestFixture
{
    private Project _sourceProject;
    private string _mainProjectDocumentName;

    protected override string LanguageName => LanguageNames.CSharp;

    protected override DiagnosticAnalyzer CreateAnalyzer() => new MyAnalyzer();

    private void SetSourceDocument(string mainProjectName, string mainProjectDocumentName,
        MetadataReference[] mainProjectReferences, string referencedProjectName,
        string referencedProjectDocumentName, string referencedProjectDocumentCode,
        MetadataReference[] referencedProjectReferences)
    {
        var compilationOptions = new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary);

        var workspace = new AdhocWorkspace();

        var referencedProject = workspace.AddProject(referencedProjectName, LanguageName)
                                         .WithCompilationOptions(compilationOptions)
                                         .AddMetadataReferences(referencedProjectReferences)
                                         .AddDocument(referencedProjectDocumentName, referencedProjectDocumentCode)
                                         .Project;

        _sourceProject = workspace.AddProject(mainProjectName, LanguageName)
            .WithCompilationOptions(compilationOptions)
            .AddProjectReference(new ProjectReference(referencedProject.Id))
            .AddMetadataReferences(mainProjectReferences);
        _mainProjectDocumentName = mainProjectDocumentName;

    }

    protected override Document CreateDocumentFromCode(string code, string languageName, IReadOnlyCollection<MetadataReference> extraReferences)
    {
        if (_sourceProject == null) throw new InvalidOperationException($"The source document must be set with {nameof(SetSourceDocument)}() before executing tests.");

        return _sourceProject.AddDocument(_mainProjectDocumentName, code); ;
    }

    [Fact]
    public void Test_method_should_trigger_diagnostic()
    {
        const string testDocument = @"
var variable = Math.[|Round|](50.23);";

        const string referencedDocument = @"";

        SetSourceDocument("TestProject", "TestDocument", Array.Empty<MetadataReference>(), "ReferencedProject", "ReferencedDocumentClass", referencedDocument, Array.Empty<MetadataReference>());

        HasDiagnostic(testDocument, DiagnosticIds.SomeDiagnosticId);
    }
}

I admit It's a little bit messy but this is a result of inheritable test fixtures. I have a few ideas on how to simplify this kind of tests, I will let you know when I publish some improvements.