Merge .NET Standard Engine code back into the main solution #479

rprouse · 2018-10-07T20:23:48Z

This is the work @ChrisMaddock and I have been doing on merging the .NET Standard version of the engine back into the main solution. I am putting the pull request up because we are getting close to done and wanted to involve the team in the final steps.

I think that the tasks that we want to complete before a merge are;

Add the .NET Standard engine and engine.api into the NuGet packages
Test this version of the engine in the adapter in Visual Studio and with dotnet test on the command line.

Once this is complete, we can start exploring the other enhancements that @ChrisMaddock has entered recently like allowing the console runner to run Core tests, adding a Core console runner, allowing the engine to load Core extensions, adding back features to the .NET Standard engine like communicating to an agent, etc.

…g work

ChrisMaddock · 2018-10-08T12:37:39Z

Thanks for putting this up Rob. (And fixing my NuGet packaging. 😩)

Never asked before - what was your thinking on adding a .NET Standard 1.3 engine - was it Xamarin support? I ask, as I'm not sure the dependencies are available on the Xamarin platforms (XmlDocument appears not to be available on iOS/Android - although I'd have to dig more deeply to check!) If we're only intending to support .NET Core with this engine however, then I'm not sure why you dropped the target level from 1.6?

rprouse · 2018-10-12T01:35:00Z

I've tested this build in the adapter and everything seems to be working fine in my test projects, so I think we are pretty close to being able to merge this.

what was your thinking on adding a .NET Standard 1.3 engine

To be honest, this has been on the back burner so long that I forget why I made that change 😦 The adapter needs to be compiled .NET Core 1.0 which is fine for 1.6 and the latest versions of Xamarin support it. It might have been for the original Windows 10 UWP, but even that could go to 1.4. We should probably bring it up to latest we can manage. Do you think that should be 1.4 for UWP support, or 1.6 for more features?

ChrisMaddock · 2018-10-12T08:07:52Z

Hmm, we don’t have a runner for original UWP, but of course that would allow others to consume the engine.

I wonder if we should stick at 1.6 for now, until that demand is expressed? UWP is a very small part of our userbase - so much so, I’d be tempted to wonder if the portable agent could be sufficient.

ChrisMaddock · 2018-10-13T11:26:08Z

I had CI working before, but it now keeps failing at the same point in restoring packages on iOS. 😞

I may give it a whirl on VSTS at some point instead...

rprouse · 2018-10-13T19:50:39Z

I updated to .NET Standard 1.6 and the only new functionality that it enabled was getting the command line arguments. The two other big areas that I'd hoped to enable, extensions and running tests async, were still not possible because Assembly.LoadFrom and Thread are still not available in 1.6. We may as well leave these at 1.3 in the hope of adding full UWP support to the adapter.

I'm pushing some minor fixes that I found while going through trying to enable features for 1.6.

I think this is ready for review.

ChrisMaddock

Wow, this was a marathon! Didn't find any major problems, just a few things that might be nice to tidy up. Lot's of these are simple things that I can do myself - but I'll come back to them another day.

Looks great Rob! 😄

src/NUnitEngine/nunit.engine.api/TestEngineActivator.cs

src/NUnitEngine/nunit.engine.tests.netstandard/Drivers/NUnitNetStandardDriverTests.cs

src/NUnitEngine/nunit.engine.tests/Extensibility/ExtensionAssemblyTests.cs

ChrisMaddock · 2018-10-14T11:54:29Z

src/NUnitEngine/nunit.engine/Services/ExtensionService.cs

@@ -434,6 +447,7 @@ internal void FindExtensionsInAssembly(ExtensionAssembly assembly)
                    return;
                }
            }
+#endif


Removing this functionality, I think means the Engine will crash if it encounters an extension targeting a framework it can't load. If we can't inspect, perhaps we should try/catch instead?

I added a unit test for this where the .NET Core 2.0 targeted tests try to load extensions from the .NET 3.5 assembly and visa-versa. Interestingly, .NET 3.5 correctly throws an NUnitEngineException because it thinks the .NET Core 2.0 targets the 4.0 full framework. The .NET Core 2.0 seems to successfully load the .NET 3.5 extensions. Both of these behaviors seem wrong to me.

src/NUnitEngine/nunit.engine/Drivers/NUnit2DriverFactory.cs

src/NUnitEngine/nunit.engine/AsyncTestEngineResult.cs

src/NUnitEngine/nunit.engine/RuntimeFramework.cs

src/CommonAssemblyInfo.cs

src/NUnitEngine/nunit.engine.tests/nunit.engine.tests.csproj

src/NUnitEngine/nunit.engine.tests/Internal/PathUtilTests.cs

ChrisMaddock · 2018-10-14T12:51:46Z

We may as well leave these at 1.3 in the hope of adding full UWP support to the adapter.

This sounds like a sensible enough justification to me! 👍

ChrisMaddock · 2018-11-09T18:08:30Z

Great, thanks Rob - and no worries!

…orking

rprouse · 2018-11-20T02:14:10Z

@ChrisMaddock I've resolved the merge conflicts, updated to .NET Standard 1.6 and got the load engine from another directory test passing. See the changes in TestEngineActivator.CreateInstance() for .NET Standard 1.6.

ChrisMaddock · 2018-11-20T10:06:46Z

Awesome! Sorry, I haven't had time to get back to this recently!

I started a PR nunit/nunit3-vs-adapter#562 to update the adapter to the current engine release, so that we can hopefully easily switch to this version once it's done. Unfortunately that seems to be non-trivial, but it's a WIP!

rprouse · 2018-11-23T01:54:41Z

Looks like my recent changes or maybe the merge from master broke packaging in Cake. I am fixing it. @ChrisMaddock anything else we should do before merging this PR and moving on to the additional tasks? I started this branch back in May, so it would be nice to get it behind us 😄

rprouse · 2018-11-23T03:18:34Z

Packaging has been fixed, now the new load engine from another directory is failing on Travis Linux on .NET 2.0

1) Failed : NUnit.Engine.Services.Tests.ExtensionServiceTests.FailsGracefullyLoadingOtherFrameworkExtensionAssembly
  Expected: <NUnit.Engine.NUnitEngineException>
  But was:  no exception thrown
  at NUnit.Engine.Services.Tests.ExtensionServiceTests.FailsGracefullyLoadingOtherFrameworkExtensionAssembly () [0x00000] in <839ae0be2830488ba956d1dfeb047f37>:0

ChrisMaddock · 2018-11-23T11:50:32Z

Yes - let’s get this merged! I’m pretty busy at the moment, but will try and give it a final look over at some point, unless you’re happy that the latest changes are simple?

Also - the failing test you’ve posted looks like a different test from the newly added one? Can’t see the appveyor logs on my phone myself.

rprouse · 2018-11-23T12:45:36Z

the failing test you’ve posted looks like a different test from the newly added one? Can’t see the appveyor logs on my phone myself.

You're right, I misread that. I didn't get much of a chance to look at it last night. I ended up struggling with getting my Linux environment instead. I'll probably be able to get back to it tomorrow.

The latest changes I made are fairly small. You can concentrate a review on the new .NET Standard 1.6 version of TestEngineActivator.CreateInstance. That was the only change of substance.

rprouse · 2018-11-23T12:48:13Z

Further to that, we might be able to use the NETSTANDARD1_6 version of CreateInstance for the NETSTANDARD2_0 target. I was just playing it safe.

ChrisMaddock

Looks good, Rob! Few simple corrections left - would be good to double check the nuspec dependency noted is still required, if you haven't already. Other than that, I think the last thing to do is fix this test! 😄

My week's pretty busy still - might get a chance to have a quick look at the weekend. 🙂

ChrisMaddock · 2018-11-27T11:37:18Z

src/CommonAssemblyInfo.cs

@@ -47,7 +47,7 @@
 [assembly: AssemblyConfiguration(".NET 3.5")]
 #elif NET20
 [assembly: AssemblyConfiguration(".NET 2.0")]
-#elif NETSTANDARD1_3 || NETCOREAPP1_1
+#elif NETSTANDARD1_6 || NETCOREAPP1_1
 [assembly: AssemblyConfiguration(".NET Standard 1.3")]


This line needs updating, and it's friend on line 29. 😄

ChrisMaddock · 2018-11-27T11:39:46Z

nuget/engine/nunit.engine.nuspec

@@ -19,7 +19,7 @@
    <copyright>Copyright (c) 2018 Charlie Poole, Rob Prouse</copyright>
    <dependencies>
      <group targetFramework="net20" />
-      <group targetFramework="netstandard1.3">
+      <group targetFramework="netstandard1.6">


We should check if any dependencies are redundant with the retargetting - looks like System.Reflection.TypeExtension is the only possibility. 😄

Update - looks like it can be removed, commit incoming. 😄

src/CommonAssemblyInfo.cs

build.cake

ChrisMaddock · 2018-11-27T13:09:27Z

Assigned myself so I could apply PR 'code suggestions' (which were all trivial) - that was a super sweet experience! 😄

…ly() and remove old dependency

ChrisMaddock · 2018-11-27T22:23:23Z

Last commit fixes the two outstanding issues - so to my mind, this is now ready to be merged! 😱 @rprouse - I'll leave it with you to merge when you're happy!

Last things I patched up:

System.Reflection.TypeExtension dependency needed under NS1.3 seemed unnecessary under NS1.6, so I removed it
The failing test was due to Assembly.GetEntryAssembly() returning null on mono, and thus the framework loading validation not being run. I just added the line below for now - although I assume trying to load a Net Core extension under mono would now lead to an noisier failure later in the process. Maybe something we should log for future...

//May be null on mono
Assume.That(Assembly.GetEntryAssembly(), Is.Not.Null, "Entry assembly is null, framework loading validation will be skipped.");

mikkelbu · 2018-11-27T22:34:48Z

We also had some unexpected behaviour regarding Assembly.GetEntryAssembly() in nunit/nunit#3084 (comment).

rprouse · 2018-11-28T02:44:25Z

Looks good, I was just looking at making the same fixes tonight 😄

If anyone is using it, I updated the docker image that build-mono-docker.ps1 uses to the latest release of .NET Core 2.6.1. I was using that for testing locally.

rprouse and others added 17 commits May 16, 2018 22:09

nunit.engine.api now supports .NET Standard 1.3 and 2.0

1163d0e

NUnit.Engine is now compiling .NET Standard 1.3 and 2.0

49945d4

Checkpointing work getting the tests to compile

770f8cd

Engine tests are compiling and running, but not passing. Checkpointin…

6ada4b3

…g work

Checkpointing test fixes

3bb3565

All .NET Standard/Core 2.0 tests passing

645fd3b

Merge latest from master

b21d378

Update the Cake build

40e6da2

Merge latest from master

135710a

Run .NET Standard 1.3 and 2.0 tests

10c247c

Get .NET Standard 1.3 Engine tests passing

bb2305c

Create multi-platform image and fix existing .NET 2.0 packaging

b9430cc

Add extra level of redirection

cb738a9

Add packaging for all platforms

2ff720c

Remove old .NET Standard specific classes

6f0427f

Get tests working on OSX and Linux

6cc8b28

Specify .NET Core runtime versions

aab445f

rprouse requested review from ChrisMaddock and jnm2 October 7, 2018 20:23

rprouse added 2 commits October 7, 2018 16:48

Put the NuGet assemblies in platform appropriate directories

ffd06d8

Add dependencies to the Nuspec files

06c4597

Enable extensions in .NET Standard 2.0

fa097fb

rprouse changed the title ~~WIP - .NET Standard Engine~~ Merge .NET Standard Engine code back into the main solution Oct 13, 2018

ChrisMaddock reviewed Oct 14, 2018

View reviewed changes

ChrisMaddock mentioned this pull request Nov 15, 2018

Update to NUnit.Engine 3.9.0, and netstandard engine 3.8.0 nunit/nunit3-vs-adapter#562

Merged

rprouse added 2 commits November 19, 2018 19:05

Merge remote-tracking branch 'origin/master' into issue-388b

d2b8a00

Switch to .NET Standard 1.6 from 1.3 and get the assembly load test w…

bf57ae5

…orking

Fixed packaging of NuGet packages

6dc6c78

ChrisMaddock previously approved these changes Nov 27, 2018

View reviewed changes

ChrisMaddock assigned ChrisMaddock and rprouse Nov 27, 2018

Apply suggestions from code review

5c9e23e

ChrisMaddock dismissed stale reviews from themself via 5c9e23e November 27, 2018 13:08

ChrisMaddock force-pushed the issue-388b branch 2 times, most recently from 4b163b6 to 014340a Compare November 27, 2018 22:09

Add debug info to FailsGracefullyLoadingOtherFrameworkExtensionAssemb…

014340a

…ly() and remove old dependency

ChrisMaddock approved these changes Nov 27, 2018

View reviewed changes

rprouse merged commit aaef18f into master Nov 28, 2018

rprouse deleted the issue-388b branch November 28, 2018 02:44

ChrisMaddock mentioned this pull request Dec 5, 2018

Re-Enable OSX CI tests #508

Closed

mikkelbu added this to the 3.10 milestone Dec 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge .NET Standard Engine code back into the main solution #479

Merge .NET Standard Engine code back into the main solution #479

rprouse commented Oct 7, 2018 •

edited

ChrisMaddock commented Oct 8, 2018

rprouse commented Oct 12, 2018

ChrisMaddock commented Oct 12, 2018

ChrisMaddock commented Oct 13, 2018

rprouse commented Oct 13, 2018

ChrisMaddock left a comment

ChrisMaddock Oct 14, 2018

rprouse Oct 16, 2018

ChrisMaddock commented Oct 14, 2018

ChrisMaddock commented Nov 9, 2018

rprouse commented Nov 20, 2018

ChrisMaddock commented Nov 20, 2018 •

edited

rprouse commented Nov 23, 2018

rprouse commented Nov 23, 2018

ChrisMaddock commented Nov 23, 2018

rprouse commented Nov 23, 2018

rprouse commented Nov 23, 2018

ChrisMaddock left a comment

ChrisMaddock Nov 27, 2018

ChrisMaddock Nov 27, 2018

ChrisMaddock Nov 27, 2018

ChrisMaddock commented Nov 27, 2018

ChrisMaddock commented Nov 27, 2018

mikkelbu commented Nov 27, 2018

rprouse commented Nov 28, 2018

Merge .NET Standard Engine code back into the main solution #479

Merge .NET Standard Engine code back into the main solution #479

Conversation

rprouse commented Oct 7, 2018 • edited

ChrisMaddock commented Oct 8, 2018

rprouse commented Oct 12, 2018

ChrisMaddock commented Oct 12, 2018

ChrisMaddock commented Oct 13, 2018

rprouse commented Oct 13, 2018

ChrisMaddock left a comment

Choose a reason for hiding this comment

ChrisMaddock Oct 14, 2018

Choose a reason for hiding this comment

rprouse Oct 16, 2018

Choose a reason for hiding this comment

ChrisMaddock commented Oct 14, 2018

ChrisMaddock commented Nov 9, 2018

rprouse commented Nov 20, 2018

ChrisMaddock commented Nov 20, 2018 • edited

rprouse commented Nov 23, 2018

rprouse commented Nov 23, 2018

ChrisMaddock commented Nov 23, 2018

rprouse commented Nov 23, 2018

rprouse commented Nov 23, 2018

ChrisMaddock left a comment

Choose a reason for hiding this comment

ChrisMaddock Nov 27, 2018

Choose a reason for hiding this comment

ChrisMaddock Nov 27, 2018

Choose a reason for hiding this comment

ChrisMaddock Nov 27, 2018

Choose a reason for hiding this comment

ChrisMaddock commented Nov 27, 2018

ChrisMaddock commented Nov 27, 2018

mikkelbu commented Nov 27, 2018

rprouse commented Nov 28, 2018

rprouse commented Oct 7, 2018 •

edited

ChrisMaddock commented Nov 20, 2018 •

edited