nunit3-vs-adapter: Mono.Cecil causes OutOfMemoryException with new .csproj PDBs
I converted the old-style test .csproj targeting .NET Framework 4.6.2 to a new-style .csproj targeting the exact same .NET Framework 4.6.2 (VS2017 build 15.0.26020.0):
<Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0">
<PropertyGroup>
<TargetFramework>net462</TargetFramework>
<RuntimeIdentifier>win</RuntimeIdentifier>
</PropertyGroup>
<ItemGroup>
<Compile Include="**\*.cs" />
<EmbeddedResource Include="**\*.resx" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="NUnit" Version="3.6.0" />
</ItemGroup>
</Project>
All tests run exactly the same as they always have using the NUnit console runner, but no tests are discovered using the NUnit VS adapter. I found out why- Mono.Cecil is causing OutOfMemoryExceptions while reading the PDB to find source code locations:
at Microsoft.Cci.Pdb.MsfDirectory..ctor(PdbReader reader, PdbFileHeader head, BitAccess bits)
at Microsoft.Cci.Pdb.PdbFile.LoadFunctions(Stream read, Dictionary`2& tokenToSourceMapping, String& sourceServerData, Int32& age, Guid& guid)
at Mono.Cecil.Pdb.PdbReader.PopulateFunctions()
at Mono.Cecil.Pdb.PdbReader.ProcessDebugHeader(ImageDebugDirectory directory, Byte[] header)
at Mono.Cecil.ModuleDefinition.ProcessDebugHeader()
at Mono.Cecil.ModuleDefinition.ReadSymbols(ISymbolReader reader)
at Mono.Cecil.ModuleReader.ReadSymbols(ModuleDefinition module, ReaderParameters parameters)
at Mono.Cecil.ModuleReader.CreateModuleFrom(Image image, ReaderParameters parameters)
at Mono.Cecil.ModuleDefinition.ReadModule(Stream stream, ReaderParameters parameters)
at Mono.Cecil.ModuleDefinition.ReadModule(String fileName, ReaderParameters parameters)
at NUnit.VisualStudio.TestAdapter.NavigationDataProvider.CacheTypes(String assemblyPath)
at NUnit.VisualStudio.TestAdapter.NavigationDataProvider.GetNavigationData(String className, String methodName)
at NUnit.VisualStudio.TestAdapter.TestConverter.MakeTestCaseFromXmlNode(XmlNode testNode)
at NUnit.VisualStudio.TestAdapter.TestConverter.ConvertTestCase(XmlNode testNode)
at NUnit.VisualStudio.TestAdapter.NUnit3TestDiscoverer.ProcessTestCases(XmlNode topNode, ITestCaseDiscoverySink discoverySink, TestConverter testConverter
I want to stress that this is not related to .NET Core. The new PDB format will be used in the future for all normal .NET projects including .NET Framework projects.
The relevant Mono.Cecil PDB issue is at https://github.com/jbevain/cecil/issues/282. Support for this new PDB format is shipped so far only in 0.10-beta1 and 0.10-beta2.
All we need is the source file and line number. I read through https://github.com/nunit/nunit3-vs-adapter/issues/183, but I’m concerned that Mono.Cecil might not be the best way to go. It’s never going to be as reliable as, say, Roslyn. xUnit’s desktop VS test adapter also does not use Mono.Cecil.
Can we use Visual Studio to give us the source file and line number for a given assembly and type or member? That’s going to be the most reliable since it’s the IDE itself that will be doing the navigating. If not, can we use Roslyn?
How close can we get to where this is all happening, so that ideally we don’t end up chasing a moving target? It’s comments like this that make me worry that we’re reinventing the wheel somewhere:
// Through NUnit 3.2.1, the class name provided by NUnit is
// the reflected class - that is, the class of the fixture
// that is being run. To use for location data, we actually
// need the class where the method is defined.
// TODO: Once NUnit is changed, try to simplify this.
Wouldn’t it be better to hand Visual Studio an assembly path, a class name and a method and have Visual Studio or at least Roslyn do the work of figuring out what file and line number the method is defined in? I mean imagine when they add the Source Generators proposal. I’d rather offload this to VS and Roslyn since they are ultimately responsible to know all this stuff. Otherwise we’ll always be playing catch-up with new language and debugging features and crash, like we’re doing right now?
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Reactions: 2
- Comments: 69 (66 by maintainers)
Links to this issue
Commits related to this issue
- Fix test discovery in VS for NewtonSoft.Json.Roslyn.sln. Add a reference to NUnit3TestAdapter. Emit full symbols for test project. Workaround for https://github.com/nunit/nunit3-vs-adapter/issues/296... — committed to codito/Newtonsoft.Json by deleted user 7 years ago
- Checkpointing the DiaSession work for #296 — committed to nunit/nunit3-vs-adapter by rprouse 7 years ago
- Explicitly build full PDB format for unit test project Workaround for https://github.com/nunit/nunit3-vs-adapter/issues/296, to allow tests to be discovered in VS Test Explorer — committed to TestableIO/System.IO.Abstractions by tathamoddie 7 years ago
Since I have a number of projects that will only ever target
net462, I’m using this and it works like a charm. Thanks @codito!The workaround for this issue is to add following line under
PropertyGroupnode in the csproj.The .NET BCL provides a way to parse portable PDBs using
System.Reflection.MetadataReader. DiaSession uses it under the hood. References:@rprouse Yes… if you read through the earlier comments here, you’ll see these options have come up before. I’m in favor of taking code from GitHub myself, provided it works with both the full and the portable formats. Basically, this is one of those items where we seem to discuss endlessly and never circle into a decision. We should make one, change it to Backlog and let somebody start on it!
Thanks. For some reason I was thinking this occurred when loading a DLL with a PDB beside it, which the engine will be doing. Let me start off with an integration test so I don’t waste my time. 😄
My vote is to pull in the code from DiaSession and the two PDB readers and use those. I can do that as part of my .NET Standard work if nobody objects. Currently I am pulling in the same version of Mono.Cecil as the engine for the Full CLR build, but the newer version in the .NET Standard build. It would be nice to just drop that.
To put this in perspective. I’d be very happy to depend solely on Microsoft for the adapter. But I think we could use this info in other places and anything that assumes the users have a copy of VIsual Studio around doesn’t work in that case.
@jnm2 It’s quite likely that it won’t plague their tests, since they don’t have parameterized methods as far as I know. The general problem with the entire adapter scenario - one that both I and the xUnit guys have pointed out many times - is that it’s designed to work with the lowest common denominator of functionality. They took MSTest and “generalized” it to work with “any” framework - that is any framework that does exactly what MSTest does in the same way but possibly with different names. Both NUnit and xUnit.net have capabilities that go beyond MSTest. So did mbUnit, when it was active.
In my work as a coach, I call this anti-pattern “Generalizing from a single case” You gotta have the cases before you can generalize! The only thing worse is “Generalizing from no cases” which is when you just make the stuff up. 😈
OK… end of rant for me. Yes, let’s hope they have improved it. If so, some such strategy as 1.1, possibly using reflection, might be appropriate. For me, it still has the big drawback that it only works with VS.
DiaSession stands for “Diagnostic Session” It’s a class in the VS object model, which is what we use when we are talking to VS in the adapter. We used it for many years and stopped using it a short while ago. I think we may still use it in the old adapter. We know it’s limitations, and that’s why we stopped using it. Going back to it is obviously an alternative and should be part of the discussion we have here before we start working on a solution to this problem.
Unfortunately, the @nunit/vs-extensions-team members who know about this stuff haven’t joined in here to make a useful discussion and you are in the position of discovering things for yourself that “we” already know. Not good teamwork, I’m afraid.
@jnm2 I appreciate your enthusiasm and wanting to jump right into the code, but let’s decide on an overall direction first. Also, please be careful about looking at other folks code unless it’s MIT-licensed, like ours. It’s OK to take a look extract a concept that you then re-implement, but it’s not OK to actually use the code, even with changes. Of course, sometimes it’s hard to define which of the two things you are doing, which is the reason we have to avoid it.
Here’s the concept I got out of the code you pointed to. Jim and Brad wanted to use the best version of DiaSession they could, based on the version of VS in use. So they wrapped it in a reflection shell. It’s a good idea, and one we could consider using, but it’s different from what we now do. We actually require an old version of the VS object model to be available to run our adapter. We could reconsider that, but it’s kind of a major architectural point and would require us to change a bunch of stuff at once. Probably a 4.x thing.
Thanks for your enthusiasm! Let’s wait a bit to get other people to chime in, make some decisions and change this from a discussion item to an issue to be worked on!