Mono.Cecil causes OutOfMemoryException with new .csproj PDBs

[jnm2]

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 jbevain/cecil#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 #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?

[CharliePoole]

Definitely something we need to figure out.

@jnm2
I agree it would be better to just hand info to VS as you describe and have VS figure it out. That has always made sense to me: you guys just compiled the darned thing, can't you figure out where stuff is? Unfortunately, that's not how Test Explorer works. It's up to us to supply the file and line number.

Our old approach used a component from VS to do it. We went to Mono Cecil because VS was not doing it correctly in some cases, which you can read about in the issue and PR where we introduced it. We definitely don't want to go back to the old method. Of course, we are also using Cecil in the engine, so it made sense to use it here.

Bottom line: if we can figure out a better way that works for the old and the new pdb format, that will be better. If we can figure out a way that doesn't require the pdb, that's even better.

Flagging this for discussion so we can decide on a direction.

[jnm2]

Okay, this is weird. MSTest.TestAdapter is using DiaSession.GetNavigationData(className, methodName) which returns a DiaNavigationData with properties FileName and MinLineNumber , but dotPeek can't locate the definition of those types and it isn't open source.

But look at what xUnit is doing: https://github.com/xunit/xunit/blob/master/src/xunit.runner.utility/Utility/DiaSession.cs#L47 All kinds of reflection!

Apparently the definitions are in Microsoft.VisualStudio.TestPlatform.ObjectModel which I can't locate but VS must have.

Edit: failed to notice the NuGet reference to Microsoft.VisualStudio.TestPlatform.ObjectModel from the NUnit test adapter project :')

[CharliePoole]

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!

[CharliePoole]

@nunit/core-team @nunit/vs-extensions-team
We really have to get better at dealing with these items that require a general discussion before anyone can effectively work on them. @jnm2 is in a position of spinning his wheels trying to figure out stuff that others already know.

Not to put to fine a point on it... that's not the kind of team I think we want to be. I marked it for discussion two weeks ago. Nobody else has commented since then. Should I be doing more? Should we put these things on some sort of timer so we get reminders? How can we do this better? Or is the overall NUnit project just getting too big?

[ChrisMaddock]

Personal reasoning: I haven't commented as I have absolutely no relevant knowledge to share here. 😄

Team notifications are often easy to skip over - I often don't acknowledge core-team mentions when I have nothing particularly new to contribute. We're a small team, and have a pretty good idea of each others areas of expertise. Maybe it's worthwhile tagging specific people when their opinions are wanted. That would at least encourage me to post that I have no idea. 😄

[OsirisTerje]

I think some of the reason is that whenever something like this pops up, it is a serious job just finding back to all the earlier issues around the same thing. We have some other issues with the mono.cecil, related to building in release. Right off the top of my head, I dont remember the issues we had with the DiaSession, but there were several. Getting into a discussion when things are spread out is something I find hard. I am not quiet because I am not interested, but because I am struggling with information retrieval, and in some cases with information overload. Yeah XUnit does it differently, but what are the pros and cons there? And, no - I don't have a good overall suggestion how we can handle this - yet. Smaller things like: I would love some hierarchies (like VSTS has), that makes wonders for keeping stuff together, but github doesnt have that. We could introduce some feature/epic tags and use that for grouping. That could help. And, again, I think getting the repro repositories up and running so that we could move more quickly between issues would help, without looking for the last thing one and each of us were doing.

[OsirisTerje]

Zenhub has the possibility for marking an issue as an Epic. Can we use that to group related things together? Like now, I can't find the debug/release issue.......

[OsirisTerje]

With an Epic set up, that epic could be the parent of all related issues. Further, it could hold links to relevant repro repos which are usable across the issues. That also means the repro repos could be placed anywhere. And, it could hold the links to the documentation in the wiki that is relevant for the same.
There is another issue in the discussion column, where I needed more information, @CharliePoole Charlie came up with this in pieces, and it should be remembered for the next issue that comes along on the same, and that issue has already popped up.

[CharliePoole]

I'll post more later, but @OsirisTerje quickly: what are the "repro repositories?"

Sorry to get all exercised here, but I'm trying to transition from a system where stuff came in and I made a decision to one where people discuss, suggest, etc. It bothered me that by the absence of either a decision or some discussion, @jnm2 had to do a bunch of original research to come up with stuff you and I already know. Maybe I should fall back to dictator mode and just pick something after a certain time has elapsed. Based on past experience, that's when all the comments would probably come in! 🙂

[CharliePoole]

@OsirisTerje I agree we could be better organized. 😄 However, one approach is to simply ask the question as a part of the discussion, to see if anyone remembers it. That's the virtual version of the well-known Expert in Earshot organizational pattern.

Here are the closed issues that reference DiaSession:
https://github.com/nunit/nunit3-vs-adapter/issues?q=is%3Aissue+diasession+is%
Here is the one that references a problem with release builds, where no pdb is generated:
#276

The main problem with DiaSession is that it's part of VS and we therefore can't use it when not running under VS. That made our CI builds fail.

A second problem, is that it doesn't deal with overloads. All the references to a particular class and method name end up at the same line of code, whatever the arguments. I don't know if Microsoft fixed this in later releases.

The main problem with Cecil, originally, was that it requires a pdb. We decided that was OK. Now, however, we have the added problem that the pdb format is changing.

I don't think we have enough info to make a decision, but we can decide where to spend time looking for more information. Choices I see include:

1. Going back to DiaSession, with it's problems.
   1.1 Perhaps we can use a newer DiaSession where available.
2. Waiting for an upgraded version of Cecil. (May not work under .NET 2.0, however)
3. Upgrading the current Cecil component to deal with the new format ourselves.
4. Writing some code ourselves to deal with the new pdb format. Is the MS Diagnostic code open sourced now?

[CharliePoole]

@OsirisTerje ZenHub has Epics, but generally we use those for big jobs that need to be split up, not as a way to organize issues that we have done at different times. To be honest, we don't have a lot of issues in this project and I found the ones I posted using two queries.

[CharliePoole]

Nope. That's something else.

…

On Fri, Jan 27, 2017 at 2:25 PM, Joseph Musser ***@***.***> wrote: Is the MS Diagnostic code open sourced now? Is that https://github.com/Microsoft/clrmd? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#296 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACjfhe45FBVeRaemb4uhX11hC_YV1Wo1ks5rWm7igaJpZM4LjrQ3> .

[jnm2]

Isn't DiaSession the only thing that will be able to get file and line number when there is no PDB output, besides pointing Roslyn at the source and compiling yourself?

A second problem, is that it doesn't deal with overloads. All the references to a particular class and method name end up at the same line of code, whatever the arguments. I don't know if Microsoft fixed this in later releases.

This will plague their own test adapter plus every other test adapter I have found, so I would hope so. If not we can open an issue.

[CharliePoole]

@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.

[jnm2]

For me, it still has the big drawback that it only works with VS.

Sounds like we have to pick a drawback: VS only, or no navigation information when no PDB is output? :(

[CharliePoole]

Or write our own code...

Or do it differently under the adapter and in other contexts... although that means we won't be able to run some of our adapter tests under the console. Possibly not so bad a problem since we can run them under vstest-console.exe.

I'm particularly concerned about where the Gui will get files and line numbers.

[jnm2]

Or write our own code...

I'm pretty sure it is not possible to get the file and line info from an assembly with no PDB. As in, the crucial information doesn't exist. Not unless you have the source and are willing to recompile or index the syntax trees against the reflection info from the assembly. If I'm right about this, DiaSession is the only way to handle PDB-less scenarios.

[CharliePoole]

I think that basic line info is included in some assemblies. AFAIK, DiaSession leverages that, as well as any available PDB. I'd like to read the source to check. I believe it's part of what was just opened by MS but I haven't had the time to go find it.

[CharliePoole]

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.

[OsirisTerje]

@CharliePoole Repro repositories is what we talked about earlier, that we should have somewhere to store issue reproduction code, so that one and each of us didnt have to create that ourselves. Sometimes the reporter add a repro, but many times they dont.

[jnm2]

Interesting, the new .NET-focused PDB format (the so-called portable PDB format) has an open source reader which implements DiaSymReader: https://github.com/dotnet/symreader-portable

[codito]

The workaround for this issue is to add following line under PropertyGroup node in the csproj.

<PropertyGroup>
  <DebugType Condition="'$(TargetFramework)' != '' AND '$(TargetFramework)' != 'netcoreapp1.0'">Full</DebugType>
</PropertyGroup>

The .NET BCL provides a way to parse portable PDBs using System.Reflection.MetadataReader. DiaSession uses it under the hood. References:

• https://github.com/Microsoft/vstest/blob/master/src/Microsoft.TestPlatform.ObjectModel/Navigation/PortablePdbReader.cs
• https://github.com/Microsoft/vstest/blob/master/src/Microsoft.TestPlatform.ObjectModel/Navigation/PortableSymbolReader.cs#L94

[jnm2]

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!

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>    
    <TargetFramework>net462</TargetFramework>
    
    <!-- Workaround for https://github.com/nunit/nunit3-vs-adapter/issues/296 -->
    <DebugType>Full</DebugType>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="NUnit" Version="3.6.1" />
  </ItemGroup>

</Project>

[jnm2]

Will wait for #450.

[rprouse]

@jnm2 if you need a specific NET Core SDK, you can update Cake and/or adjust the version it pulls in the build scripts. I've run into the same thing with all the other projects. It confused me to no end why dotnet --version was different before and after a command line build.

[jnm2]

Thanks. Might do a separate PR. SDK 1.1.4 has security advisories too IIRC. Sorry for taking this out from under you, hope it's a help!

[jnm2]

I actually rebased onto a test merge of #450 and got all net45 tests passing. Only thing left is netcoreapp1.0.

[jnm2]

I'm switching gears.

• MetadataReader, while fast and clean, is not a small amount of code. I'm estimating between 1.5 and 2k lines if I would finish it. That's a moderate maintenance burden and something I wasn't looking forward to bribing people into reviewing.

• Resolving assembly file paths from related projects is too complex. We need the adapter to work with dotnet test which runs the project without publishing it, so the dependencies are not present in the same folder. Assembly.Load the only thing I know of to determine the path.

• Once we accept that we must use reflection to load the assembly, it's okay anyway to use reflection to load the assemblies because the NUnit engine will have done it already anyway during discovery. This is true because .NET Core has no AppDomains and the engine doesn't do this work out-of-process.

The great thing is that the code will end up very similar for both net35 and netcoreapp1.0.

I parked the MetadataReader code in case it ever becomes advantageous to come back to it.

[jnm2]

Done already.

[jnm2]

Last thing I've just now discovered that only affects the combination of dotnet test and .NET Framework, and then everything is finished:

Exception Microsoft.VisualStudio.TestPlatform.ObjectModel.TestPlatformException, Exception thrown executing tests in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapterTests\bin\Release\net45\NUnit.VisualStudio.TestAdapter.Tests.dll
Could not find the manifest file C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\.dotnet\sdk\1.1.4\TestHost\TestPlatform.ObjectModel.x86.manifest for Registry free Com registration.
   at Microsoft.VisualStudio.TestPlatform.ObjectModel.Navigation.FullSymbolReader.GetManifestFileForRegFreeCom()
   at Microsoft.VisualStudio.TestPlatform.ObjectModel.Navigation.FullSymbolReader.CacheSymbols(String binaryPath, String searchPath)
   at Microsoft.VisualStudio.TestPlatform.ObjectModel.DiaSession..ctor(String binaryPath, String searchPath, ISymbolReader symbolReader)
   at NUnit.VisualStudio.TestAdapter.NavigationDataProvider.TryGetSessionData(String assemblyPath, String declaringTypeName, String methodName) in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapter\NavigationDataProvider.cs:line 70
   at NUnit.VisualStudio.TestAdapter.NavigationDataProvider.GetNavigationData(String className, String methodName) in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapter\NavigationDataProvider.cs:line 61
   at NUnit.VisualStudio.TestAdapter.TestConverter.MakeTestCaseFromXmlNode(XmlNode testNode) in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapter\TestConverter.cs:line 155
   at NUnit.VisualStudio.TestAdapter.TestConverter.ConvertTestCase(XmlNode testNode) in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapter\TestConverter.cs:line 81
   at NUnit.VisualStudio.TestAdapter.NUnit3TestExecutor.RunAssembly(String assemblyPath, TestFilter filter) in C:\Users\Joseph\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapter\NUnit3TestExecutor.cs:line 247

[jnm2]

I suppose we don't need navigation data unless we're running inside the IDE, so it doesn't matter if dotnet test can't access navigation data. I'll make it fault-tolerant to DiaSession.

[jnm2]

Everything done except for a new issue which that revealed: dotnet vstest successfully discovers all tests but then hangs indefinitely without running adapter code if I specify /TestCaseFilter:"TestCategory!=Navigation". IDE VSTest doesn't. No idea why yet.

[jnm2]

The hang is an unrelated, preexisting issue, identical on our current master branch.

[jnm2]

Oh crap, the problem is much more evident using IDE VSTest because it lists the tests as they succeed. The presence of /TestCaseFilter:"TestCategory!=Navigation" causes explicit tests to run. I somehow remember this being a thorny issue.

[jnm2]

It's only 40 extra minutes of CI time =P

nunit3-vs-adapter/src/NUnitTestAdapterTests/IssueNo24Tests.cs

Lines 22 to 32 in 126a677

	[Test]
	public void Slow()
	{
	Thread.Sleep(150000);
	}
	[Test]
	public void Slower()
	{
	Thread.Sleep(250000);
	}

[jnm2]

TestCategory!=LongRunning does not work to exclude them...

[jnm2]

Yikes, TestCategory!=Navigation doesn't exclude navigation tests either! TestCategory=Navigation finds no tests. Is test category filtering broken?

[jnm2]

#452 is going to have to be fixed before it's possible for the Mono.Cecil fix to pass in CI unless we disable all .NET Framework tests that use the dotnet CLI.

[jnm2]

It’s ready! 🎉 🎉 🎉

PR can be reviewed at #454, and merged once #450 and #453 are merged!

I'm just a little excited about this! =D