Feedback on install

[remi-san]

First, thank you for developing this addon. Once it works, it is a very good way to unit test Godot objects with XUnit (which, I have to admit, I failed to do until now, as Godot derived objects cannot be instantiated without the Godot context - or am I mistaken and did something wrong?).

But, this is not the reason I'm writing this issue: after some unsuccessful tries, I finally got the addon to work.

As, in the README, you asked for feedback on the install process, here are the problems I had to solve while installing:

Details on how to do some things

You sometimes say things like add these files in your project file. That can be pretty straightforward for people very used to manipulate projects, but, if they're like me and don't do that very often, an example is always welcome.
For this particular example when you ask to add the addon files, with the new format of .csproj files coming with Godot 3.2.3, all cs files are included by default in the project. So, instead of adding them, we have to remove the others:

  <ItemGroup>
    <Compile Remove="addons\GodotXUnit\GodotXUnitApi\**" />
  </ItemGroup>

Note that if you try to remove the whole addons\GodotXUnit\** directory and whitelist the 3 files, it won't work and make Godot incapable of compiling the solution (I believe, because it is a key conflict) it resolves each file in the directory, add it to a dictionary, and when trying to whitelist, tries to add the key a second time which creates the error).

Same thing for adding the reference to GodotXUnit in the main project, an example is always a good thing:

  <ItemGroup>
    <ProjectReference Include="addons\GodotXUnit\GodotXUnitApi\GodotXUnitApi.csproj" />
  </ItemGroup>

AssetLib

When downloading from the AssetLib in Godot, it asks to download a bit too much of files, you could just limit it to the addon directory in my opinion, as the other files are not necessary. The licence is already in the addons/GodotXUnit directory, and the github workflow files are very specific to each project and not everybody uses github, so it should be removed.

Simpler install procedure

You're providing the whole project in the addon directory. Maybe you should consider providing the compiled dll (in the directory or as a nuget package). As it is a dependency, we won't modify it, so providing the source directly is not necessary (and would have prevented some puzzling errors I got at some point).

Different project for tests

By now, you should have understood that I'm a bit of a maniac and love when stuff are simple and tidied up. So I ended up creating a new project for my tests. I wrote a test and compiled.
As expected, it showed up in the dropdown list, I selected it, ran it, and nothing happened. In fact something did happen, but not what in expected, I ended up with an exception preventing me to run the test.

error while attempting to get test assembly

System.IO.FileNotFoundException: 
  at (wrapper managed-to-native) System.Reflection.Assembly.InternalGetAssemblyName(string,Mono.MonoAssemblyName&,string&)
  at System.Reflection.AssemblyName.GetAssemblyName (System.String assemblyFile) [0x0001a] in <46e2faba55964e57bc4a72159b9574b8>:0 
  at GodotXUnit.GodotTestRunner.GetTargetAssembly (GodotXUnitApi.GodotXUnitSummary summary) [0x00140] in /Users/evaneos/projects/games/test-xunit/addons/GodotXUnit/GodotTestRunner.cs:47 
  at GodotXUnitApi.Internal.GodotXUnitRunner.CreateRunner () [0x00002] in /Users/evaneos/projects/games/test-xunit/addons/GodotXUnit/GodotXUnitApi/Internal/GodotXUnitRunner.cs:131 

I haven't resolved that one yet, but I believe this happened because my project file doesn't define explicitly its targets, which is not mandatory. It might need a default value to return, or removing the project of the dropdown list if it lacks information to run.

Compiling

That was the frustrating part once I got it running: I changed my tests and tried to ran it and... it still ran the old one. i
It didn't compile before launching the tests. I believe you did that on purpose and I get that waiting for the project to compile is frustrating, but I don't see any case where you don't need to compile before running the tests. Could you consider adding this step before any test run (and maybe an option to disable it of you really don't want it), because it really is error prone, and one click is always a better UX than two.

Thank you for reading me, and tell me if you need a hand to do some of the stuff I brought up.

[remi-san]

I've been able to dig a little deeper on the test assembly matter:

I believe this happened because my project file doesn't define explicitly its targets, which is not mandatory. It might need a default value to return, or removing the project of the dropdown list if it lacks information to run.

As I've read the code since earlier, I now know my assumptions were false, as your code only looks in bin\Debug for the dll (cf GetDllFromCsProjectPath()).

By default VisualStudio adds the target framework, meaning my dll is in bin\Debug\net472.

I guess it should read from the .csproj file the debug values for <TargetFramework>net472</TargetFramework>, <OutputPath>bin\Debug</OutputPath>, and <AppendTargetFrameworkToOutputPath>true</AppendTargetFrameworkToOutputPath>, so that, if it's configured, we're sure to get the appropriate value, or to fallback to one of the 2 (by testing the file presence) if it isn't.

Or using a checkbox to ask the user if there's the framework version in the path, in which case, only the <TargetFramework>net472</TargetFramework> would have to be read.

Let me know what you think.

[remi-san]

To compile, I've tried a few things in StartTests, between WorkFiles.CleanWorkDir(); and var runArgs = new StringList();.

But I haven't been able to find a satisfying solution.

For an unknown reason, if I make a call to msbuild directly, it doesn't find the binary and fails:

            GodotArray output = new GodotArray();
            int returnCode = OS.Execute("msbuild",new string[0], true, output, true);
            if (returnCode != 0)
            {
                GD.PrintErr("Build failed");
                GD.Print(output.ToString());
            }

Prints:

 Build failed
[sh: msbuild: command not found]

But it wouldn't have informed the user on the build result like the mono tab does (no progress window, no green check icon, etc.)

I've tried to find a way to call what the Build Project button calls, but couldn't find a way to do so.

So I got a bit creative and ended up with this:

            GodotArray output = new GodotArray();
            int returnCode = OS.Execute(OS.GetExecutablePath(), new string[2] { "--build-solutions", "--quit" }, true, output, true);
            if (returnCode != 0)
            {
                GD.PrintErr("Build failed");
                GD.Print(output.ToString());
            }

Which does give me the feedback I wanted, but in a new Godot window which disappears once the build is done, and we sadly need to wait for the Godot editor to boot up...

[GeorgeS2019]

Thanks for the feedback, it helps!

[Calinou]

When downloading from the AssetLib in Godot, it asks to download a bit too much of files, you could just limit it to the addon directory in my opinion, as the other files are not necessary. The licence is already in the addons/GodotXUnit directory, and the github workflow files are very specific to each project and not everybody uses github, so it should be removed.

See https://github.com/Calinou/godot-addon-template for an example of excluding unneeded files using .gitattributes.

You're providing the whole project in the addon directory. Maybe you should consider providing the compiled dll (in the directory or as a nuget package). As it is a dependency, we won't modify it, so providing the source directly is not necessary (and would have prevented some puzzling errors I got at some point).

Unfortunately, there is no easy way to do that yet. You could commit compiled libraries to the repository, but this will bloat your repository fairly quickly. This is especially true if you include debug symbols as well.

A more clean approach is to create build libraries and publish them to GitHub Releases when a new version is tagged. However, this requires using the Custom download provider on the asset library (which currently requires moderator privileges to use).

[rexfleischer]

Thanks for the feedback all! It's exciting to see.

To add to the first comment about the changes to Godot 3.2.3, it completely broke the multi project builds for me and I wasn't able to get it working in separate projects. If there is someone that knows how to get a multi project build working in godot now, I'd appreciate pointers or a PR example. There is a technical reason with xunit for why it should be a separate project. Though, I like really like the dll idea. Although, that has issues too, but it could probably work with adding it as a dep in the root project.

The multi project issue is with xunit attributes: https://github.com/fledware/GodotXUnit/blob/master/addons/GodotXUnit/GodotXUnitApi/GodotFactAttribute.cs#L48

I have a working example of a single project build working for godot 3.2.3 on the hack-example branch: https://github.com/fledware/GodotXUnit/tree/hack-example

But it requires developers to change the annotation value: https://github.com/fledware/GodotXUnit/blob/hack-example/addons/GodotXUnit/GodotXUnitApi/GodotFactAttribute.cs#L50

But, iirc, the main issue is with the multi project setup in godot. If someone knows how to setup mutli project .net in godot, please let me know and I'll be happy to give it an attempt. I absolutely love godot, and would love to start working with godot again.