nunit / nunit-console

NUnit Console runner and test engine
MIT License
215 stars 151 forks source link

Unable to run nunit-console with VSProjectLoader that has a WPF 8.0 project #1468

Closed WindingWinter closed 6 days ago

WindingWinter commented 1 week ago

If you just add a WPF Windows .net 8.0 project to a solution, such as this one:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>WinExe</OutputType>
    <TargetFramework>net8.0-windows</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <UseWPF>true</UseWPF>
  </PropertyGroup>

</Project>

Then when you run the code

"%NunitConsole%\nunit3-console.exe" "%slnFile%"

Where %slnFile% is the path of your solution file, then it just won't work.

This is the error message:

NUnit Console 3.18.1+5e16ca2ef85f57f9f09bb41ff08feb8f3168a384 (Release)
Copyright (c) 2022 Charlie Poole, Rob Prouse
Tuesday, September 3, 2024 10:25:58 AM

Runtime Environment
   OS Version: Microsoft Windows NT 6.2.9200.0
   Runtime: .NET Framework CLR v4.0.30319.42000

Test Files: XXX.sln

System.BadImageFormatException : Format of the executable (.exe) or library (.dll) is invalid.

--BadImageFormatException
Format of the executable (.exe) or library (.dll) is invalid.
   at TestCentric.Metadata.PE.ImageReader.ReadOptionalHeaders(UInt16& subsystem, UInt16& dll_characteristics)
   at TestCentric.Metadata.PE.ImageReader.ReadImage()
   at TestCentric.Metadata.PE.ImageReader.ReadImage(Disposable`1 stream, String file_name)
   at TestCentric.Metadata.ModuleDefinition.ReadModule(String fileName, ReaderParameters parameters)
   at NUnit.Engine.Services.RuntimeFrameworkService.ApplyImageData(TestPackage package)
   at NUnit.Engine.Services.RuntimeFrameworkService.ApplyImageData(TestPackage package)
   at NUnit.Engine.Services.RuntimeFrameworkService.ApplyImageData(TestPackage package)
   at NUnit.Engine.Services.RuntimeFrameworkService.SelectRuntimeFramework(TestPackage package)
   at NUnit.Engine.Runners.MasterTestRunner.GetEngineRunner()
   at NUnit.Engine.Runners.MasterTestRunner.RunTests(ITestEventListener listener, TestFilter filter)
   at NUnit.Engine.Runners.MasterTestRunner.Run(ITestEventListener listener, TestFilter filter)
   at NUnit.ConsoleRunner.ConsoleRunner.RunTests(TestPackage package, TestFilter filter)
   at NUnit.ConsoleRunner.Program.Main(String[] args)

I've tested this with nunit console 3.18.1 standard version, and the VSProjectLoader 3.9 version.

It should be noted that the existence of WPF .net framework 4.8 version won't have any problem at all.

CharliePoole commented 1 week ago

@WindingWinter Coincidentally, I just ran into the same error in the TestCentric GUI. As you can see, the exception is thrown by TestCentric.Metadata, a package I maintain. It is used both by my GUI and by the NUnit engine.

The code of TestCentric.Metadata is rather old and it's likely that the internal structure of a managed assembly image has had some additions, which are causing this exception. I don't think it's a problem which will be resolved quickly but I'll start looking into it.

@OsirisTerje FYI... this may have big implications for the adapter.

OsirisTerje commented 1 week ago

@WindingWinter Does this crash with dotnet test (meaning you have the adapter as part of the chain) ?

WindingWinter commented 1 week ago

@OsirisTerje , not entirely sure what you mean, but if I run it with dotnet test %sln% then it won't crash with the System.BadImageFormatException as per above.

And the WinExe project file doesn't even have to be a unit test project in the sense that it's not referring to Nunit or test adapter.

OsirisTerje commented 1 week ago

That means it is not related to the engine per se, as the adapter 4.6 also use the 3.18.1, but to what executables it is trying to load.

It seems the console is trying to load that executable without needing to do so, probably to check if it is a test assembly or not. The filtering of assemblies happens in the Microsoft testhost (or perhaps even before that one too).

CharliePoole commented 1 week ago

@WindingWinter Coincidentally, I just ran into the same error in the TestCentric GUI. As you can see, the exception is thrown by TestCentric.Metadata, a package I maintain. It is used both by my GUI and by the NUnit engine.

The code of TestCentric.Metadata is rather old and it's likely that the internal structure of a managed assembly image has had some additions, which are causing this exception. I don't think it's a problem which will be resolved quickly but I'll start looking into it.

@OsirisTerje FYI... this may have big implications for the adapter.

CharliePoole commented 1 week ago

@OsirisTerje @WindingWinter

Above comment was written before I saw @OsirisTerje 's comment. So... more info...

I think what we have here is a serious bug in the TestCentric.Metadata package. As I say, it's not surprising due to the age of the code. Here is some background about that package...

BACKGROUND

A long time ago we used reflection exclusively but reflection did not help us in our cross-platform scenario, i.e. we were building the runner and engine with .NET 2.0 but wanting to examine newer framework versions and the newly arrived .NET Core and .Net Standard. To solve that problem, we switched to Mono.Cecil.

Mono.Cecil worked for a while but we ran into issues when testing apps that used Mono.Cecil themselves in a different version from ours. Also, Mono.Cecil eventually dropped some older platforms we were still using.

TestCentric.Metadata, which I initially did for the TC GUI, took a version of Mono.Cecil that supported .NET 2.0 and higher and dropped all the assembly-writing features. Eventually, we replaced NUnit's use of Mono.Cecil with my package.

THIS ISSUE

I think this issue would be readily solved for @WindingWinter if we didn't try to load the executable at all. That would involve looking at the package produced by the VS Project Loader extension to see if it includes the .NET Standard assembly. If so, that's a bug in the extension. If so, I'd view that as relatively low priority because there is already a simpler workaround, in this case, of simply not using the solution at all but either using the project or the single assembly itself on the command-line. We should also consider issue #25, which involves a much bigger rewrite of the project loader.

BROADER PROBLEM

I'm seeing the same stack trace in the TC GUI when loading a .NET 8.0 test assembly. That's where I see a potentially bigger implication. I'll try to replicate that problem for this issue and I'll create an issue for the metadata project itself.

It's not clear whether this impacts the VS adapter. For that, I'll need to narrow down the particular APIs that are failing, determine where they are used in the engine and whether the adapter is likely to trigger those calls. Since the adapter only uses a subset of the engine API, there may be no immediate effect. That would take a bit of the pressure off this problem but it will still need to be solved eventually. Meanwhile, we should keep an eye out for any exceptions that occur in the metadata project and link them here.

CharliePoole commented 1 week ago

@WindingWinter If you switch to using either the .csproj file or the assembly itself on the command-line, does that work?

WindingWinter commented 1 week ago

@CharliePoole , if I run it on DLL basis then no, no such problem. It makes sense though because when I run it on DLL basis then the nunit console will have no information of the WPF 8.0 version, so no screwing up.

CharliePoole commented 1 week ago

Not sure what you mean by that. The console doesn't get any information from the solution or project files about the WPF version you use. Neither does the project loader extension. All information is extracted from the dll itself by the engine, including all your references.

However, running from the dll vs csproj vs sln can change how the engine examines the assembly, just what calls it makes and in what order, so this information is a useful lead for me. For now, I suggest you just run the assembly on the command-line and watch this issue for a fix.

CharliePoole commented 1 week ago

Oh... are you saying that the WPF project is not a test project? That would be important info!

CharliePoole commented 1 week ago

If the WPF project is not a test project (waiting for confirmation from @WindingWinter ), then this would really be a duplicate of #1465. The question there is "why are we examining this assembly at all?" and the fact that it's .net 8.0 or .net standard isn't relevant.

However the stack trace in this error indicates a failure in examining the metadata for the .net 8.0 app, which I have seen before in the TestCentric GUI, so I'll retain this issue either way.

@WindingWinter You could help by running NUnit3-console against the .net 8.0 exe alone. I'm pretty sure we will see the same stack trace.

WindingWinter commented 1 week ago

@CharliePoole , yes you are right. The WPF project is not a test project at all. As indicated totally by the csproject file information:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>WinExe</OutputType>
    <TargetFramework>net8.0-windows</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <UseWPF>true</UseWPF>
  </PropertyGroup>

</Project>
CharliePoole commented 1 week ago

@WindingWinter Yes, that's how I guessed. it would be unusual (but possible) to have a WinExe as a test project. :-)

Even so, it's still a bug if the engine throws an exception while analyzing any assembly so I'll continue to work on that problem here, while working the "spurious error" problem under issue #1465.

Further question to narrow down the problem: Is the WPF project is a self-contained executable?

WindingWinter commented 1 week ago

Yes it is a self contained thing with no reference to anyone. It's just a dummy winexe project and it can already reproduce the problem

On Thu, Sep 5, 2024, 6:26 PM CharliePoole @.***> wrote:

@WindingWinter https://github.com/WindingWinter Yes, that's how I guessed. it would be unusual (but possible) to have a WinExe as a test project. :-)

Even so, it's still a bug if the engine throws an exception while analyzing any assembly so I'll continue to work on that problem here, while working the "spurious error" problem under issue #1465 https://github.com/nunit/nunit-console/issues/1465.

Further question to narrow down the problem: Is the WPF project is a self-contained executable?

— Reply to this email directly, view it on GitHub https://github.com/nunit/nunit-console/issues/1468#issuecomment-2331167222, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADENIZVPUOPI6XTKHQ5N2ULZVAWWNAVCNFSM6AAAAABNRFPYN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZRGE3DOMRSGI . You are receiving this because you were mentioned.Message ID: @.***>

CharliePoole commented 1 week ago

I thought so. I think the exception may occur because of a missing optional header, but one which we assume must be present in a test assembly.

CharliePoole commented 1 week ago

I have replicated this in my latest set of tests.

CharliePoole commented 1 week ago

@OsirisTerje @WindingWinter

It turns out that the answer is relatively simple and my worries about the long-term viability of TestCentric.Metadata were a bit premature. I forgot one thing and didn't know another...

  1. What I forgot is that TestCentric.Metadata like Mono.Cecil can only process managed assemblies.
  2. What I didn't know is that a WPF WinExe app, and I believe any stand-alone exe, is unmanaged.

@WindingWinter You can check this yourself. If your app is named MyWPFApp, you can use ildasm to open MyWPFApp.dll without a problem. But if you try to open MyWPFApp.exe, you'll get an error message.

I can resolve this in one of two ways...

  1. Catch the exception and handle it in the console runner.
  2. Modify TestCentric.Metadata to actually have an API that tells you if an assembly is managed or unmanaged.

In fact, I'll do both things, option 1 right now for the console runner and an internal fix in the next release of TestCentric Metadata.

@jbevain I'd appreciate any thoughts you have on the above.

CharliePoole commented 6 days ago

@WindingWinter Fix is on myget, version 3.18.2-dev00023 if you would like to test it.

@OsirisTerje I created a "test runner" for unmanaged executables. It doesn't actually execute anything, of course, but it returns a result when you call it rather than throwing an exception. This is an adhoc experimental implementation but I'll come back to it in a separate issue to create similar (non)runners for other situations. More detail on that issue when I create it.