Merged
Conversation
Opened the project in godot 4.1 and allowed the auto upgrade to do its thing.
Godot updated the SDK attribute in the GodotXUnit.csproj file. Some resources updated as well.
It replaced occurrences of Path with Path3D (thinking it was the Godot node type Path). In fact it's System.IO.Path.
CharacterBody2D now has a build in Velocity property which we'll use. UpDirection is also defaulted to Vector2.Up
Trying to run the plugin gave the error below.
The tag CopyLocalLockFileAssemblies seems to fix the error, as the
xunit.core.dll is included with the output files.
```
Godot Engine v4.1.beta2.mono.official (c) 2007-present Juan Linietsky, Ariel Manzur & Godot Contributors.
modules/gltf/register_types.cpp:72 - Blend file import is enabled in the project settings, but no Blender path is configured in the editor settings. Blend files will not be imported.
--- Debug adapter server started ---
--- GDScript language server started ---
modules/mono/glue/runtime_interop.cpp:1325 - System.IO.FileNotFoundException: Could not load file or assembly 'xunit.core, Version=2.4.1.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c'. The system cannot find the file specified.
File name: 'xunit.core, Version=2.4.1.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c'
at System.ModuleHandle.ResolveType(QCallModule module, Int32 typeToken, IntPtr* typeInstArgs, Int32 typeInstCount, IntPtr* methodInstArgs, Int32 methodInstCount, ObjectHandleOnStack type)
at System.ModuleHandle.ResolveTypeHandle(Int32 typeToken, RuntimeTypeHandle[] typeInstantiationContext, RuntimeTypeHandle[] methodInstantiationContext)
at System.Reflection.RuntimeModule.ResolveType(Int32 metadataToken, Type[] genericTypeArguments, Type[] genericMethodArguments)
at System.Reflection.CustomAttribute.FilterCustomAttributeRecord(MetadataToken caCtorToken, MetadataImport& scope, RuntimeModule decoratedModule, MetadataToken decoratedToken, RuntimeType attributeFilterType, Boolean mustBeInheritable, ListBuilder`1& derivedAttributes, RuntimeType& attributeType, IRuntimeMethodInfo& ctorWithParameters, Boolean& isVarArg)
at System.Reflection.CustomAttribute.AddCustomAttributes(ListBuilder`1& attributes, RuntimeModule decoratedModule, Int32 decoratedMetadataToken, RuntimeType attributeFilterType, Boolean mustBeInheritable, ListBuilder`1 derivedAttributes)
at System.Reflection.CustomAttribute.GetCustomAttributes(RuntimeMethodInfo method, RuntimeType caType, Boolean inherit)
at System.Reflection.RuntimeMethodInfo.GetCustomAttributes(Boolean inherit)
at Godot.Bridge.ScriptManagerBridge.UpdateScriptClassInfo(IntPtr scriptPtr, godot_string* outClassName, godot_bool* outTool, godot_bool* outGlobal, godot_string* outIconPath, godot_array* outMethodsDest, godot_dictionary* outRpcFunctionsDest, godot_dictionary* outEventSignalsDest, godot_ref* outBaseScript) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 698
```
Don't know what changed to make the GetExecutingAssembly() return an assembly with an empty Location property, but it did and thus made the AssemblyRunner.WithoutAppDomain fail its file-exists check even though we had a valid assembly. There's no way to pass a fully qualified Assembly instance to the XUnit AssemblyRunner, only a path string. Reworked the target assembly resolution and so it now passes a full path to the default assembly correctly and there are no errors. However it still doesn't detect any tests.
The assemblies would continue to not be located correctly even though a fully qualified path was passed to the AssemblyRunner. I attached an event handler to the app domain's AssemblyResolve event to retry locating the assembly. Cudos to this article: https://weblog.west-wind.com/posts/2016/dec/12/loading-net-assemblies-out-of-seperate-folders Trying to run the tests/ project assemblies still complained about xunit.core and xunit.assert to be missing. Adding the CopyLocalLockFileAssemblies flag to those projects fixed the issue.
Godot only allows specific threads to access Node as a safety precaution. This makes most tests always fail since XUnit spawns at least one thread to run tests in (even if parallel is disabled) To get around this, all tests are now invoked inside a deferred lambda that will run on Godot's main thread. A semaphore is then used to let XUnit wait for the test to finish.
Contributor
|
You might want to try this way of layouting multiple projects in a solution Otherwise I am seeing a weird compilation issues like |
Contributor
Author
Absolutely! I decided not to for keeping this PR less noisy tho.
Not happening here. Did you checkout from an existing clone of the repo? Maybe some old build causing it? |
Contributor
|
May happen if you rebuild all. I had a clean checkout. |
Contributor
Author
|
@van800 Sorry I can't reproduce it even with a clean build. The file is created during the build for me. Will adding this to the failing csproject fix it for you? |
Contributor
Author
|
@van800 What's your output when running |
Contributor
dotnet --list-sdks |
Contributor
|
If I rebuild only
No, it didn't help. |
Contributor
|
The problem is that output path for both |
Closed
Member
|
Very nice work. I'll be taking a deeper dive into this within the next week. Thanks! |
Member
|
The PR looks great. I found a a couple issues:
the actual code issues don't seem to be related to any of your changes. The first is because godot changed the equals implementation of variant. The second seems to be where/how assemblies are created in godot. The I have the fixes locally, but I don't think I have permissions to push to this PR I believe...? IDK. So, we can either merge this and I can put in those fixes, or you can. I don't mind either way. |
Member
|
Another weird thing that I've noticed is that native calls related to threads seem to hang sometimes. Specifically, |
Contributor
Author
|
@rexfleischer Good to hear from you! You can go ahead and merge it and add your fixes. I think you can retarget this PR to merge to another branch, like a new godot4 branch on this repo, if you'd prefer to keep the master in working order while the bugs are ironed out. The hanging is worrying for sure and probably due to the Wait() mentioned. I think it'd be possible to make it fully async or wrap it so that it doesn't lock that thread. I'll try to have a look today and get back to you! |
Contributor
Author
|
@rexfleischer You can also just push your branch to the main repo and close this PR (which it actually might do automatically due to all the commits being in your push) |
Contributor
Author
|
Think I've got a fix for the threading issue, but I'll make a new PR for that as soon as you've merged this! :) |
Member
|
The main branch is broken right now, so merging this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three main things in this PR in order to get the project working in Godot 4.
Auto-porting done by Godot and fixing the build
Godot automatically converted most of the code but still required a fair bit of manual fixes to get it to compile. Quite possible that regressions have been introduced at this step.
Also updating the language to C# 10.
Assembly loading
Assembly loading was broken, and I'm not entirely sure what changed for it to break. I think that the main (the godot game project's) assembly is loaded differently than before resulting in it not having a Location attribute anymore. I refactored the code a bit to pass around the assembly paths instead of the
Assemblyobjects, since theXunit.AssemblyRunnertakes the path and not theAssemblyobject (bit of a weird design limitation honestly, but probably has some reasons).Oddly enough it still fails to load certain assemblies even with the fully qualified path, but some research showed others have had this problem when dynamically loading assemblies. Turns out there's an event on
AppDomaincalledAssemblyResolvethat lets you try to find the assembly yourself if the default method failed to locate it. In fact, it's not uncommon for the assembly to already be loaded in the domain even though it failed to find it! OurAssemblyResolvethus searches already loaded assemblies, and if that fails, attempts to locate the.dllvia the expected bin path, which appears to be working correctly.Thread safety
Godot recently added thread safety checks on
Node, which causes errors if accessing any Node methods while not in the safe main thread. XUnit always runs the test cases in at least one new separate thread, even when parallelism is disabled, so these guards were always triggered in tests using Nodes. The fix I devised wraps the test case withCallInGodotMainwhich sends it off to Godot's main thread by using CallDeferred. Just to be clear, deferred calls are put in a queue that is continually processed until empty, sort of like Javascript's SetTimeout. It's not deferred to the next frame, as many people online seem to believe.Known bugs
Potential risks
CallInGodotMainmethod uses a blocking wait. It doesn't seem to pose a problem though, but there seems to be a risk of deadlocks if a thread blocks while using async methods.Additional fixes to consider
Some things that popped out while working on this, but kept it out to minimize the changes for this PR.
.csfiles in the.csproj