Nunit-console: DirectoryFinder.GetDirectories throws for a path with the drive specified

[ChrisMaddock]

DirectoryFinder.GetDirectories() is a method used when locating engine extensions. (See locating addins in the docs, for background) It searches file paths for wildcard characters, and returns all possible directories that could match the wildcard.

It currently throws when passed a path containing a drive, e.g. C:\Users\Chris\mock-event-listener.dll. It should be able to handle such paths in the normal way.

Tagging this as a nice, approachable issue for a first time contributor. If I was tackling this, my first step would be to create a unit test which reproduces the issue, and look at why the exception is thrown, and how this could be prevented.

Below is the stack trace currently thrown:

System.ArgumentException
  HResult=0x80070057
  Message=Second path fragment must not be a drive or UNC name.
Parameter name: path2
  Source=mscorlib
  StackTrace:
   at System.IO.Path.InternalCombine(String path1, String path2)
   at System.IO.FileSystemEnumerableIterator`1.GetFullSearchString(String fullPath, String searchPattern)
   at System.IO.FileSystemEnumerableIterator`1..ctor(String path, String originalUserPath, String searchPattern, SearchOption searchOption, SearchResultHandler`1 resultHandler, Boolean checkHost)
   at System.IO.DirectoryInfo.InternalGetDirectories(String searchPattern, SearchOption searchOption)
   at NUnit.Engine.Internal.DirectoryFinder.ExpandOneStep(IList`1 dirList, String pattern) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Internal\DirectoryFinder.cs:line 147
   at NUnit.Engine.Internal.DirectoryFinder.GetDirectories(DirectoryInfo baseDir, String pattern) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Internal\DirectoryFinder.cs:line 76
   at NUnit.Engine.Internal.DirectoryFinder.GetFiles(DirectoryInfo baseDir, String pattern) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Internal\DirectoryFinder.cs:line 102
   at NUnit.Engine.Services.ExtensionService.ProcessAddinsFile(DirectoryInfo baseDir, String fileName, Boolean fromWildCard) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Services\ExtensionService.cs:line 351
   at NUnit.Engine.Services.ExtensionService.ProcessAddinsFiles(DirectoryInfo startDir, Boolean fromWildCard) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Services\ExtensionService.cs:line 315
   at NUnit.Engine.Services.ExtensionService.FindExtensionAssemblies(DirectoryInfo startDir) in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Services\ExtensionService.cs:line 289
   at NUnit.Engine.Services.ExtensionService.StartService() in C:\Users\Chris\Documents\git\nunit-console\src\NUnitEngine\nunit.engine.core\Services\ExtensionService.cs:line 185

[x789]

I will try to solve the issue. @ChrisMaddock, can you assign it to me?

[mikkelbu]

I can do that :). It has been assigned to you @x789.

[ChrisMaddock]

Great - thanks both! 😊

[x789]

Well, getting my feet wet with NUnit development is more tedious that I have thought...

My first idea to start with was to debug the unit-tests (like @ChrisMaddock suggested). But I am not able to create a debuggable build of nunit.engine.api and nunit.engine.core. No matter whether I build the 'debug' configuration using VS2019 or the build-script, each time I attach a debugger it tells me that I try to step into an optimized build. It even did not help to remove optimization for all configurations from the csproj-files.
I tried to get help on that from the NUnit-developer group at Google. But my message was either lost or is still stuck somewhere in a review process. (But if somebody reads this who can help me to debug, please do so!)

Apart from my technical problems I took a closer look at the DirectoryFinder and discovered that most of its unit-tests are deactivated because they rely on a specific file-system layout.

I think it would be a good thing if DirectoryFinder is covered by unit-tests in the future. These tests will be brittle if there is no possibility to inject a (faked) file-system into DirectoryFinder. But methods of DirectoryFinder have the sealed types DirectoryInfo and FileInfo in their signature that are not fakeable. That means if we change to a kind of virtual (fakeable) file-system, this interface will be broken and all clients will be affected. As of today, I think that ExtensionService is the only class that needs to be adapted.
But I am not sure if there is a dependency out there that I am not aware of, since DirectoryFinder is a public class. (Even though the rules say one should reference nunit.engine.api instead of nunit.engine.core there might be somebody who ignored that.)

This issue just requests a fix for DirectoryFinder and not the severe changes I suggested. That's why I would like to discuss it with you. Shall I invest more time for a more detailed plan or shall I drop it, because the solution I outlined above would be rejected anyway?

[CharliePoole]

Regarding debugging... it depends how you are running the tests. In order to debug the actual engine or console code, you have to be running the console that you just built. If you are trying to run tests in some other way you are probably executing a released package of some version of the engine. So... how are you running tests?

I went ahead and approved your post to the google group but we may as well discuss here anyway.

[ChrisMaddock]

Sorry you've not had the best experience with running the tests. I don't use the adapter myself, so I'm not totally clear on the issues with it, but I expect what Charlie's posted on the Google group is the issue - that your tests will be running against the VS adapter's own version of the engine, rather than the version we're trying to test.

What I normally do is either:
1) Set a suitable command line for the nunit3-console project, and debug into the engine from there. (Remember to rebuild the whole solution before doing that - as the console project doesn't have a project-reference to the engine project, it won't build automatically)
2) Run the .NET Core tests as an NUnit Lite project. For that, if you have the .NET Core build selected in VS, you should be able to execute the nunit.engine.tests assembly itself, which will run all tests. You can pass in filters to target a particular test in the same way as the console, e.g. with --test=My.Test.Here

Apart from my technical problems I took a closer look at the DirectoryFinder and discovered that most of its unit-tests are deactivated because they rely on a specific file-system layout. [...] That's why I would like to discuss it with you. Shall I invest more time for a more detailed plan or shall I drop it, because the solution I outlined above would be rejected anyway?

What you've suggested sounds great! The DirectoryFinderTests are really one of our many bits of technical debt, and anything to help with that would be appreciated.

In terms of it being a public class, you're right to ask the question. We have a problem across the NUnit codebases that more code is exposed as public than really should be - which is something we're working on rectifying over time. In the Engine/Console repository - we're happy to consider anything in the nunit.engine or nunit.engine.core repositories to be code we can change, as long as it can be done without changing the interfaces in nunit.engine.api. As you say, people may have ignored the "rules" - but unfortunately we have to draw the line somewhere, or we'd never be able to make any progress. That's one of the big reasons we want to make more stuff internal in future - so we can prevent people from accidentally breaking the rules!

Hope that helps - let us know if there's anything else we can help with. 🙂

[x789]

Thanks, @CharliePoole and @ChrisMaddock for pointing me into the right direction. I tried the second of Chris' suggested approaches:

Run the .NET Core tests as an NUnit Lite project. For that, if you have the .NET Core build selected in VS, you should be able to execute the nunit.engine.tests assembly itself, which will run all tests. You can pass in filters to target a particular test in the same way as the console, e.g. with --test=My.Test.Here

Having this problem solved, I can now focus on the original issue. 🙂

As soon as I have worked out a plan to abstract the filesystem accesses and have an idea what kind of breaking changes we might experience, I will post it here for discussion.

[ChrisMaddock]

Sounds good - let us know if we can help at all! 😊

[x789]

So here is my proposal on how to approach this issue. Most of it deals with the testability of DirectoryFinder.
Since it will lead to my first contribution to the project, it would be nice if you tell me if it will suit the overall architecture or conventions. I have no problem if the plan has to be adapted according to your feedback or is completely rejected. This is only a proposal for discussion. 😊

New File System Abstraction

• Can be used instead of System.IO.Directory, System.IO.DirectoryInfo and System.IO.FileInfo.
• When this work-item is completed, the abstraction is more a stub than a full featured abstraction. It only includes functionality that is necessary for DirectoryFinder. It needs to be extended if more entities of the engine shall use it (e.g. add functionality to open files which is required by ExtensionService).
• New interface IFileSystem
  
  
  
  • has the visibility internal
  
  
  • is located in namespace NUnit.Engine.Internal.FileSystemAccess
  
  
  • has a method bool Exists(IDirectory) to check if a directory exists
  
  
  • has a method bool Exists(IFile) to check if a file exists
  
  
  • has a method IDirectory GetDirectory(String path) to get an IDirectory
  
  
  • has a method IFile GetFile(String path) to get an IFile
  
  
• New interface IDirectory
  
  
  
  • has the visibility internal
  
  
  • located in namespace NUnit.Engine.Internal.FileSystemAccess
  
  
  • has a method IEnumerable<IDirectory> GetDirectories(searchPattern, SearchOption) that returns all subdirectories
  
  
  • has a method IEnumerable<IFile> GetFiles(pattern) that returns all files in this directory
  
  
  • has a read-only property Parent to get the directory's parent
  
  
  • has a read-only property FullName to get the directory's full name as String.
  
  
• New interface IFile
  
  
  
  • has the visibility internal
  
  
  • located in namespace NUnit.Engine.Internal.FileSystemAccess
  
  
  • has a read-only property Parent to get the directory that contains this file
  
  
  • has a read-only property FullName to get the file's full name as String
  
  
• New implementations FileSystem (=IFileSystem), Directory (=IDirectory) and File (=IFile).
  
  
  
  • have the visibility internal
  
  
  • are located in namespace NUnit.Engine.Internal.FileSystemAccess.Default
  
  
  • use classes from System.IO to do the actual work
  
  
  • will have unit-tests that require access to the file-system. Tests that require the file-system will be marked as _explicit_.
  
  
  • ⚠️ The names Directory and File collide with types from System.IO. Even though that is no problem for the compiler, it can lead to confusion. On the other hand, the new types are internal and thus cheap to change. But suggestion for better names are welcome.
  
  

Breaking API Changes

• Visibility of DirectoryFinder is changed to internal.
• DirectoryFinder.GetPackageDirectory is non-static and private
• DirectoryFinder.GetFiles is non-static
• DirectoryFinder.GetDirectories is non-static
• DirectoryFinder is non-static and sealed.

Recommended Breaking API Changes

• ExtensionService
  
  
  
  • is internal and sealed
  
  

Necessary Changes

• ExtensionService
  
  
  
  • each instance of ExtensionService has a private member that implements IFileSystem
  
  
  • internal constructor ExtensionService(bool isRunningOnAgent, IFileSystem fileSystem) is added. (This can be used to inject a faked file-system)
  
  
  • existing public constructor ExtensionService(bool isRunningOnAgent) is kept to avoid changes in clients of ExtensionService. This constructor is changed so it instantiates an implementation of IFileSystem and then calls the new constructor.
  
  
  • method ProcessAddinsFile gets adapted to use IDirectory and IFile. The way how it accesses the file-system is not changed. (This will be necessary to improve its testability, but I think that is out of scope of this work-item.)
  
  
• DirectoryFinder
  
  
  
  • use IFileSystem, IDirectory and IFile
  
  
  • getting the existing unit-tests in DirectoryFinderTests.cs to work
  
  
  • fix the original issue 😊
  
  

[ChrisMaddock]

Hi @x789 - this looks like a good plan. Great to say you've thought it out in so much detail before committing. This sounds like a really nice, generic abstraction which we could potentially also roll out to other file-loading functionality in future to help test that as well.

Some thoughts on specifics:

• Breaking/Recommended Breaking changes - all sound fine to me.
• I like your suggestion around the constructors, makes the production code remain as clean as possible.

⚠️ The names Directory and File collide with types from System.IO. Even though that is no problem for the compiler, it can lead to confusion.

This sounds fine to me. Personally I'd prioritise the simpler names over any potential confustion.

All sounds great! Let's get it written up in code and see what it looks like. 😄

[siprbaum]

Have you looked into https://github.com/System-IO-Abstractions/System.IO.Abstractions which - to my understanding provides exactly what you wanted here.
Of course, this would introducing a dependency to another library, which might or might not be an issue by itself

[CharliePoole]

I also like the level of detail in @x789 's plan. OTOH using an existing package is attractive because... existing!

I don't think the dependency is an issue _per se_. The problem we have seen with packages in the past is that they don't always support all our targets, or they support them initially but then eventually drop them. That's what happened with Mono.Cecil.

System.IO.Abstractions currently targets .NET Standard 2.0 and 2.1 and .NET Framework 4.6.1. If we used that package in the console, we would curently need to build an alternate private implementation for .NET 2.0. We could possibly do it with their source code rather than creating an entirely independent implementation. If we did that, we might be able to eventually dump the implementation depending on how long we want to support running under versions prior to 4.6.1.

OTOH, if @x789 creates an implementation for us, it will also have to support .NET 2.0, so the work needs to be done either way.

@x789 what's your take on this?

[ChrisMaddock]

My thoughts - I'd prefer to avoid dependencies for the engine wherever possible. Because some runners (namely the VS adapter) copy the engine into a project's bin file, adding any dependency to the engine has the potential to cause collisions with user code. When we had a dependency on Mono.Cecil, the adapter guys had to embed the dependency inside their own assembly (https://github.com/nunit/nunit3-vs-adapter/pull/541) - which I don't imagine was much fun!

If this was a bigger requirement I think we could consider it - but for this particular case I think @x789 has come up with a nice solution, and personally I think we'd be looking at pulling in a big dependency to solve a small problem. The engine has a particular set of issues here in my eyes - if this was a feature we were doing in the console, things could be very different!

[x789]

Thanks for the suggestion @siprbaum. That library looks very decent and I did not know it before.

But I fully agree with @ChrisMaddock, that the engine should have only dependencies that are crucial for its core functionality. And since we only need a small set of the features offered by System.IO.Abstractions (we do not need to move files or a FileSystemWatcher for instance), I think we should stick with a small file-system abstraction like I described above.

But beside that "keep dependency count low", there is another point why I propose to not useSystem.IO.Abstractions. As @CharliePoole already mentioned, that library is not compatible with .NET 2.0.
I took a brief look at its source-code. Its current implementation needs some changes to be compatible with .NET 2.0. Therefore we would need to fork that library and make some changes and then need to persuade the maintainers of System.IO.Abstractions why they should merge our stuff and keep our changes. And if they do not want to do that, we need to maintain that fork...
That would be way too much work for what we get. Therefore I would like to follow the original plan I proposed.

[x789]

Just a quick progress report. After some busy weeks I managed to get time to work on this work-item.
I completed the file-system abstraction and will now start to fix the original issue.

[ChrisMaddock]

Great - look forward to seeing what you've put together! 😄

[x789]

In the meantime I implemented the changes I described above. I also wrote a bunch of unit tests that cover the current behavior of the DirectoryFinder. But I haven't implemented the solution for the actual problem yet. Because now that I've dealt so intensively with the DirectoryFinder and the ExtensionService, I'd rather solve the problem with changes to the ExtensionService than to change the DirectoryFinder.

Why? Well, the DirectoryFinder.GetFiles() method is passed a starting directory and a search pattern. The method returns all files that match the search pattern. If this method would accept an absolute path as search pattern, this value would override the start directory. Even if this is possible, this would be surprising to me as a user of the method.

For me, a modification of ExtensionService to solve the problem is more elegant, since it aligns with the principle of least astonishment. How about this:

• ExtensionService checks if the current entry in the addins-file is an absolute path.
  
  
  
  • Yes: The complete part left of the first wildcard is used as start directory, the wildcard and the part right of it is used as search pattern.
  
  
  • No: The complete entry is forwarded to the DirectoryFinder (as before).
  
  
• To better test the interaction between ExtensionService and DirectoryFinder, a new interface IDirectoryFinder is introduced. Consequently, the internal constructor ExtensionService(bool, FileSystem) is changed to ExtensionService(bool, FileSystem, IDirectoryFinder). The public constructor then creates a DirectoryFinder in addition to the default file system.

What do you think, should the DirectoryFinder or the ExtensionService be adapted?

Independent of the above decision, I will create a pull-request for the testable DirectoryFinder. Since it heavily modifies the ExtensionService and DirectoryFinder, there should be a decision about the PR since this determines which code-base to use for the bugfix.

By the way, I noticed that on systems that use '/' as file-system-root no absolute paths can be used either. These are interpreted by DirectoryFinder as relative paths. Should we fix this behavior too or leave it as we don't know how many 'invalid' addins-files would become unusable by the bugfix?

[CharliePoole]

I like the idea of improved testability through an interfaced, which may be mocked in the tests.

One quibble: I don't think we should worry about confusing general users. This is an internal method, as indicated by the namespace. It's visibility is public only because we once wrote code that way and are only gradually changing things to internal. IMO, it's no more surprising that an absolute path would be used directly than it is when a programmer uses Path.Combine.

[ChrisMaddock]

Sorry @x789 - missed this one at the time.

What do you think, should the DirectoryFinder or the ExtensionService be adapted?

I think you make a good argument for ExtensionService here. Sounds sensible to me - I think you know these classes better than anyone right now!

By the way, I noticed that on systems that use '/' as file-system-root no absolute paths can be used either. These are interpreted by DirectoryFinder as relative paths. Should we fix this behavior too or leave it as we don't know how many 'invalid' addins-files would become unusable by the bugfix?

Hmmm - I guess we can't tell the difference here, right? I wonder how IsPathFullyQualified(String) works - presumably would return false? I feel like following default .NET behaviour is the sensible thing to do here.

[x789]

I will tackle the original issue first. I think absolute path support on Unix systems is priority 2.

The short answer to the question if we can use IsPathFullyQualified(string) is "yes".
The long answer is:
On systems that use '/' as root, any path that starts with a '/' is an absolute path.
On Windows systems, the first three characters of the path must be examined, with the second character being the DriveSeparator and the third character being the DirectorySeparator.

If you want to see how that part of IsPathFullyQualified(string) is implemented, check out the method IsPartiallyQualified in PathInternal.Windows.cs and PathInternal.Unix.cs.

[ChrisMaddock]

Fixed by #914, thanks @x789!