Mono.Cecil causes OutOfMemoryException with new .csproj PDBs
jnm2 opened this issue · 69 comments
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?
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.
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 :')
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!
@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?
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. 😄
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.
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.......
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.
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! 🙂
@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:
- Going back to DiaSession, with it's problems.
1.1 Perhaps we can use a newer DiaSession where available. - Waiting for an upgraded version of Cecil. (May not work under .NET 2.0, however)
- Upgrading the current Cecil component to deal with the new format ourselves.
- Writing some code ourselves to deal with the new pdb format. Is the MS Diagnostic code open sourced now?
@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.
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.
@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.
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? :(
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.
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.
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.
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.
@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.
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
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:
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>I just ran into this issue running the .NET 4.5 tests in my .NET Core branch of the adapter. @jnm2's workaround fixes it, but this is something we are going to have to deal with. It seems to me that we are in a bit of a catch-22 situation though. It is fixed in the latest betas of Mono.Cecil, but Mono.Cecil has dropped support for .NET 2.0. Our Engine is 2.0 though and we can't load the newer version of Mono.Cecil and hope that it works because there have been API changes to Mono.Cecil that we use.
@CharliePoole and @OsirisTerje do you have opinions on this? The issue goes back to the engine and I don't think we want to migrate the engine from 2.0, so other than dropping Mono.Cecil in the adapter, I'm not sure what to do.
@CharliePoole I consider it a workaround. Ideally I wouldn't have to put those lines in every .csproj; ideally NUnit would work with portable PDBs. If I'm not wrong, .NET Core uses the portable format exclusively because the "full" .NET PDB is shoehorned into a Windows specific unmanaged PDB format rather than something designed for .NET. I think handling portable PDBs is your only option for supporting .NET Core.
@CharliePoole I also consider it a workaround. Hand-copying that sort of thing into every .NET Core unit test project and if it needs to be added to every .NET Core/Standard project not just the test project, that is definately unworkable.
I checked the Mono.Cecil repo. This problem was first fixed in 0.10.0-beta1. 0.10 drops support for .NET 2.0. The last version to support .NET 2.0 is 0.9.6.4.
See http://cecil.pe/post/149243207656/mono-cecil-010-beta-1 and jbevain/cecil#282
Didn't we use DiaSession before? Newer versions support the portable PDB format. We could also copy the code we need, it is MIT. https://github.com/Microsoft/vstest/blob/master/src/Microsoft.TestPlatform.ObjectModel/Navigation/DiaSession.cs
@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!
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.
I spent a couple of hours on this tonight on my .NET Core branch and it isn't as easy as I had hoped. The .NET Core version seems to be working although a few of our tests are failing. The full CLR version isn't working. It uses "registry free activation context" which requires a manifest file that I don't know how to create. Anyone remember COM?
I am also concerned with their approach. The .NET 4.6 version of their adapter uses the Full PDB reader and the .NET Core adapter uses the Portable Reader. This doesn't seem right to me since .NET 4.x tests could be compiled using either the new or old CSPROJ format and produce either type of PDB.
I am going to step back from this for a bit. I pushed branch issue-296 if anyone wants to look, but I need to let this percolate for a bit.
@rprouse This is a pretty big change and I'm a little concerned about making both changes in the same PR, if that's what you mean.
Would we end up owning/maintaining the DiaSession code, or do you think we can hold it constant for the most part?
We would end up owning it. It is 4 files right now, but it is also using a private NuGet feed which I don't like. As I said, I am going to back away from this for a bit and just concentrate on the .NET Core changes.
Working workaround for this and the next problem with TestServer.CreateClient() issue http://bit.ly/2q8jbch
This issue is killing me. Doing the work around but it is deeply disappointing.
I'd like to get rid of the dependency on Mono.Cecil, whatever that entails.
I'm willing enough to throw myself at the problem when I get back if no one else would rather do it.
@jnm2
Please do :-)
@OsirisTerje Preliminary question. Why is the adapter targeting net35 instead of net45, since net45 or higher is installed with VS2012 and up?
Answered my own question. VSTest ships with vstest.executionengine.clr20.exe which loads a minimum of net35. The discovery engine loads a minimum of net45.
I can't see a way to do anything here without first removing Mono.Cecil from the engine, including the engine's nupkg. This is going to be a lot trickier since the projects aren't contained in a monorepo.
Are you sure you can't start with changing the navigation first to use the Dia objects from MS, and just let the cecil dll's stay.
Then next round start moving them.
That way you can ensure the navigation continues to work with Dia, and fix potential isses with that first. Do this one step at a time.
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. :D
The master build is failing:
========================================
TestAdapterUsingVSTest
========================================
Executing task: TestAdapterUsingVSTest
An error occurred when executing task 'TestAdapterUsingVSTest'.
Error: One or more errors occurred.
And the tests run Debugger.Launch() four times during build.cake -t test.
Debugger: nunit3testexecutor.cs, line 24, needs to be commented out
Build fail: Yeah, asked for some help with those builds, not sure what stops them. Works locally, once you comment out the debugger,
After commenting that out, I get this: for -t test (on TestAdapterNetCore):
1) Failed : NUnit.VisualStudio.TestAdapter.Tests.TestExecutionTests.AttachmentsShowSupportMultipleFiles
Expected: 2
But was: 1
at NUnit.VisualStudio.TestAdapter.Tests.TestExecutionTests.AttachmentsShowSupportMultipleFiles() in C:\Users\me\Source\Repos\nunit3-vs-adapter\src\NUnitTestAdapterTests\TestExecutionTests.cs:line 158
2) Failed : NUnit.VisualStudio.TestAdapter.Tests.TestExecutionTests.TestOutcomeTotalsAreCorrect(Passed)
Expected: 22
But was: 21
at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
at NUnit.Framework.Internal.Execution.SimpleWorkItem.PerformWork()
3) Failed : NUnit.VisualStudio.TestAdapter.Tests.TestExecutionTests.TestOutcomeTotalsAreCorrect(Failed)
Expected: 5
But was: 6
at NUnit.Framework.Internal.Commands.TestMethodCommand.Execute(TestExecutionContext context)
at NUnit.Framework.Internal.Execution.SimpleWorkItem.PerformWork()
src\NUnitTestAdapterTests\bin\Release\netcoreapp1.0\nunit.framework.dll is not present for FixtureWithAttachment.AttachmentTest on netcoreapp1.0. I think we need a dotnet publish like we do for the framework (https://github.com/nunit/nunit/blob/259066fb2b8edb753f982a88662834e0c6a09215/build.cake#L149-L154).
Remaining CI failure fixed! #449 is my work for today. I'll be home in a couple more days.
Note to self: should verify whether /debugtype:embedded crashes Mono.Cecil. (I think that is when the DLL contains the symbols like full, but the PDB info is portable. This only mitigation would be to remove Mono.Cecil from the engine itself, something which could provide speed benefits.)
Here's the integration test, failing spectacularly 😂: https://github.com/nunit/nunit3-vs-adapter/compare/jnm2/replace_cecil
I wasted an unprecedented amount of time trying to figure out why on earth the projects were building properly in VS, the command line, and the MSBuild Structured Log Viewer, but any time I built from Cake it started including extra files in the build output.
Turns out, build.ps1 puts the dotnet SDK 1.1.4 in the PATH and that causes MSBuild to pick up that SDK.
I did not know that was a thing.
I'll know for the future.
@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.
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!
I actually rebased onto a test merge of #450 and got all net45 tests passing. Only thing left is netcoreapp1.0.
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 testwhich runs the project without publishing it, so the dependencies are not present in the same folder.Assembly.Loadthe 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.
Done already.
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
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.
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.
The hang is an unrelated, preexisting issue, identical on our current master branch.
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.
It's only 40 extra minutes of CI time =P
nunit3-vs-adapter/src/NUnitTestAdapterTests/IssueNo24Tests.cs
Lines 22 to 32 in 126a677
TestCategory!=LongRunning does not work to exclude them...
Yikes, TestCategory!=Navigation doesn't exclude navigation tests either! TestCategory=Navigation finds no tests. Is test category filtering broken?