Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.4.0 breaking change for TestAssemblyLoadContext #1066

Closed
dave-yotta opened this issue Feb 27, 2023 · 32 comments
Closed

4.4.0 breaking change for TestAssemblyLoadContext #1066

dave-yotta opened this issue Feb 27, 2023 · 32 comments

Comments

@dave-yotta
Copy link

Looks like it may be specific to roslyn generated types:

System.InvalidCastException : [A]Microsoft.CodeAnalysis.Scripting.Hosting.CommandLineScriptGlobals cannot be cast to [B]Microsoft.CodeAnalysis.Scripting.Hosting.CommandLineScriptGlobals. Type A originates from 'Microsoft.CodeAnalysis.Scripting, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' in the context '"" NUnit.Engine.Internal.TestAssemblyLoadContext #2' at location '/alloy-test/AlloyEngineTest/bin/x64/Debug/net7.0/Microsoft.CodeAnalysis.Scripting.dll'. Type B originates from 'Microsoft.CodeAnalysis.Scripting, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' in the context 'Default' at location '/alloy-test/AlloyEngineTest/bin/x64/Debug/net7.0/Microsoft.CodeAnalysis.Scripting.dll'.

When reporting a bug, please provide the following information to speed up triage:

  • NUnit and NUnit3TestAdapter versions: 4.4.0 / 17.5.0
  • Visual Studio edition and full version number (see Help About): Pro 17.4.4 (but error is on dotnet test command without VS installed)
  • A short repro, preferably attached or pointing to a git repo or gist: Looks like an object passed to a Script execution that comes from the test load context, while the script was compiled in the Default context.
  • What .net platform and version is being targeted: 7
  • If TFS/VSTS issue, what version, hosted or on-premises, and what build task you see this in: it's dotnet test running inside a docker container mcr.microsoft.com/dotnet/sdk:7.0
@paulhickman-a365
Copy link

I think I'm having the same issue. I've created a test solution at https://github.com/paulhickman-a365/Nunit440Bug

I'm using a .Net 6 project with VS2022 17.4.4. If you run the test with 4.4.0 if fails, but 4.3.1 is fine.

Foo, the class being tested is:

using Microsoft.Extensions.DependencyInjection;

namespace OtherAssembly
{
    public class Foo
    {
        public static void Bar(IServiceCollection serviceCollection)
        {

        }
    }
}

The test which loads the assembly that Foo is in dynamically is:

public class FooTest
    {
        [Test]
        public void SystemWebServicesTest() => RunTest(
            "..\\..\\..\\..\\OtherAssembly\\bin\\Debug\\net6.0",
            "OtherAssembly.dll"
        );


        private void RunTest(
            string assemblyFolder,
            string assemblyName)
        {
            //If we are running a release build of the tests, look for release builds of the rest of the code
            if (Environment.CurrentDirectory.Contains("\\Release\\", StringComparison.CurrentCulture))
            {
                assemblyFolder = assemblyFolder.Replace("\\Debug\\", "\\Release\\");
            }

            string assemblyPath = Path.Combine(assemblyFolder, assemblyName);
            if (!File.Exists(assemblyPath))
            {
                Assert.Fail($"{assemblyPath} does not exist. Have you built the full solution?.");
                return;
            }

            var context = new AssemblyLoadContext("test", true);
            context.EnterContextualReflection();
            context.Resolving += (context, childAssemblyName) =>
            {
                var tryPath = Path.Combine(assemblyFolder, childAssemblyName.Name + ".dll");
                if (File.Exists(tryPath))
                {
                    return context.LoadFromAssemblyPath(Path.Combine(Environment.CurrentDirectory, tryPath));
                }
                return null;
            };

            try
            {
                var assembly = context.LoadFromAssemblyPath(Path.Combine(Environment.CurrentDirectory, assemblyPath));
                var foo = assembly.GetTypes().FirstOrDefault(t => t.Name == "Foo");
                var bar = foo.GetMethod("Bar", BindingFlags.Static | BindingFlags.Public | BindingFlags.NonPublic);
                var services = new ServiceCollection();
                bar.Invoke(null, new[] { services }); //4.4.0 test adaptor fails here, 4.3.1 works
                context.Unload();
            }
            finally
            {
                AssemblyLoadContext.Default.EnterContextualReflection();
            }

        }
    }

The error message is:

 System.ArgumentException : Object of type 'Microsoft.Extensions.DependencyInjection.ServiceCollection' cannot be converted to type 'Microsoft.Extensions.DependencyInjection.IServiceCollection'.

@OsirisTerje
Copy link
Member

@paulhickman-a365 @dave-yotta Does this happen on both Windows and Linux ? We have another issue reported today that seems to only happen on Linux. They look related. #1065

Thanks for the repro @paulhickman-a365 , I'll have a look at it.

Can you also just dump the stack traces you see ?

@Methuselah96
Copy link

This happens on both Windows and Linux for me.

@OsirisTerje
Copy link
Member

@Methuselah96 What code do you have where this happens? Can you provide a short repro ?

@OsirisTerje
Copy link
Member

@paulhickman-a365 Trying your repro on Windows, and it works there.

@paulhickman-a365
Copy link

paulhickman-a365 commented Feb 27, 2023

@paulhickman-a365 Trying your repro on Windows, and it works there.

I accidentally pushed the repo on 4.3.1. I've updated the project reference to 4.4.0 now. I'm using Windows Server 2022.

@OsirisTerje
Copy link
Member

@paulhickman-a365 Thanks! Then it failed on Windows!

@OsirisTerje
Copy link
Member

@paulhickman-a365 The adapter 4.4.0 uses NUnit.Engine 3.16.3. This version of the engine uses Microsoft.Extensions.DependencyModel version 3.1.0 (in order to be compatible with net core 3.1, which we still support). If I change the references you have to the Microsoft.Extensions.DependencyInjection.Abstractions to the same version 3.1.0 (since I assume these are a "package deal"), then your code show compiler errors that look similar to the runtime errors.
image
in FooTest.cs
image

Since adapter 4.3.1 doesn't use this Microsoft package, it works. So, it looks like we need to rethink package version or possible the way this is solved in the engine

@daviddenis-stx
Copy link

Same issue here, SF reports a lot of failed tests with System.InvalidCastException (inhouse injection system) it happens when casting instances created using reflexion to a certain type. Like "var castedInstance = (T)instance". Works perfectly in 4.3.1. Kaboom in 4.4.0.

@paulhickman-a365
Copy link

It looks like the class ServiceCollection is in the assembly Microsoft.Extensions.DependencyInjection for versions <=5 but was moved to Microsoft.Extensions.DependencyInjection.Abstractions for version 6+

See dotnet/runtime#52284

I'm not sure I'm my issue is the same as those others are reporting anymore as it seems specific to ServiceCollection

@OsirisTerje
Copy link
Member

OsirisTerje commented Feb 27, 2023

@paulhickman-a365 It may explain it though, as I do see a lot of different crashes, and they all boil down to the loading of assemblies, one way or another. We have relied on using the same packages for all, keeping them at a low version, but also to avoid taking dependencies on too many external packages. I suspect this is another one of those where it doesn't work. So, we either have to package in for different frameworks, or not use this package at all. The latter seems more tempting.

PS: @daviddenis-stx What is SF ? Does the cast error happens also when not using your inhouse system - if so, can you provide a repro?

@daviddenis-stx
Copy link

Sorry, Software Factory. Upgrading the nuget package made the factory unit tests go red in any part using some degree of reflection :) NUnit3TestAdapter 4.3.1 green results, NUnit3TestAdapter 4.4.0 red results.

It fails in the region below, basically we just created an instance of something using type lookup, then calling the constructor (the instance is created correctly, that is it finds the assembly, the constructor, and invokes it nicely). Then it tries to cast the received "object" to T and fails with InvalidCastException. For example an instance of class MyThing : IMyThing will fail on the cast line, throwing an exception "Cannot cast to IMyThing". I suspect there's something weird with the reflexive loading of assemblies and the loading of types in 4.4.0 ?

        T cast;
        try
        {
            cast = (T)instance;

            // Log
            Logger.Debug($"CreateInstance: Instance was casted to {typeof(T)}");
        }
        catch (InvalidCastException invalidCastException)
        {
            // Log
            Logger.Error($"CreateInstance: Instance failed to be casted to {typeof(T)} with exception", invalidCastException);
            return Status.Failure<T>($"Instance failed to be casted to {typeof(T)} with exception", invalidCastException);
        }

@daviddenis-stx
Copy link

@OsirisTerje
Copy link
Member

@daviddenis-stx Yes, I think that's where it happens, and you see that the MS package was introduced there.

@paulhickman-a365
Copy link

Is it possible to workaround with a binding redirect? I tried adding

<?xml version="1.0" encoding="utf-8" ?>
<configuration>
	<dependentAssembly>
		<assemblyIdentity name="Microsoft.Extensions.DependencyInjestion.Abstractions"
		  publicKeyToken="adb9793829ddae60"
		  culture="en-us" />
		<bindingRedirect oldVersion="3.1.32.0" newVersion="7.0.0.0" />
	</dependentAssembly>
	<appSettings>
		<add key="foo" value="bar"/>
	</appSettings>
</configuration>

But I couldn't make it read the app setting from the app.config regardless if I called it that or Nunit440Bug.config as per http://charliepoole.org/technical/how-nunit-finds-config-files.html so I'm not sure if the binding direct won't workaround it or if the repo isn't reading the config file at all

@dave-yotta
Copy link
Author

I dont think binding redirects will help in my case:

This type:

Type: Microsoft.CodeAnalysis.Scripting.Hosting.CommandLineScriptGlobals
Assembly: Microsoft.CodeAnalysis.Scripting, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35
Context: NUnit.Engine.Internal.TestAssemblyLoadContext #2

Cannot be casted to this type:

Type: Microsoft.CodeAnalysis.Scripting.Hosting.CommandLineScriptGlobals
Assembly: Microsoft.CodeAnalysis.Scripting, Version=3.4.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35
Context: Default

So the only difference here is the load context.

@OsirisTerje
Copy link
Member

@rprouse We need a strategy here. Any thoughts?

@OsirisTerje
Copy link
Member

@daviddenis-stx Does it happen for any type of cast?

@OsirisTerje
Copy link
Member

@Methuselah96 @dave-yotta @daviddenis-stx @paulhickman-a365
Please find attached an alpha version of 4.4.1-alpha.1 and a nuget config file. The alpha version works now on @paulhickman-a365's repro. Can you all please verify that it also works on your solutions?
This alpha uses the 3.15.2 engine, but extended as a 3.15.3-dev engine with added support for .net 8. That is a "Point in time" where I am pretty sure we have a stable version. It will thus not have the fixes that are in the 3.16.X engine, but looking through the issues there I think it will work. The 4.3.1 adapter is based on 3.15.2 engine.
I suggest you add the package to a c:\nuget folder locally, and then add the nuget.config (in the nuget.zip file) to your solution, so that the package is found there.

NUnit3TestAdapter.4.4.1-alpha.1.zip
nuget.zip

@daviddenis-stx
Copy link

@Methuselah96 @dave-yotta @daviddenis-stx @paulhickman-a365 Please find attached an alpha version of 4.4.1-alpha.1 and a nuget config file. The alpha version works now on @paulhickman-a365's repro. Can you all please verify that it also works on your solutions? This alpha uses the 3.15.2 engine, but extended as a 3.15.3-dev engine with added support for .net 8. That is a "Point in time" where I am pretty sure we have a stable version. It will thus not have the fixes that are in the 3.16.X engine, but looking through the issues there I think it will work. The 4.3.1 adapter is based on 3.15.2 engine. I suggest you add the package to a c:\nuget folder locally, and then add the nuget.config (in the nuget.zip file) to your solution, so that the package is found there.

NUnit3TestAdapter.4.4.1-alpha.1.zip nuget.zip

This nuget package seems to solve the issues, good job !

@rprouse
Copy link
Member

rprouse commented Feb 28, 2023

@OsirisTerje .NET 3.1 is EOL, so we could retire it as a target.

Looking at this, nunit/nunit-console@3f854b2#diff-b82806cbe6520a13c308f28d24d01cdbd2639024609d411d529b5022bbe9401bR30, Microsoft.Extensions.DependencyModel was only added for .NET 3.1, but it is included in every 3.1+ target in nunit/nunit-console@3f854b2#diff-72ade0bfd4490ccb1ba5297dda7341e5729052d00436da77071eefb0d1545839R103. That seems incorrect to me?

@OsirisTerje
Copy link
Member

OsirisTerje commented Feb 28, 2023

@rprouse I agree. I did a quick look at retiring both net fw 2.0 and .net core 3.1, but it is quite some job to do that.
But if we can shoehorn in the right version in the cake script that might be doable, although I would assume we do need to touch the csprojs and the nuspec too.
I see the targets there for the console, but it's not for the engine

@SimonCropp
Copy link

@OsirisTerje thanks for the quick fix. any chance of getting that fix deployed? as a beta would suffice

@OsirisTerje
Copy link
Member

@SimonCropp I'll need to push out a beta (or even alpha) of the NUnit.Engine too. No problem getting the adapter out, as I have all the rights to do that, but I don't think I have similar rights for the Engine project, in particular on the Nuget feed...... hmm... perhaps I do... but would be nice if @rprouse could approve me doing that.

@SimonCropp
Copy link

@OsirisTerje should 4.4 be unlisted? it broke approx 50% of projects i used it on

@OsirisTerje
Copy link
Member

@SimonCropp I normally don't unlist, just add a hotfix and information in the release notes. I'll plan to get a hotfix 4.4.1 out today with the same code as in the dev version enclosed in #1066. It will probably use a locally built version of the engine, based on 3.15.2 but with the fix for the .net 8.

Can you test on some of your projects with NUnit.Console 3.16.3. You should get the same crashes there. Imho we should get a hotfix out there too (Issue nunit/nunit-console#1324) . I believe also the 3.16.2 is broken.

For anyone using the NUnit.COnsole in addition, they would need to then try the 3.15.2 version to work together with the upcoming 4.4.1.

OsirisTerje added a commit that referenced this issue Mar 1, 2023
* minor fixes to nuspec and update package for acceptance

* Fixes for issues #1065 and #1066

* updated version

* removed local feed
OsirisTerje added a commit that referenced this issue Mar 1, 2023
* minor fixes to nuspec and update package for acceptance

* Fixes for issues #1065 and #1066

* updated version

* removed local feed

* updated to 4.4.2, removed refs to dependencymodel package
@OsirisTerje
Copy link
Member

Hotfix version 4.4.2, nearly identical to the alpha version attached yesterday, is now released. The difference from the attached version is that the Microsoft.Extensions.DependencyModel is not include in the adapter package. It is not needed there, so it was removed. The release notes is here https://docs.nunit.org/articles/vs-test-adapter/AdapterV4-Release-Notes.html (includes some more explanations) and the package is uploaded to nuget https://www.nuget.org/packages/NUnit3TestAdapter/4.4.2. Thanks, everyone for reporting and providing repros!

@OsirisTerje OsirisTerje added this to the 4.4.1 milestone Mar 1, 2023
OsirisTerje added a commit that referenced this issue Mar 1, 2023
* minor fixes to nuspec and update package for acceptance

* Fixes for issues #1065 and #1066

* updated version

* removed local feed

* updated to 4.4.2, removed refs to dependencymodel package
OsirisTerje added a commit that referenced this issue Mar 1, 2023
* minor fixes to nuspec and update package for acceptance

* Fixes for issues #1065 and #1066

* updated version

* removed local feed

* updated to 4.4.2, removed refs to dependencymodel package

* UPdate appveyor
@SimonCropp
Copy link

@OsirisTerje can confirm the new version works. thanks

@OsirisTerje
Copy link
Member

Thanks for the confirmation @SimonCropp :-)

@gregsdennis
Copy link

☝️ different exception, but likely the same cause.

I also recommend unlisting 4.4.0.

@OsirisTerje
Copy link
Member

@SimonCropp @gregsdennis Ok, Ok, you guys win! It's unlisted!!

@dave-yotta
Copy link
Author

v4.4.2 is working thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants