Should --Explore Error On "Invalid" Assemblies?

[aolszowka]

Reproduce With The Following:

echo.>bad.dll
nunit3-console.exe -explore bad.dll

Yields:

NUnit.Engine.NUnitEngineException : bad.dll could not be examined
  ----> System.BadImageFormatException : Format of the executable (.exe) or library (.dll) is invalid.

--NUnitEngineException
bad.dll could not be examined
   at NUnit.Engine.Internal.TargetFrameworkHelper..ctor(String assemblyPath)
   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.Explore(TestFilter filter)
   at NUnit.ConsoleRunner.ConsoleRunner.ExploreTests(TestPackage package, TestFilter filter)
   at NUnit.ConsoleRunner.Program.Main(String[] args)
--
BadImageFormatException
Format of the executable (.exe) or library (.dll) is invalid.
   at Mono.Cecil.PE.ImageReader.ReadImage()
   at Mono.Cecil.PE.ImageReader.ReadImageFrom(Stream stream)
   at Mono.Cecil.ModuleDefinition.ReadModule(Stream stream, ReaderParameters parameters)
   at Mono.Cecil.ModuleDefinition.ReadModule(String fileName, ReaderParameters parameters)
   at NUnit.Engine.Internal.TargetFrameworkHelper..ctor(String assemblyPath)

Obviously because this is a completely invalid DLL.

Our use case is a little more exotic than this, basically we have a common bin output location in which hundreds of DLL's end up, some managed, some unmanaged, and we are asking the question "Which of these have tests?".

The error in question comes from:

nunit-console/src/NUnitEngine/nunit.engine.core/Internal/TargetFrameworkHelper.cs

Line 44 in 0958996

throw new NUnitEngineException($"{assemblyPath} could not be examined", e);

We have similar code in our code base, however we simply ignore the load failure.

The question is more one of design, should this be the expected behavior? Or should --explore be "Best Effort"?

[ChrisMaddock]

Interesting question about what the behaviour of explore should be - especially when we have other open issues like #658 to make it more strict!

My initial thoughts - --explore should reflect the same behaviour as running. Currently, if you tried to run that assembly, you would get a crash, that it was an invalid assembly.

That's how I currently see explore, just to find the tests in the explicit list of assemblies you're passing in. What you're trying to do seems to me to be a bit of a different use-case, more of a 'discover'?

I would say there's another argument as to whether crashing in this way is the correct behaviour, of if we should instead suppress the exception here, and create a InvalidAssemblyFrameworkDriver. That would give a more proper error when trying to run the tests - I don't know what the behaviour would be for explore, however.

@nunit/engine-team - anyone any thoughts?

[ChrisMaddock]

@aolszowka - what's you're actual usecase, sorry - are you intending to use the console, or are you planning to use the engine itself?

[ChrisMaddock]

Thinking more about this, this particular crash I'm pretty sure should definitely be reworked to use InvalidAssemblyFrameworkDriver. (The current behaviour pre-dates the addition of InvalidAssemblyFrameworkDriver, IIRC.)

That fixes the engine - the question then is how the Console should behave. Given I don't know how the console behaves in this situation currently however, that's a difficult one to reason about!

The crash is triggered by this, which shouldn't really be here anyway. 😬 We'll need to have a think about how we're going to mark up sub-packages as invalid, I think.

nunit-console/src/NUnitEngine/nunit.engine/Runners/MasterTestRunner.cs

Lines 241 to 247 in 0d037ca

	// Use SelectRuntimeFramework for its side effects.
	// Info will be left behind in the package about
	// each contained assembly, which will subsequently
	// be used to determine how to run the assembly.
	#if NETFRAMEWORK
	_runtimeService.SelectRuntimeFramework(TestPackage);
	#endif

[CharliePoole]

At a certain level, this is understandable. That is, if you tell the console to explore something that is not a test assembly, or possibly not even an assembly, then an error should be expected. It's always been a basic (maybe unstated) principle of NUnit that it tries to do exactly what you tell it to do rather than assuming you meant something else.

Of course, if the OP's error means that the program crashes, that's a bug. We should error by giving an error message and then exiting with a recognizable return code.

The case where you have a bunch of dlls in a directory and don't actually know which of them have tests is another matter. Neither the console runner nor the engine has a feature to do that, so the user is forced to either pass in all the assemblies or to determine outside of NUnit which ones are test assemblies. There is an option to ignore non-test assemblies, so maybe there should be an option to ignore anything that can't be loaded.

If we wanted to take it a step further, we could have a feature that allows exploring an entire set of assemblies using a pattern.

[aolszowka]

Thank you all for the responses let me start by responding oldest to newest:

@ChrisMaddock

what's you're actual usecase, sorry - are you intending to use the console, or are you planning to use the engine itself?

Basically we have a common bin output location in which hundreds of DLL's end up, some managed, some unmanaged, and we are asking the question "Which of these have tests?".

This list is then sent to NUnit Console to execute.

Right now today we have an in house MSBuild task that does this, the jist of which is:

        /// <summary>
        /// Identify if the given assembly is a unit test.
        /// </summary>
        /// <returns>
        /// <c>true</c> if the given assembly is a unit test; otherwise, <c>false</c>.
        /// </returns>
        /// <remarks>
        /// Returns <c>true</c> only if the assembly contains a reference to
        /// NUnit.Framework and does not contain InteractiveTest in its
        /// Assembly Name (NOT the filename!).
        /// </remarks>
        private bool _IsAUnitTestAssembly(string targetAssembly)
        {
            bool isATestAssembly = false;

            // We need to load each assembly into its own AppDomain this
            // allows us to unload the assembly once we're done. Also this
            // avoids any potential issues with assemblies that may have
            // already been loaded into our AppDomain
            AppDomainSetup ads = new AppDomainSetup();
            ads.ApplicationBase = Environment.CurrentDirectory;

            AppDomain ad = AppDomain.CreateDomain("currentTargetAssembly" + targetAssembly, null, ads);

            // This is a Remote Object!
            DependencyScannerRemote dsi =
                (DependencyScannerRemote)ad.CreateInstanceAndUnwrap(Assembly.GetExecutingAssembly().FullName,
                                                                    typeof(DependencyScannerRemote).FullName);

            if (dsi.IsManagedAssembly(targetAssembly))
            {
                string assemblyName = dsi.GetAssemblyName(targetAssembly);
                if (assemblyName.Contains(INTERACTIVE_TEST_IDENTIFIER))
                {
                    Log.LogMessage(MessageImportance.Normal, Resources.GetUnitTestAssembliesSkippingInteractiveTest, assemblyName);
                }
                else
                {
                    IEnumerable<string> assemblyReferences = dsi.GetReferencedAssemblies(targetAssembly);
                    isATestAssembly = assemblyReferences.Any(current => current.ToLower().StartsWith(NUNIT_FRAMEWORK));
                }
            }
            else
            {
                if (this.OnlyWarnOnFailureToLoadAssembly)
                {
                    Log.LogWarning(Resources.GetUnitTestAssembliesNotManagedAssembly, targetAssembly);
                }
                else
                {
                    Log.LogError(Resources.GetUnitTestAssembliesNotManagedAssembly, targetAssembly);
                }
            }

            // Unload this AppDomain
            AppDomain.Unload(ad);

            return isATestAssembly;
        }

As you can see it does a bit more than just see if the Assembly contains a reference to NUnit.Framework but the hope was to have something out of the box and replace our custom logic.

@CharliePoole

At a certain level, this is understandable. That is, if you tell the console to explore something that is not a test assembly, or possibly not even an assembly, then an error should be expected. It's always been a basic (maybe unstated) principle of NUnit that it tries to do exactly what you tell it to do rather than assuming you meant something else.

The case where you have a bunch of dlls in a directory and don't actually know which of them have tests is another matter. Neither the console runner nor the engine has a feature to do that, so the user is forced to either pass in all the assemblies or to determine outside of NUnit which ones are test assemblies. There is an option to ignore non-test assemblies, so maybe there should be an option to ignore anything that can't be loaded.

I think this is the answer to the question. Totally fair and I realize its a design choice :) just wanted to make that clear.

I feel OK closing this issue without further discussion. Thank you again for taking the time to answer and thank you for NUnit!

[CharliePoole]

Here's what got me thinking about a feature...

If you run *. dll on some (most? many?) Linux shells, the runner gets passed a list of assemblies, doesn't it?

If that's the case, perhaps we should deal with it, recognizing that the user may not have literally typed in the invalid file name.

[aolszowka]

if you run *. dll on some (most? many?) Linux shells, the runner gets passed a list of assemblies, doesn't it?

FWIW I generated the "Bad" listing by doing something similar in Windows:

echo --explore> Assemblies.txt
dir /b /s *.dll >> Assemblies.txt
nunit3-console.exe @Assemblies.txt

Not as slick as *.dll for sure but in Windows I am comfortable enough composing commands like the above.

Totally aware that I was subverting our in-house tooling to filter to just NUnit Test Assemblies, I was just looking for a quick one off rather than opening up our cumbersome tooling (and this is coming from the guy who wrote that tooling!)

[CharliePoole]

@aolszowka The key difference, as I see it, is that the naive user may assume that NUnit runner itself is supporting *.dll when, in fact, the file substitution takes place without the runner's knowledge.

Now you could run nunit3-console *.dll --explore --skipnontestassemblies to eliminate the errors that arise when a valid assembly has no tests. But that still leaves a few cases where an exception could be thrown... i.e. any *.dll matches, which cannot be loaded at all.

[ChrisMaddock]

Ok, let's draw some conclusions out of this.

I think we all agree the engine shouldn't crash! I've created #798 to cover that.

I think we also agree that this isn't really what --explore was intended for, however given it doesn't currently error on invalid assemblies or fixtures, I don't particularly see why this couldn't work.

@aolszowka - there's one bit missing in my mental model of your process - how are you planning to get from the output of --explore, which is test names, to a list of assemblies. Or are you instead planning to run --explore to get the xml output, and retrieving the assembly names from that? The latter sounds like a thing which could actually work fine.

As I say, I don't think this was ever intended as functionality, but if we accept that the crash shouldn't happen, then I can't think why it should be possible. @CharliePoole - can you think of anything I'm missing?

[CharliePoole]

@ChrisMaddock I don't think you're missing anything. :-)

An important distinction between @aolszowka 's experiment using a response file is that the runner doesn't know how that file was generated. In the second execution of nunit3-console, the runner simply receives a list of files just as if the user had typed them on the command-line, so it can't know that there may be some invalid files there to be ignored.

If we had an option like --skipinvalidfiles or --skipnonloadablefiles, then that option could either be included in the response file or added to the command-line alongside it. Key thing would be to be very precise about which kinds of problems we might want to ignore. We already support skipping non-test assemblies so the other things I can imagine silently skipping are non-assemblies and assemblies that target a framework we don't support.

[aolszowka]

@ChrisMaddock I think @CharliePoole hit it right on the head; I intend to pipe the output of --explore more or less into a response file to perform execution on.

The original use case was I was just trying to get a listing of all NUnit Tests that had a category of "Smoke". We are reevaluating some of these "Smoke" tests for dubious/heavy tests as part of a rewrite of our CI. I would have used --explore to get the filtered tests and then piped to another post processing program (to filter it down even more) and then piped it right back into nunit3-console to get the output.

Maybe think of it as a second layer of filtering that has our business specific logic applied to it?

I am about to head offline for about a week and a half and I apologize for not getting back to you immediately, but I promise to do so when I get back. Thank you for all your work on NUnit and Open Source!

[ChrisMaddock]

Ahh ok!

So hang on - --explore gives you a list of tests, rather than a list of assemblies, right? So you would need to pipe the output of explore into a file, and then pass it back in with the --testList parameter? And your Assemblies.txt above would actually be TestNames.txt. (I think what I'm saying is correct - haven't used those features myself in a while!)

That bits important, as trying to execute an assembly with no tests counts as invalid, and gives a non-zero exit code in the NUnit Console. (Regardless of whether you use --skipNonTestAssemblies or not - something I would like to review for the next major version of NUnit)

To make this work, I think you would need some sort of way to turn your list of tests into a list of valid assemblies. I don't know if you test names are standardised enough to be able to script that?

A second, more hair-brained idea might be to implement an IDriverFactory extension, which could (I think) override the default engine behaviour above, and ultimately output a zero exit code for non-test assemblies. I'd need to look at the logic of how those are loaded to work out if that's possible - and I'm not convinced trying to override the engine's "intent" in that way is actually a good idea! It's an interesting idea to think about though...

I am about to head offline for about a week and a half and I apologize for not getting back to you immediately, but I promise to do so when I get back. Thank you for all your work on NUnit and Open Source!

No problem - hope you have a good break! 👋

[ChrisMaddock]

Oh, I'm really overthinking this. I forgot that --explore can take an output spec as a parameter.

I'd recommend writing a simple Result Writer Extension, which you can use with --explore to output a list of valid assembly names. That's much cleaner than the rubbish I was sprouting in my last post, and actually quite a nice use of some more niche functionality! 😄

TestCaseResultWriter is a good example of the sort of thing you’d need to do: https://github.com/nunit/nunit-console/blob/master/src/NUnitEngine/nunit.engine/Services/ResultWriters/TestCaseResultWriter.cs

[CharliePoole]

Yeah, I think we have jumped back and forth between explore creating test input and the user creating a list of assemblies. My thinking on the latter was that the list, generated simply enough on the command-line would go in a response file. The hypothetical --skipxxxx option would tell NUnit "Don't be surprised if some of these files can't be loaded."

I think that's the problem we started out with here.

[aolszowka]

Hey back, and yeah I think @CharliePoole is on the right track with what I am thinking.

Realistically though it's a more "nice to have" rather than a serious gap of any sort.

A good rule of thumb might be the willingness to provide a patch that has the changes, at this time I probably wouldn't chime up too loud, but would throw it on a "nice to have" list.

I am not sure where these types of issues go, and I am totally OK with closing this with a non-actionable status.

[ChrisMaddock]

Thanks for starting the conversation @aolszowka! #798 now covers the crash, and I've just created #827 to cover the potential change in behaviour, which I personally think would be a good one! 🙂