Azure Storage is still hard to unit test / mock

[PureKrome]

Hi Azure Storage team!

(question editted with extra info, about 10 mins after posting this).

This question is a continuation of #349, #318 and sorta #80

With release version 8.* of the Azure SDK more methods have been virtualized. Yay! better unit testing :)

But ... I'm not having much luck with some stuff still -> mainly when I need to access a property .. which relys on some internal magic ... like the Name of a CloudFile or CloudDirectory, etc. This is because the constructors require specific arguments passed in (I believe).

I think we might need some more things virtualized => I think sss needs to be virtualized.

So .. given the latest 8.* version of the SDK, how could this simple piece of code be Unit Tested?

Here is the code I wish to test: Given a Share in my Azure File Storage, list all the folders that exist in that share.

Here's my code which I believe does this...

public class AzureFileServiceExample
{
    private readonly CloudFileClient _cloudFileClient;

    public AzureFileServiceExample(CloudFileClient cloudFileClient)
    {
        _cloudFileClient = cloudFileClient;
    }

    public string[] GetDirectories()
    {
        // 1. Get a reference to the main, root share directory in yout azure storage account.
        var shareRootDirectory = _cloudFileClient.GetShareReference("ShareDirectoryInAzure")
                                                 .GetRootDirectoryReference();

        // 2. Get a list of all the directories in this share.
        var directories = shareRootDirectory.ListFilesAndDirectories()
                                            .OfType<CloudFileDirectory>()
                                            .Select(x => x.Name)
                                            .ToArray();

        return directories;
    }
}

---

Here's a start of some unit testing code to help kick start things to make your life easier :

[Fact]
public void GivenSomeFilesExist_GetDirectories_ReturnsSomeDirectoryNames()
{
    // Arrange.
    const string uri = "https://pewpew.file.core.windows.net/";

    var fakeStorageUri = new StorageUri(new Uri($"{uri}movies/"));
    var fakeStorageCredentials = new StorageCredentials();

    var existingFilesAndDirectories = new IListFileItem[]
    {
        new CloudFileDirectory(new StorageUri(new Uri($"{uri}movies/folder-a/")), fakeStorageCredentials),
        new CloudFileDirectory(new StorageUri(new Uri($"{uri}movies/folder-b/")), fakeStorageCredentials),
        new CloudFile(new StorageUri(new Uri($"{uri}movies/folder-a/file-a.xml")), fakeStorageCredentials),
        new CloudFile(new StorageUri(new Uri($"{uri}movies/folder-a/file-b.json")), fakeStorageCredentials),
        new CloudFile(new StorageUri(new Uri($"{uri}movies/folder-a/file-c.zip")), fakeStorageCredentials),
        new CloudFile(new StorageUri(new Uri($"{uri}movies/folder-b/file-1.xml")), fakeStorageCredentials),
        new CloudFile(new StorageUri(new Uri($"{uri}movies/folder-b/file-2.json")), fakeStorageCredentials)
    };

    var cloudFileDirectory = new Mock<CloudFileDirectory>(fakeStorageUri, fakeStorageCredentials);
    cloudFileDirectory.Setup(x => x.ListFilesAndDirectories(null, null))
                        .Returns(existingFilesAndDirectories);

    var cloudFileShare = new Mock<CloudFileShare>(fakeStorageUri, fakeStorageCredentials);
    // cloudFileShare.Setup(x => x.GetRootDirectoryReference())
    //               .Returns(cloudFileDirectory.Object);  // <-- Cannot work cause it's not virtual.

    var cloudFileClient = new Mock<CloudFileClient>(fakeStorageUri, fakeStorageCredentials);
    cloudFileClient.Setup(x => x.GetShareReference(UniqueKeys.AzureFileShareDirectoryKey))
                    .Returns(cloudFileShare.Object);

    var fileService = new AzureFileServiceExample(cloudFileClient.Object);

    // Act.
    var files = fileService.GetDirectories();

    // Assert.
   // lots of checks and verifies here.
}

Problem: Because we cannot mock the GetRootDirectoryReference() method, a real CloudFileDirectory instance is returned. Which means it will do real hits to the storage endpoints. Not kewl 😢

So - could we please add virtual to GetRootDirectoryReference() please?

Side Point: would also love the properties also made virtual also 👍 Like Name etc...

NOTE: I know this feels like a StackOverflow question, but I wanted to ask here because I'm really asking if the SDK has been designed enough to handle unit testing. Also, please do not suggest using the emulator in a unit testing (manual/local, CI/CD scenario's) situation please.

[kieronlanning]

+1 in some instances we've proxied loads of the storage SDK with our own classes and interfaces, just so we can test some crucial parts of our systems.

[PureKrome]

@kieronlanning I'm pretty sick of have a separate repo just with XXXWrapper classes over each main class (which we use) from the Storage SDK. e.g. CloudFileClientWrapper, etc

:/

Feeling like we're getting close though!

[PureKrome]

Any thoughts about this discussion @pemari-msft or @erezvani1529 ?

[erezvani1529]

Hi @PureKrome,

Apologies for the delay in response. Is there a reason you can't create a Mock CloudFileDirectory given the mocked share and the uri?

We would like to confirm that for the sake of improving the current unit testing experience we will consider and apply your feedback about the virtual properties in our next breaking release 9.0.

Thanks,
Elham

[PureKrome]

Hi @erezvani1529

Is there a reason you can't create a Mock CloudFileDirectory given the mocked share and the uri?

Um, I think I'm already doing this, above??

   var cloudFileDirectory = new Mock<CloudFileDirectory>(fakeStorageUri, fakeStorageCredentials);
    cloudFileDirectory.Setup(x => x.ListFilesAndDirectories(null, null))
                        .Returns(existingFilesAndDirectories);

Like that?

Then I create the mocked share..

var cloudFileShare = new Mock<CloudFileShare>(fakeStorageUri, fakeStorageCredentials);

And finally, I need to get a reference to the root directory of this mocked-share ... so I can then return my mocked cloud directory...

    // Cannot work because 'GetRootDirectoryReference' is not virtual.
    // cloudFileShare.Setup(x => x.GetRootDirectoryReference())
    //               .Returns(cloudFileDirectory.Object);

is this what you ment?

otherwise, can you provide some code that shows me what I'm doing wrong?

[erezvani1529]

Hi @PureKrome,

Apologies since I think I misunderstood your original question. You are right, this method should be virtualized as well, as part of virtualizing all Get*Reference methods.

So with this issue we are tracking virtualizing the below items:

1. Get*Reference APIs if there is anything that was not covered before
2. Properties

Please let me know if there is anything else so that we can consider and add it to the list above.

Thanks for your collaboration,
Elham

[PureKrome]

[Jericho]

FYI: I identified another sealed class in #514

[Jericho]

Issue #514 was closed and I was told to use this issue to track unsealing of classes and virtualizing of methods for milestone 9.0 so I am including the relevant information that I had included in #514:

I found one additional class marked as sealed which prevents unit testing: CloudStorageAccount. Additionally, the Create*Client methods in this class are not virtual which therefore prevents mocking.

Here's a contrived example where I am prevented from mocking the storage account and also mocking the blob client created by this storage account

public class Foo
{
    private readonly CloudStorageAccount _storageAccount;
    public Foo(CloudStorageAccount storageAccount)
    {
        _storageAccount = storageAccount;
    }
    public async Task DoSomething()
    {
        var blobClient = _storageAccount.CreateCloudBlobClient();
        ... do something useful with the blob client ...
    }
}

Can you please consider un-sealing the StorageAccount class in version 9.0?

[StenPetrov]

OperationContext also has one "sealed" too many.

There should be a requirement to put a comment explaining why is something sealed so that PRs can weed out unneeded "sealed" (pretty much all of them).
"internal" is another questionable modifier - if tests need to access a property or field chances are you can make it protected

[PureKrome]

There should be a requirement to put a comment explaining why is something sealed

aye! here here! Caused heaps of PITA.

[MovGP0]

There should be a requirement to put a comment explaining why is something sealed

I disagree with this one. There is a good reason for using sealed on something: you can make sure that no class derives from it. All my classes are either abstract or sealed. Still, my codebase is perfectly testable.

However, as a general rule you need to make sure that the SOLID Principles are kept:

• all classes should either contain state (Entities), or represent behavior (functions), but never both.
• all classes with behavior need to have an interface to allow mocking (entities may not have interfaces)
  • the base interface might combine multiple interfaces as needed

Classes like CloudStorageAccount are therefore a horrible construction, since it is a class that represents state and behavior, and also has no interfaces for mocking.

[DiaAWAY]

Currently writing some functions which uses Azure Storage accounts and would like to be able to do some mocking; any news on when this will be released?

Looks like Milestone 9 is 4 days overdue with 18 open issues :) Any ETA on this?

[erezvani1529]

Hi @DiaAWAY,

We are waiting on a breaking service version release for this version to be out. The current planned date is mid-January. (We do not want to break twice as the new service version includes some breaking changes so we are bundling them together :) ).

Thanks,
Elham

[erezvani1529]

We have released package v9.0.0 with a few improvements for mocking.

Thanks again for your feekback 👍 !

Please share any other feedbacks so that we can include them in our split packages (Blob, Queue, File) (currently in preview, hence no need to wait for major version release) for faster delivery.

Thanks!

[kieronlanning]

@erezvani1529 What sort of improvements? Looking at the changelog.txt in GitHub, I can't see anything relevant to mocking...

[erezvani1529]

@kieronlanning ,

Please checkout the BreakingChange list to find out about these improvements.

Thanks

[kieronlanning]

@erezvani1529 Gotcha, thank-you.

[spottedmahn]

Please checkout the BreakingChange list to find out about these improvements.

Is returning 404. I searched through the repo and couldn't find a file named BreakingChanges.txt.

[amnguye]

If you search the repo for BreakingChange, you will find one for Blob, Queue and File.