Improve FileRegionsCache testability

[ghost]

I'm writing some unit tests for the VS Extension that require the use of a FileRegionsCache object, a concrete class that interacts with the file system. I can create, and use in my tests, a FileRegionsCache object with a mock IFileSystem. But that requires me to understand the internal workings of FileRegionsCache so I can mock the correct set of file system calls, and I also have to provide fake file system content to the mock.

What I want/need instead is an IFileRegionsCache interface that I can mock, so that when the VS Extension product code that I'm unit testing calls fileRegionsCache.PopulateTextRegionProperties, I can directly return the value I want.

@michaelcfanning @eddynaka @GabeDeBacker FYI

[eddynaka]

Hi @lgolding ,

I was taking a look at this. I can expose the interface, but i will have to include one more method if that's not a problem. Below the interface:

public interface IFileRegionsCache
    {
        Region PopulateTextRegionProperties(Region inputRegion, Uri uri, bool populateSnippet);

        Region ConstructMultilineContextSnippet(Region inputRegion, Uri uri);
    }

Today the ConstructMultilineContextSnippet method is internal and I would have to move to public.

[michaelcfanning]

I'm not a big fan of using interfaces exclusively to drive testability. This results in a certain amount of type proliferation which generally only serves a single purpose (to help with testing). A more appropriate use of interfaces supports arbitrary additional uses.

What tends to happen in projects like this one is that the interfaces are created in a somewhat haphazard manner and applied similarly (i.e., the abstraction is pushed through the API only in reaction to whatever testing is done). Broad areas of the code base don't have this kind of interface use, with the result that the API itself isn't consistent.

My suggestion here is simply to ensure that FileRegionsCache is public, unsealed and to apply the 'virtual' keyword to relevant methods. This can be a simple, clean approach that allows easier testing. This approach can break down when attempting to test objects that are created indirectly by other classes/API but that doesn't seem to be the case here.

[eddynaka]

I'm not a big fan of using interfaces exclusively to drive testability. This results in a certain amount of type proliferation which generally only serves a single purpose (to help with testing). A more appropriate use of interfaces supports arbitrary additional uses.

What tends to happen in projects like this one is that the interfaces are created in a somewhat haphazard manner and applied similarly (i.e., the abstraction is pushed through the API only in reaction to whatever testing is done). Broad areas of the code base don't have this kind of interface use, with the result that the API itself isn't consistent.

My suggestion here is simply to ensure that FileRegionsCache is public, unsealed and to apply the 'virtual' keyword to relevant methods. This can be a simple, clean approach that allows easier testing. This approach can break down when attempting to test objects that are created indirectly by other classes/API but that doesn't seem to be the case here.

just created a second PR with this approach + capacity fix + docs update