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

Wrong assembly resolving logic when several runtimes are installed and assembly is missing in deps.json #1330

Closed
svg2003 opened this issue Apr 5, 2023 · 8 comments
Assignees
Labels
Milestone

Comments

@svg2003
Copy link
Contributor

svg2003 commented Apr 5, 2023

Suppose we have Test.dll, targeted for net6 that is using some net472 targeted assembly (RealCode.dll).
RealCode.dll is using some assembly, that is NOT in deps.json file for Test.dll. Let it be System.Windows.Forms

When TestAssemblyResolver is trying to resolve this assembly, it will get:

private Assembly OnResolving(AssemblyLoadContext context, AssemblyName name)
{
DebugLog("OnResolving {0} version {1}", name.Name, name.Version); => [TestAssemblyResolver][OnResolving]: OnResolving System.Windows.Forms version 4.0.0.0

Now, if test computer has several Runtimes installed, Shared folder would have several folders for every installed runtime

_Directory of C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App

01/28/2023 11:53 AM

.
01/28/2023 11:53 AM ..
06/18/2022 01:16 AM 3.1.26
06/27/2022 10:03 PM 5.0.17
10/21/2022 01:27 AM 6.0.10
11/23/2022 02:21 PM 6.0.11
01/28/2023 11:53 AM 6.0.13
01/28/2023 03:48 AM 7.0.2_

So, when TestAssemblyResolver would start enumerate those folders, it will load wrong version (5.0.17) instead of some one from 6.0.x, that is in reality using agent:

https://github.com/nunit/nunit-console/blob/main/src/NUnitEngine/nunit.engine.core/Internal/TestAssemblyResolver.cs#L86

foreach(string frameworkDirectory in AdditionalFrameworkDirectories)
{
DebugLog("AdditionalDir: {0}", frameworkDirectory); => [TestAssemblyResolver][OnResolving]: AdditionalDir: C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App
var versionDir = FindBestVersionDir(frameworkDirectory, name.Version);
DebugLog("VerDir: {0}", versionDir); => [TestAssemblyResolver][OnResolving]: VerDir: 5.0.17
if (versionDir != null)
{
string candidate = Path.Combine(frameworkDirectory, versionDir, name.Name + ".dll");
if (File.Exists(candidate))
return _loadContext.LoadFromAssemblyPath(candidate);
}
}

@svg2003
Copy link
Contributor Author

svg2003 commented Apr 5, 2023

I think, more or less correct, would be check assemblies from TRUSTED_PLATFORM_ASSEMBLIES variable before to go into advance search logic.
I'll check tomorrow on our test cases, and nothing would break - probably will create PR

https://learn.microsoft.com/en-us/dotnet/core/dependency-loading/default-probing

@OsirisTerje
Copy link
Member

@svg2003
Copy link
Contributor Author

svg2003 commented Apr 6, 2023

#1325 - likely would be fixed with my commit.
At least it failed before, and pass after commit.
Actually, I'd say, this is exactly how it supposed to work, so, strictly say, MissingMethodException would be from other test runner (for example, resharper) as well, but never the less, I decided to fix it.
The issue is - nunit is using Microsoft.Extensions.DependencyModel.dll which is refer on System.Text.Json. When nunit is loading, it will use dll (and load assembly into AppDomain) from framework's folder (6.0.0 in my case).
After, loader would try to resolve "System.Text.Json 7.0.0" and return it from AppDomain, where is loaded version 6.0.0. But test is expected version 7.0.0, so will throw an exception
Previously it worked, because there was no Microsoft.Extensions.DependencyModel.dll, so was no System.Text.Json dependency

@CharliePoole
Copy link
Member

I hope you'll consider adding package tests to demonstrate each issue you resolve. It's somewhat impractical to have true unit tests for behavior at this level but a simple package test, i.e. a system-level test based on actually installing the package produced by the build accomplishes this and prevents regressions. The build script has all the infrastructure for this so you only need to do is create a repro in the form of a simple assembly and add the test definition to the script.

@svg2003
Copy link
Contributor Author

svg2003 commented Apr 6, 2023

@CharliePoole ,

Could you please guide me, or add test package yourself?

Below issue report has attached zip file. Test project2 will fail now and will pass after my commit.
So, how to add such test into nunit tests?
#1325

@OsirisTerje
Copy link
Member

OsirisTerje commented Apr 6, 2023

@CharliePoole I don't see any explicit package tests in the project. I do see some small integration tests for the engine, and some command line tests for the console. Are there any acceptance/integration tests at that level anywhere that I'm missing ?

UPDATE: Ok, I found some definitions in the package-tests.cake script. I'll take a look at those and see if I can get TestProject1.zip in there.

@CharliePoole
Copy link
Member

Yes, the script consists of .cake files, which are (except for build.cake) in the cake directory. This is something I had experimented with for a while, so it's at different stages in each project I worked on and the version3 and version 4 engine code is different as well.

In the version3 branch, cake/build-settings.cake has the definitions of the tests and the logic for running them is in cake/package-definitions. The individual packages we create are defined in cake/packages.cake. Note that this code is more advanced than the main branch, which uses an earlier version of the script. Ideally, the script should be a package itself, referenced by projects that use it. That's the direction I had planned to take with the engine and have now done with TestCentric.

It sounds as if you are on the right path @OsirisTerje and I hope you'll find this useful. To be frank, I couldn't have put out releases of the engine over the past year without having these tests to lean on.

@OsirisTerje
Copy link
Member

I'll add package tests for this and others, and will add that to a new issue.

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

No branches or pull requests

3 participants