Replaced Mono.Cecil

[jnm2]

Fixes #296! 🎉

Everything is finished and just waiting on the merge of PRs #450 and #453!

NB: Rather than looking at the branch diff, please review each commit below the ——TEST MERGE—— lines individually. There are 12 commits, some large, so it'll probably make the most sense that way anyway. I will rebase onto master once the dependency PRs are in master.

[OsirisTerje]

The reason for moving to Mono.cecil is here: nunit/nunit3-vs-adapter #183
Also found this issue nunit/nunit3-vs-adapter #138
And this should be checked out: nunit/nunit3-vs-adapter #163
And we still have this open on the NUNit2 adapter, nunit/nunit-vs-adapter#6
And see this comment in nunit/nunit3-vs-adapter #171 : "Now for the solution, I found that Microsoft.VisualStudio.TestPlatform.ObjectModel.DiaSession.GetNavigationData was unable to retrieve navigation data if the provided method was defined in a base class of the given class( rather than being defined in the given class itself). It's probably a limitation in DiaSession.GetNavigationData behaviour, but we can make up for that by looking up the real class containing the method definition and providing its name to DiaSession.GetNavigationData."

@jnm2 I locally merged the two other PRs, and I'm now looking at the branch jnm/replace_Cecil. I now see that the tests are no longer showing up in Visual Studio. We had the adapter as a package included, but I can't see it anymore. It's deletion might have slipped in my review of the changes.

[jnm2]

@OsirisTerje

I now see that the tests are no longer showing up in Visual Studio. We had the adapter as a package included, but I can't see it anymore. It's deletion might have slipped in my review of the changes.

For me, this was because I had the 3.9 VSIX installed and it overrides any adapter-as-package in all projects. This is the last time you'll have to do this, but you'll need to disable the 3.9 VSIX in order to test this locally.

I deleted the the NUnit3TestAdapter 3.9 NuGet package reference for two reasons:

• Visual Studio was not even restoring the packages. It had a warning icon. Maybe because it conflicted with the adapter project reference? In any case, removing the NUnit3TestAdapter package reference made no difference to the discovery of tests either before or after this PR. They were still discovered in the Visual Studio Test Explorer window without the NuGet reference and with the VSIX disabled.

• Even if the NUnit3TestAdapter package reference worked, it would have prevented this PR from ever passing all tests because it's version 3.9, and version 3.9 has the Mono.Cecil issue.

I'll do the merge and comment on the issues.

[jnm2]

The reason for moving to Mono.cecil is here: nunit/nunit3-vs-adapter #183
Also found this issue nunit/nunit3-vs-adapter #138

There are various complications with use of DiaSession to retrieve navigation data in the adapter. One of the greatest is that we cannot easily use the console to run our navigation data tests.

This was mainly what #450 was about. A central point is that using self-executing tests isn't really what we want to test. The same point applies to running using NUnit Console.

On the practical side, you can use build.ps1 to run navigation tests from the command line in addition to being able to run navigation tests from the Test Explorer.

[jnm2]

And this should be checked out: nunit/nunit3-vs-adapter #163
And see this comment in nunit/nunit3-vs-adapter #171 : "Now for the solution, I found that Microsoft.VisualStudio.TestPlatform.ObjectModel.DiaSession.GetNavigationData was unable to retrieve navigation data if the provided method was defined in a base class of the given class( rather than being defined in the given class itself). It's probably a limitation in DiaSession.GetNavigationData behaviour, but we can make up for that by looking up the real class containing the method definition and providing its name to DiaSession.GetNavigationData."

I didn't remove any tests, so I preserved this behavior.

Tests:

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

Lines 50 to 51 in 446f36a

	[TestCase("+DerivedClass", "EmptyMethod_ThreeLines", 174, 175)]
	[TestCase("+DerivedClass", "AsyncMethod_Task", 178, 180)]

Implementation:

nunit3-vs-adapter/src/NUnitTestAdapter/NavigationDataProvider.cs

Lines 60 to 66 in 446f36a

	public NavigationData GetNavigationData(string className, string methodName)
	{
	return TryGetSessionData(_assemblyPath, className, methodName)
	?? TryGetSessionData(DoSafe(_metadataProvider.GetStateMachineType, _assemblyPath, className, methodName), "MoveNext")
	?? TryGetSessionData(DoSafe(_metadataProvider.GetDeclaringType, _assemblyPath, className, methodName), methodName)
	?? NavigationData.Invalid;
	}

Also, this behavior was preserved: #315 (Support base-class tests from external assemblies):

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

Lines 57 to 62 in 446f36a

	[TestCase("+DerivedFromExternalAbstractClass", "EmptyMethod_ThreeLines", 6, 7)]
	[TestCase("+DerivedFromExternalConcreteClass", "EmptyMethod_ThreeLines", 13, 14)]
	public void VerifyNavigationData_WithExternalAssembly(string suffix, string methodName, int expectedLineDebug, int expectedLineRelease)
	{
	VerifyNavigationData(suffix, methodName, "NUnit3AdapterExternalTests", expectedLineDebug, expectedLineRelease);
	}

[jnm2]

@OsirisTerje I really appreciate your thoroughness here.

And we still have this open on the NUNit2 adapter, nunit/nunit-vs-adapter#6

I haven't seen this issue yet. Let me add tests and see what can be done.

[jnm2]

I just had to remind myself of the instructions I added to the readme:

To run and debug tests on .NET Framework, load DisableAppDomain.runsettings.

https://github.com/nunit/nunit3-vs-adapter/blob/master/README.md#developing

[jnm2]

@OsirisTerje Added two commits testing and fixing nunit/nunit-vs-adapter#6.

There are no outstanding issues that I know of, so, back to you. 😊

[Dispersia]

Hey! Not being pushy, glad to see this fixed, but any idea when this'll be pushed to nuget? If this fixes the mono cecil throw then it would be great to use :)

Went back to test #371 and got the same issue @jons-aura got (I could build locally, but don't want to send it to all of our developers and have them reference that as a temporary fix)

[jnm2]

I'll let @OsirisTerje answer that, but in the meantime you can merge this with your nuget.config and use version 3.10.0-dev-00750 or newer:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
    <packageSources>
        <add key="NUnit prerelease" value="https://www.myget.org/F/nunit/api/v3/index.json" />
    </packageSources>
</configuration>

[OsirisTerje]

We'll try to get it out real soon now, hopefully during the upcoming weekend, but we would also really appreciate some testing up front. So if you (@Dispersia ) could add the link @jnm2 shows above and take it for a run, see if all works as it should, we would be grateful. There are some changes here, and it needs some test runs.

[Dispersia]

Confirmed, works perfectly for me, thanks for the fixes guys :) (no mono cecil throw and get profiling like expected)

[jnm2]

I messed up the link there for the nuget.config (fixed).