Test in all target environments using the correct version of the NUnit VSTest adapter #450

jnm2 · 2018-02-10T21:27:32Z

Blocking my work on #296. This was complex enough to peel away into a targeted PR.

From my findings in https://github.com/nunit/nunit3-vs-adapter/compare/jnm2/replace_cecil:

Any code that uses DiaSession must run in an environment that has already loaded a version of Microsoft.VisualStudio.TestPlatform.ObjectModel.dll which is newer than the one we must compile against for net35. The one we compile against tries to load msdia110typelib_clr0200.dll from the VS2012 installation. While we must compile against it, we must not distribute it or run our tests against it.
We got away with running tests against it in the past, but so many of our tests require navigation data (even unrelated tests) that we will not be able to continue with this misconfiguration once we start using TP.ObjectModel.dll's DiaSession constructor. This includes both direct referencing or pure reflection.
Self-executing adapter tests are out of the question unless we write a host that locates VSTest from .dotnet or vswhere and loads the correct Microsoft.VisualStudio.TestPlatform.ObjectModel.dll, or unless we barely run any tests at all. The point of our VSTest adapter has nothing to do with self-executing tests. Running those few tests that can pass is merely duplicating the testing already done by the tests which we do run using VSTest. Self-executing tests are complexity with no benefit.
Since we publish a VSTest adapter, what we do care about is testing it in every environment that might use it. Ones I know about currently include:
- VSTest, from VS installation or dotnet SDK (dotnet vstest), against any .NET Framework or .NET Core assembly
- dotnet test, against any .NET Framework or .NET Core project
If VSTest supports frameworks other than .NET Framework and .NET Core such as UWP (or when VSTest finally supports .NET Standard test assemblies), we should look at expanding that someday.

Therefore, I removed our TestAdapter and TestAdapterNetCore self-executing test targets and expanded our TestAdapterUsingVSTest into four targets:

VSTest-net45
VSTest-netcoreapp1.0
DotnetTest-net45
DotnetTest-netcoreapp1.0

As soon as the self-executing .NET Core tests were gone, we no longer needed the publish step. I was thus able to undo that part of 3bf25c8. Test failures forced me to undo other parts. With the new test targets, the folder nesting level for the assemblies built for .NET Core now matches .NET Framework.

Does this change seem sound?

…t VSTest adapter

…r tested against

jnm2 · 2018-02-10T21:31:03Z

@OsirisTerje I have permission to merge without review. I'm so used to clicking it as soon as it turns green that I'm afraid I may cause an accident.

CharliePoole · 2018-02-10T21:43:22Z

@jnm2 @OsirisTerje Just a thought... Being allowed to do something by a software system is not equivalent to "what you are supposed to do." That's one reason I drag my feet as far as automating various "supposed to do" things like this, like standards, etc. I believe the best teams are grown when people actually talk to one another rather than relying on software permissions.

On a more practical note... only owners (me and Rob) and admins (set by each project's lead) are allowed to override the review requirement. It's possible that @OsirisTerje has not set up the review requirement for that repository. Recollect that we did agree that such things were up to each project lead.

FWIW if I were the project lead, I would not set up the GitHub review requirement because it leads to the bad situation that @jnm2 describes - people get used to the system telling them what it's OK to do. See paragraph 1. Of course, that's just me. 😸

jnm2 · 2018-02-10T21:47:35Z

I'll continue to try to pay attention. Staying up till 1 am every night is not the best way to fight jetlag after skipping a night of sleep, I found out today. 😝

jnm2 · 2018-02-10T23:13:29Z

Hey, I just realized that this isn't ready to merge yet. The second commit starts skipping all net45 tests because on net45 the engine loads the tests in a different appdomain and it can't find Microsoft.VisualStudio.TestPlatform.ObjectModel.dll.

I can't think of any way to make this work other than a .runsettings file containing <DisableAppDomain>. (The engine doesn't appear to provide any chance of obtaining a reference to the appdomain after creating it and before loading assemblies into it, and ImageRequiresDefaultAppDomainAssemblyResolver doesn't have any effect because the engine is itself already loaded into a secondary appdomain.)

Running the tests in the same appdomain as the engine seems to cause some remoting URL conflicts. I'll try to see what that's about.

(To be clear, these measures are not necessary in general. Only for the test project NUnit.TestAdapter.Tests which references TP.ObjectModel.dll—we do need it to use the right one.)

jnm2 · 2018-02-10T23:18:44Z

Hey, WIPbot showed up! How cool is that! 😄

jnm2 · 2018-02-11T00:19:05Z

Starting to look like we'll need a new release of the engine which allows you to override the default URL of the test agency before starting it without having to know about and set up every other service on your own.

…del.dll

jnm2 · 2018-02-11T01:08:56Z

src/NUnitTestAdapterTests/TestAdapterUtils.cs

+                engine.Services.Add(new DefaultTestRunnerFactory());
+                engine.Services.Add(new TestAgency("TestAgency for " + TestContext.CurrentContext.Test.Name, 0));
+                engine.Services.Add(new ResultService());
+                engine.Services.Add(new TestFilterService());


The engine needs to provide some way to override a service before starting all the services without forcing you to maintain an almost identical list like this.

I added something like this in a recent test. I think it needed InternalsVisibleTo...

Oh. Looks like it never got committed. 😞 nunit/nunit-console#320 (comment)

jnm2 · 2018-02-11T01:09:45Z

Got it working! 🎉 Ready for review!

jnm2 · 2018-02-11T01:28:23Z

I locally rebased https://github.com/nunit/nunit3-vs-adapter/compare/jnm2/replace_cecil on a test merge of this PR and it's looking very positive! This PR was the hard part! 😊

Only thing left is some of the navigation tests like the async ones. I can look at what we had before #186.

jnm2 · 2018-02-14T04:16:03Z

I discovered that there is a difference between the IDE's vstest and dotnet vstest when it comes to .NET Framework tests, so we need to be testing:

vswhere…vstest.console.exe, both target frameworks
dotnet vstest, both target frameworks
dotnet test, both target frameworks

This is done in the last commit.

jnm2 · 2018-02-14T04:28:11Z

NuGet timed out on VSTS. I queued a new build which succeeded but didn't update the status of the commit.

jnm2 · 2018-02-14T17:40:54Z

I'm not absolutely sure that we need to run all tests against dotnet vstest because dotnet test ends up calling the same stuff. But the tests are so fast, why not?

jnm2 · 2018-02-15T05:15:55Z

Hooray, I have nothing left to do! The Mono.Cecil removal PR is waiting on this one at #454.

OsirisTerje

Ok

jnm2 added 2 commits February 10, 2018 15:35

Test in all target environments using the correct version of the NUni…

2038d87

…t VSTest adapter

Ensure that referenced version of TP.ObjectModel is not distributed o…

8c28068

…r tested against

jnm2 added pri:critical is:build labels Feb 10, 2018

jnm2 requested a review from OsirisTerje February 10, 2018 21:27

jnm2 mentioned this pull request Feb 10, 2018

Mono.Cecil causes OutOfMemoryException with new .csproj PDBs #296

Closed

jnm2 changed the title ~~Test in all target environments using the correct version of the NUnit VSTest adapter~~ [Do not merge] Test in all target environments using the correct version of the NUnit VSTest adapter Feb 10, 2018

jnm2 force-pushed the jnm2/run_all_tests_with_adapter branch from 7694f91 to 92d52f6 Compare February 11, 2018 00:58

Run tests in same AppDomain against the correct TestPlatform.ObjectMo…

f248d4c

…del.dll

jnm2 force-pushed the jnm2/run_all_tests_with_adapter branch from 92d52f6 to bf648f5 Compare February 11, 2018 01:05

jnm2 commented Feb 11, 2018

View reviewed changes

jnm2 changed the title ~~[Do not merge] Test in all target environments using the correct version of the NUnit VSTest adapter~~ Test in all target environments using the correct version of the NUnit VSTest adapter Feb 11, 2018

jnm2 added 2 commits February 10, 2018 20:59

Make adapter internals visible to adapter tests

8d2408d

Enable engine tests to run in the same AppDomain as the adapter's engine

c802ff7

jnm2 force-pushed the jnm2/run_all_tests_with_adapter branch from bf648f5 to c802ff7 Compare February 11, 2018 01:59

jnm2 mentioned this pull request Feb 12, 2018

NUnit 3 Test Adaptor (3.9.0): Numerous InvalidOperationException from Mono.Cecil and no tests in Test Explorer nunit/nunit#2708

Closed

jnm2 added 2 commits February 12, 2018 20:43

Added DisableAppDomain.runsettings to solution items

c65d0e1

Test IDE and dotnet vstest separately

d050592

Added test debugging instructions to readme

f2a6f69

jnm2 mentioned this pull request Feb 15, 2018

Replaced Mono.Cecil #454

Merged

OsirisTerje approved these changes Feb 18, 2018

View reviewed changes

OsirisTerje merged commit 323b598 into master Feb 18, 2018

jnm2 deleted the jnm2/run_all_tests_with_adapter branch February 18, 2018 14:49

jnm2 mentioned this pull request Mar 13, 2018

Cannot run navigation tests under the console runner #138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test in all target environments using the correct version of the NUnit VSTest adapter #450

Test in all target environments using the correct version of the NUnit VSTest adapter #450

jnm2 commented Feb 10, 2018

jnm2 commented Feb 10, 2018

CharliePoole commented Feb 10, 2018

jnm2 commented Feb 10, 2018

jnm2 commented Feb 10, 2018 •

edited

Loading

jnm2 commented Feb 10, 2018

jnm2 commented Feb 11, 2018

jnm2 Feb 11, 2018

ChrisMaddock Feb 13, 2018

ChrisMaddock Feb 13, 2018

jnm2 commented Feb 11, 2018

jnm2 commented Feb 11, 2018 •

edited

Loading

jnm2 commented Feb 14, 2018

jnm2 commented Feb 14, 2018

jnm2 commented Feb 14, 2018

jnm2 commented Feb 15, 2018

OsirisTerje left a comment

Test in all target environments using the correct version of the NUnit VSTest adapter #450

Test in all target environments using the correct version of the NUnit VSTest adapter #450

Conversation

jnm2 commented Feb 10, 2018

jnm2 commented Feb 10, 2018

CharliePoole commented Feb 10, 2018

jnm2 commented Feb 10, 2018

jnm2 commented Feb 10, 2018 • edited Loading

jnm2 commented Feb 10, 2018

jnm2 commented Feb 11, 2018

jnm2 Feb 11, 2018

Choose a reason for hiding this comment

ChrisMaddock Feb 13, 2018

Choose a reason for hiding this comment

ChrisMaddock Feb 13, 2018

Choose a reason for hiding this comment

jnm2 commented Feb 11, 2018

jnm2 commented Feb 11, 2018 • edited Loading

jnm2 commented Feb 14, 2018

jnm2 commented Feb 14, 2018

jnm2 commented Feb 14, 2018

jnm2 commented Feb 15, 2018

OsirisTerje left a comment

Choose a reason for hiding this comment

jnm2 commented Feb 10, 2018 •

edited

Loading

jnm2 commented Feb 11, 2018 •

edited

Loading