search

HTBox / allReady

This repo contains the code for allReady, an open-source solution focused on increasing awareness, efficiency and impact of preparedness campaigns as they are delivered by humanitarian and disaster response organizations in local communities.
http://www.htbox.org/projects/allready
MIT License
891 stars 626 forks source link

Unit test task attachments #1942

Open DanielSchiavini opened 7 years ago

DanielSchiavini commented 7 years ago

The code introduced in PR #1660 is not completely unit tested. We should at least introduce tests to TaskAttachmentService but preferably to the other classes involved in the PR.

mgmccarthy commented 7 years ago

@DanielSchiavini

We can't test TaskAttachmentService b/c it contains the real implementation for CRUD based blob storage in Azure. That's why TaskAttachementService is abstracted by ITaskAttachmentService. Any code that uses ITaskAttachementService should be tested/testable, but testing the service itself has no value.

The only thing I think could be testable by taking a look at the code is how we're building the path to the actual blob. That would be something I consider testable, but it's a very small piece and not really worth the effort.

That, OR we move the abstraction closer to the Azure API and abstract ONLY the Azure calls. For example, any code working with CloudBlobContainer and/or CloudBlockBlob would be abstracted away and our TaskAttachement service would use that abstraction in order to communicate with Azure.

So lines like this:

var blobContainer = CloudStorageAccount.Parse(_options.AzureStorage)
                .CreateCloudBlobClient()
                .GetContainerReference(ContainerName);
blobContainer.Uri.AbsoluteUri
blobContainer.GetBlockBlobReference
blockBlob.DeleteAsync()
container.CreateIfNotExistsAsync
container.GetBlockBlobReference
blockBlob.Properties.ContentType
blockBlob.UploadFromByteArrayAsync
blockBlob.Uri

would have to be abstracted. That's a lot of work, and as we use more and more Azure based functionality, this abstraction would only grow more unweildy... so again, I don't think there is value here.

Also, the Debug.WriteLine statements in this class should be deleted. We don't want that type of stuff in production code.