Azure / azure-storage-net

Microsoft Azure Storage Libraries for .NET
Apache License 2.0
446 stars 373 forks source link

ICloudBlob does not contain any StartCopyAsync overloads #421

Closed iouri-s closed 6 years ago

iouri-s commented 7 years ago

In our unit tests, we mock the implementation of ICloudBlob, we need to be able to mock any overloads for that member. The deprecated and now removed member that this replaces (StartCopyFromBlobAync) was part of the interface.

erezvani1529 commented 7 years ago

Hi @iouri-s,

We would recommend that you replace ICloudBlob interface with CloudBlob concrete class which includes all the APIs you need. Please see this issue for more information and kindly let us know if you have a different scenario which can not work with CloudBlob instead of ICloudBlob.

Thanks, Elham

iouri-s commented 7 years ago

The scenario is that it is much easier to mock an interface than it is a concrete class. It was working before, it is sad that you should take it away. Concrete classes can only be mocked if all their methods and properties are virtual and that is not the case for CloudBlob, for example, Uri property is not virtual, nor does it have a public setter. Please keep and update the interfaces for testability.

doguva-msft commented 7 years ago

Just worth mentioning that, you could easily create the interface yourself from the concrete class in the SDK, Visual Studio 2015/2017 has out of shelf support for it and generates the interface for you automatically. So you could generate your own ICloudBlob interface from CloudBlob and use that interface instead in your higher level code. Of course that means you would need to have a proxy adapter layer between your own ICloudBlob interface and the CloudBlob class in the SDK but it may not be a bad thing because it will shield you from changes on the SDK as well as enable you to test your code. (via mocks)

iouri-s commented 7 years ago

Just worth mentioning that the language SDKs are a thin layer over the REST protocol which are supposed to provide a good programming experience. Not providing interfaces reduces their value in that regard. If in removing the interfaces the goal was to reduce the cost of maintenance, you could look at removing the countless method overloads present in both the interface and the concrete class, and implementing them as extension methods instead, the way AutoRest does it for management libraries. All Microsoft management libraries use interfaces, as do many (AFAIK all) the data plane libraries (DocumentDB, KeyVault and until now, Storage). Why would you want to pull them out? The proxy adapter layer you suggest is a kludge we use to test "legacy" code (peppered with mocking comments deploring the short-sightedness of whoever wrote it) an that's what we'll use for Storage if you leave us no choice.

doguva-msft commented 7 years ago

Just to clarify that I am not from the product team :), I am letting you know what worked for us in mocking azure external dependencies in the past. Document Db also does not provide interfaces for every public concrete class so whatever we needed from Doc Db SDK which is not covered by the out of shelf interface, we created our own interfaces for that matter.

iouri-s commented 7 years ago

That's true, we've had to wrap some parts of DocDB and Storage as well, even before StartCopyAsync. We did not enjoy having to do that, though...