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

Integrate TestAssemblyLoadContext changes to version315 #1376

Merged

Conversation

veleek
Copy link
Contributor

@veleek veleek commented Jan 9, 2024

As per discussion in #1370, this change integrates all of the updates to TestAssembyLoadContext (nee CustomAssemblyLoadContext) from the main to the version315 branch. Unfortunately, there have been some changes to some of the packaging bits and pieces in order to add in the extra library used by TestAssemblyResolver, so I had to get those changes all merged as well.

Here is the set of commits that touched TestAssemblyLoadContext.cs and TestAssemblyResolver.cs (from oldest to newest) and what I did to resolve them:

3f854b2 - Manual resolution needed to deal with packaging changes due to new dependency.
f11499a - Do we need this change? I merged the modifications to TestAssemblyLoadContext but just ignored everything else related to project changes for extra tests.
485153d - Clean
b01ad3e - Clean
38d141e - Clean
462d9d2 - Clean

Note: Merge commits c756682, b92df46, b92df46, 8f472b0, and 887516a were just ignored.

@veleek
Copy link
Contributor Author

veleek commented Jan 9, 2024

@OsirisTerje - This is probably more complicated than I'm assuming you were expecting when you suggested raising this PR for version315. Before I spend any time testing this and verifying it works, can you confirm that you think this is a reasonable change to make? I have not even bothered building this yet.

If you do still want to go ahead with this, is there anything that I need to know around building/testing the changes that were done to the different packages files (cake/choco/msi)?

@OsirisTerje
Copy link
Member

OsirisTerje commented Jan 10, 2024

I think we should try to move on with this. There are a comprehensive suite of tests in the console. Do all of them run?

build --target test

PS: I am on travel now, and my laptop doesnt have all the frameworks installed, and slow internet lines, so I can't check it. I also got some vulnerability warnings.

@OsirisTerje OsirisTerje changed the title Integrate TestAssemblyLoadContext changes to version315 WIP: Integrate TestAssemblyLoadContext changes to version315 Jan 24, 2024
@veleek
Copy link
Contributor Author

veleek commented Feb 21, 2024

Okay, finally found a few minutes to troubleshoot this. Had to add some extra dependencies for .NET 5 and .NET 6. Verified tests all pass on Windows, I don't have any way to easily test on other platforms.

Attached build/test log given the above command.
nunit_build.log

@veleek veleek changed the title WIP: Integrate TestAssemblyLoadContext changes to version315 Integrate TestAssemblyLoadContext changes to version315 Feb 21, 2024
@veleek
Copy link
Contributor Author

veleek commented Mar 17, 2024

@OsirisTerje - Quick bump in case this you missed this one.

@OsirisTerje
Copy link
Member

Not missed, but lack of time. I need to work through the different changes you have done, to ensure the bugs in 3.16.X is not coming over. Even if you're pretty sure it is, a second look here should be done.

@veleek
Copy link
Contributor Author

veleek commented Mar 17, 2024

Sounds good! Absolutely no pressure intended from my end. I'm not blocked in any way, just wanted to make sure it wasn't lost in the cracks (because I took so long for the original follow up).

@OsirisTerje
Copy link
Member

@veleek

  1. Can you redirect this PR to branch "version3X" ?
  2. Any possibility you could test that the engine created from this PR will work with the NUnit3TestAdapter?

@veleek veleek changed the base branch from version315 to version3X April 4, 2024 00:05
veleek and others added 7 commits April 3, 2024 20:55
Try to load assembly from trusted list, before to start using some complex logic and enumerate AdditinalFrameworkDirectories
Improve logging for assembly resolving process
(cherry picked from commit f642e99)
Loading of unmanaged assemblies suffers from the same issues as loading managed assemblies in that the wrong deps.json is used by default.  This updates the TestAssemblyLoadContext to use the exact same pattern for loading unmanaged assemblies as managed assemblies.
1. Use the default assembly loading logic (I'm unsure if we actually need this since, in both managed and unmanaged, the base AssemblyLoadContext logic is a no-op and it's actually the VM that has some logic for loading assemblies).
2. Use an AssemblyDependencyResolver for the test assembly.
3. Check in the same folder as the test assembly (in case the dependencies are not fully specified in deps.json).

Resolves nunit#1253
@veleek veleek force-pushed the veleek/native-assembly-resolve-v315 branch from 696af74 to 87d6996 Compare April 4, 2024 00:56
…hich were added/merged after this PR was created.
@veleek
Copy link
Contributor Author

veleek commented Apr 4, 2024

  1. Can you redirect this PR to branch "version3X" ?

Okay, I just rebased this onto version3X and retargeted the PR. I had to make some extra changes because .NET 7 and 8 support were added, but everything seems to build and test fine now.

  1. Any possibility you could test that the engine created from this PR will work with the NUnit3TestAdapter?

Is there some semi-automated way to do this? I got as far as figuring out how the nunit3-vs-adapter repo builds, and I feel like I would need to publish a NuGet package with my updated binaries, then rebuild the adapter and then pull it into a test project.

I did the bare minimum testing and it seemed to work:

  1. Grab NUnit3.TestAdapter.dll from the published NuGet package
  2. Grab nunit.framework/engine.dll et. al. from my local build.
  3. Create a project and manually add references to those DLLs.
  4. Create a test and run it from Visual Studio and verify it works.

@OsirisTerje OsirisTerje merged commit a0e18d3 into nunit:version3X May 22, 2024
2 checks passed
@veleek veleek deleted the veleek/native-assembly-resolve-v315 branch May 22, 2024 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants