Adapter 3.8.0 breaks when tests uses newer Mono.Cecil

[bording]

After updating to 3.8.0, I had a test project that disappeared from Test Explorer in Visual Studio. If I tried to use dotnet test to run the tests, I'd see that NUnit "failed to load" the assembly and that "No test is available".

I've been able to reduce the problem down to a simple repro project, which I've attached to the issue.

The test assembly is referencing Mono.Cecil 0.10.0-beta6, with a simple test that uses one of its types. If I use 3.8.0-alpha1 of the adapter, the test is discovered properly, and the test passes. If I instead use 3.8.0, the test is still discovered, but when I run it, it fails with the following error:

Message: System.IO.FileLoadException : Could not load file or assembly 'Mono.Cecil, Version=0.10.0.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

While still using 3.8.0, if I then add an additional package that uses Mono.Cecil more extensively, PublicApiGenerator 6.1.0-beta2, the test no longer appears in the Test Explorer window at all, and I can't run the test from the command line with dotnet test.

Using this package with 3.8.0-alpha1, everything works correctly, The test is discovered and passes.

TestAdapterProblem.zip

Versions:

• NUnit 3.7.1
• NUnit3TestAdapter 3.8.0
• VS 2017 Professional 15.2 26430.16 (also tested on 15.3.0 Preview 4 with .NET Core 2.0 Preview 2.0 installed)
• .NET Framework 4.5.2 - 4.7 (didn't try anything earlier)

[rprouse]

The only thing that changed between the alpha and the 3.8 release was I updated Mono.Cecil to 0.10.0-beta6 from beta5 in the .NET Core version of the adapter. The .NET Framework version remains the same.

[bording]

The only thing that changed between the alpha and the 3.8 release was I updated Mono.Cecil to 0.10.0-beta6 from beta5 in the .NET Core version of the adapter. The .NET Framework version remains the same.

It looks like the .NET Framework version did change as well, from 0.9.6.1 to 0.9.6.4.

I'm not sure why that would make a difference, but I'm definitely seeing broken behavior now.

[rprouse]

This is one issue with NUnit taking a dependency on Mono.Cecil and why we generally do not take dependencies on 3rd party libraries.

What happens if you run your tests targeting .NET Framework 4.5+ using the NUnit Console instead of dotnet test? Same error? This might be more appropriate as an NUnit Engine issue rather than an adapter issue.

[rprouse]

I see from the referenced issue that the workaround is to include a specific PackageReference to Mono.Cecil.

I might be misdiagnosing this issue. Is any of your code using Mono.Cecil?

[bording]

We do use Mono.Cecil in our tests for various things: https://github.com/Particular/NServiceBus/blob/develop/src/NServiceBus.Core.Tests/NServiceBus.Core.Tests.csproj#L26

The repro project I included here also has a direct reference, which mimics the test project with the problem fairly well.

[bording]

What happens if you run your tests targeting .NET Framework 4.5+ using the NUnit Console instead of dotnet test? Same error? This might be more appropriate as an NUnit Engine issue rather than an adapter issue.

Using the TestAdapterProblem repro I included here, if I'm using 3.8.0 of the test adapter, I get the following when I use the console runner:

1) Error : TestAdapterProblem.Test.TestMethod
System.IO.FileLoadException : Could not load file or assembly 'Mono.Cecil, Version=0.10.0.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)
   at TestAdapterProblem.Test.TestMethod()

If I use the alpha, then everything works correctly and the test passes.

[vvv444]

I also bumped into this problem. The test adapter overwrote my (newer) Mono.Cecil in the output directory and my tests stopped working.

However, I think I found a solution. I changed the configuration so that test adapter won't copy any files into the output directory (I set all CopyToOutputDirectory in props files to Never). This seems to have solved the problem.

Can this be a solution for all? What is the reason for the test adapter to be copied into the output directory at first place?

[CharliePoole]

This is a surprise to me. The adapter should be neither referenced from your code nor copied to output as far as I understand it.

@OsirisTerje Back in October, 2016, I modified the adapter from use of the lib directory to the tools directory, in order to eliminate unneeded references and copying. When you use the lib directory, a reference is automatically added to the project but the adapter is, in fact, a tool and shouldn't be referenced - just made available.

Subsequently, you switched to use of a build directory. I haven't seen any documentation on what is done with stuff in that directory, but it sounds like it's at least copying and possibly creating a reference. Is there a reason we are now using a build directory?

[rprouse]

I switched to the build directory because that is what both MSTest and xUnit use. The tools directory does not support multi-targeting. I just checked, it is copying the files into the bin directory but it isn't adding a reference in the project.

The NUnit3TestAdapter.props file has copy to output directory in it. We should test to see if the adapter works when it is not copied into the output directory,

<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <Content Include="$(MSBuildThisFileDirectory)NUnit3.TestAdapter.dll">
      <Link>NUnit3.TestAdapter.dll</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <Visible>False</Visible>
    </Content>
    <Content Include="$(MSBuildThisFileDirectory)nunit.engine.netstandard.dll">
      <Link>nunit.engine.netstandard.dll</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <Visible>False</Visible>
    </Content>
    <Content Include="$(MSBuildThisFileDirectory)Mono.Cecil.dll">
      <Link>Mono.Cecil.dll</Link>
      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
      <Visible>False</Visible>
    </Content>
  </ItemGroup>
</Project>

[CharliePoole]

The older change from lib to tools eliminated copying and worked fine. That was the primary reason I did it. I don't know if .net core changes that.

The build directory is documented as being for targets and other build stuff. Of course you can use multiple directories depending on how you want things handled.

I believe the references and copying to output are causing some of our problems. If there's some reason Microsoft now wants it done that way, we should prioritize turning the engine into a separate tools package.

[matthid]

Wasted some hours on this today. Easiest workaround was to downgrade to 3.7.0.

[tg73]

We likewise have test projects that reference Mono.Cecil and having just tried upgrading a project from NUnit Framework 2.6.4 to 3.8.1 with NUnit3TestAdapter 3.8.0, the referenced version of Mono.Cecil gets overwritten in the output directory. This is from a .NET Framework project using VS2017. As per @matthid, downgrading to NUnit3TestAdapter 3.7.0 fixed the problem.

[CharliePoole]

@rprouse What were you planning to do with this one? It seems to me it was a non-problem when we were not introducing references to the adapter in the test code by use of the lib directory.

[bording]

I see you've just released a new 3.9.0 version that I'd really like to be able to upgrade to get the benefits of #392, but this issue is still a problem in 3.9.0.

I'm stuck on 3.8.0-alpha1 because of this! Is there something I can do to help get this resolved?

[rprouse]

@bording I need to take the time to edit the targets files to not copy local, build a nuget package using that and then test to ensure it works. I talked to the VS test team and they said they recommend copying the files in, but it should work leaving them in the build directory. They are just not sure if it will break some VS features like Live Unit testing.

If it does turn out to break VS features, then another option might be to ILMerge Mono.Cecil into the adapter.

[bording]

@rprouse I'd be happy to help test a modified package if you get one put together. I don't have access to the live unit testing feature, so I wouldn't be able to test if that breaks or not, though.

[rprouse]

I tried not copying the adapter into the test directory. It works for the full .NET Framework using the old CSPROJ style project files, but it does not work for .NET Core or full framework using the new CSPROJ format. It looks like we are going to have to continue copying the adapter into the bin directory like MSTest and xUnit do ☹️

I was hoping that would work, ILMerge is a harder proposition and I've never tried it with .NET Core.

[rprouse]

The other option is to drop Mono.Cecil and switch back to using DiaSession

[bording]

I was hoping that would work, ILMerge is a harder proposition and I've never tried it with .NET Core.

That doesn't really work all that well currently. Neither ILMerge nor ILRepack can work with portable PDBs, and if you do switch to full PDBs (losing cross-platform support in the process) you get some weird resulting assemblies, with references to mscorlib, etc. when there shouldn't be any.

[bording]

What does the test adapter use Mono.Cecil for? If it's for something that all test adapters need to do, would it make sense to look at the xUnit adapter to see how they're handling it?

[CharliePoole]

As I understand it, we can "look" for a general idea but can't use their code due to licensing.

[OsirisTerje]

Mono.Cecil is used for getting source code navigation information, that is, file and line of the test, so that we can jump to the right codeline from test explorer. Microsoft is changing test explorer, ref the RFC they put out, and 3.9 has the CollectSourceInformation setting, which if set false, will not run the navigationprovider. Discovery will then be done by the test explorer up front before calling the adapter.
So an idea for a temporary workaround could be to set this property to true, add a msbuild prebuild target to remove our copy of mono.cecil. You could try this @bording .

If we continue with mono.cecil, we could make a fork of it, and embed the code we need into the adapter. It has an MIT license, so as long as we keep the copyright notices there, and otherwise abide by their license, that should be doable, and then we would no longer have any conflict with the dlls.

We could also revert to the Dia, but we did have quite a bunch of issues with that too, iirc, and then we would have to revisit those, but then I would assume mstest would have more or less the same issues.

[bording]

Microsoft is changing test explorer, ref the RFC they put out, and 3.9 has the CollectSourceInformation setting, which if set false, will not run the navigationprovider. Discovery will then be done by the test explorer up front before calling the adapter.

Do you know what version of Visual Studio is expected to start passing this flag to the adapter? According to the RFC it's hidden behind a feature flag for the IDE.

It does look like I could possibly pass in the value via runsettings for dotnet test but that still doesn't solve the IDE issue. How would I set runsettings for the Test Explorer window?

[OsirisTerje]

I've have no information about when it will arrive, but I would guess pretty soon, since they contributed this code for 3.9 now, and their cadence has been high the last years.

You add the runsettings file to your solution, templates for it are here https://marketplace.visualstudio.com/items?itemName=OsirisTerje.Runsettings-19151 , not updated for the latest changes yet, but coming soon, at least you get the general layout - AND you just add the setting

false inside the RunConfiguration section (along with the MaxCpuCount). I'll update the templates by the end of this week with the new properties included, both the general ones and the ones for NUnit. When you have added this to the solution (Note: solution, not project), you enable it from the top Test menu, (1) in the screenshot below:  You select the file (2) , and later the same menu looks like this (3) , and you can switch it on/off (4). If you run a server build using vstest.console, or the vstest task for VSTS/TFS, you can add the runsettings file to those too.

[rprouse]

Mono.Cecil Issue

[CharliePoole]

@rprouse Whatever it is, it's not visible without a login. 😄

[rprouse]

LOL, sorry about that. I use Trello to track work that I am doing and added the GitHub integration today. It looks like it adds comment here to the Trello cards when I link them. That isn't very useful to people that don't have access to my Trello account, so I guess I will need to go back to doing it manually.

[viceice]

My workaround is to use Cecil 0.9.6.4 for netXX and Cecil 0.10.0-beta7 for netstandardXX build.

[jnm2]

I've finished the work of completely removing Mono.Cecil from the VS test adapter (though not the shared NUnit engine published by the nunit-console repo). I'm expecting that the next release will contain this change.

(https://github.com/nunit/nunit3-vs-adapter/issues/296#issuecomment-365499002)

[bording]

@jnm2 Is there any plan to remove the Mono.Cecil dependency from the engine as well? While removing it from the test adapter (along with the new test discovery in VS 2017 15.6) means that my tests don't disappear from the VS Test Explorer, there is still a problem because the assembly from the test adapter package gets copied into the bin folder.

The version of Mono.Cecil.dll included in the 3.10.0 adapter package has a newer timestamp compared to the version in the actual Mono.Cecil package, so that is now being copied into the bin folder because of the PreserveNewest setting in the adapter's props file.

That means if I use 3.9.0 of the adapter, I get the version of Mono.Cecil that I've referenced in my project, but if I use 3.10.0, I get the copy in the adapter package instead.

At the very least, the engine should be updated to use Mono.Cecil 0.10.0-beta7 instead of 0.9.6.0 and 0.10.0-beta6 like it currently does.

As a side note, while investigating this, I noticed that the test adapter is still using version 3.7 of the engine, while 3.8 is the latest version.

[CharliePoole]

Best to ask this on the nunit-console repo. The teams overlap but are not the same.

FWIW, Removing Mono.Cecil from the engine seems unlikely to me, but restructuring it so that the bits are not copied to output is entirely possible. In fact, I believe this could be done separately for the adapter, since there is no package dependency - the adapter nuspec includes the engine binaries and can control whether or not they are copied.

[bording]

I believe this could be done separately for the adapter, since there is no package dependency - the adapter nuspec includes the engine binaries and can control whether or not they are copied.

I think all of the assemblies required for the adapter have to be in the same folder as the test assembly in order for things like dotnet test to work properly.

[rprouse]

@bording is correct. I did a spike switching back to not copying the files in and dotnet test stopped working. I was wondering if we could ilmerge Cecil into the engine.

[bording]

@rprouse Unfortunately, ILMerge/ILRepack might cause more problems then it solves: https://github.com/nunit/nunit3-vs-adapter/issues/366#issuecomment-340943511

[CharliePoole]

Although the adapter has to be in the output assembly for dotnet test to work, the engine does not actually have to be in the same directory as the adapter. The engine is not linked to the adapter but is located dynamically. I once had an experimental build that kept the engine separate from the adapter, but subsequent changes obsoleted it. No doubt it could be done again.

[rprouse]

It is difficult (impossible?) to load assemblies outside the probing path in .NET Core 1.1. That is why the Core version of the engine does not support addins.

[CharliePoole]

That was one of several things that argued against having a combined adapter and a combined engine for desktop .NET and .NET Core, at least in my view. Water under the bridge now.

[jnm2]

@bording You read my mind. I've been doing preparatory work to see if that is feasible.

I'm hoping there is enough demand at https://github.com/dotnet/corefx/issues/27469 to make an officially maintained net35 build of System.Reflection.Metadata happen. That would be the ideal outcome for everyone, as far as I can see.

If that falls through I may just follow the pattern of Sam Harwell's net35 port of S.C.Immutable with S.R.Metadata.

Either way we will be depending on another DLL in the output folder and we may want to jump the gun by IL-linking the adapter dependencies into a single assembly as a first step.

[jnm2]

I say this because I ran into very real issues with VSTest in the .NET Core 1.0 SDK loading an outdated version of System.Collections.Immutable which makes it impossible for users to write tests that compile with Roslyn or any other library which depends on newish S.C.Immutable.

I don't want us to be doing the same thing to people with the adapter's dependencies- exactly what seems to be happening to you here.

[bording]

I don't want us to be doing the same thing to people with the adapter's dependencies- exactly what seems to be happening to you here.

Yeah, any dependencies you have can have this problem when you have to copy them into a project's bin folder.

For example, I'm also using System.Reflection.Metadata in one project, so I'd potentially have the same problem with that.

If there's no way to avoid having a dependency at all, then doing something to make it private is really the only way to solve this.

[jnm2]

These are the possibilities I'm aware of, each of which needs research:

1. Instead of bundling Mono.Cecil, add a NuGet package dependency on it. This way the build tooling resolves the best version of Mono.Cecil and sets up the correct binding redirects in .NET Framework, all for free.

2. Make the props smarter. Instead of adding content items, run a target that only copies when it does not detect a ReferenceCopyLocal item with the same name. This feels like a worse version of (1).

3. IL-link Mono.Cecil into the engine assembly. This makes a private copy just as though we had written Mono.Cecil ourselves, inside the engine project, so there are no dependency conflicts.

4. Maintain our own low-level PE and metadata reader. Might be okay depending on what subset of functionality the engine really needs. I've done this to very small extents. It's the most work but also has the potential to run even faster than the full S.R.Metadata. It would live inside the engine assembly, so there are no dependency conflicts.

(1) is my current favorite idea. What do @OsirisTerje and @rprouse think?

[rprouse]

I dislike adding a reference to your project although it may become necessary. I've also thought about adding the test SDK, so maybe.

We could also fork Cecil. Also not ideal although it might help with the fact they dropped support for. NET 2.0

[jnm2]

Bundling instead of adding references usually means there will be DLL version conflicts. I think adding a package dependency on Mono.Cecil does not have to mean that the test project gets a compile-time reference to Mono.Cecil if we use ExcludeAssets="compile". But if the test project does decide to start directly referencing the Mono.Cecil package, the build system will unify the versions.

We could fork. I'd prefer to fork S.R.Metadata since that is a speedy low-level reader and Mono.Cecil eagerly loads a full domain model. What do you think of (4)? I'm guessing the engine only needs half a percent of the functionality of either one.

Right now we run people's net20 tests on CLR v2 against the .NET Framework 2.0 assemblies. If we dropped our engine's net20 build, we would run people's net20 tests on CLR v2 against the .NET 3.5 Framework assemblies. How bad would that really be? IIRC Mono actually forces all net20 assemblies to run against the net35 libraries.

[jnm2]

Good news: I fixed this and added nupkg acceptance tests proving that it works! https://github.com/nunit/nunit3-vs-adapter/pull/541

[jnm2]

Closed in nunit/nunit3-vs-adapter#541.